Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial role verification cases #38925

Merged
merged 13 commits into from
Mar 28, 2023

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Mar 10, 2023

@zcorpan @gsnedders @jnurthen @spectranaut Though this doesn't complete all the roles tests, the two files in the diff are fairly self-contained. Other files still needed will include:

Expectation differences

Context complexity

  • ./grid-roles.html (e.g. an orphaned cell is not a cell, it needs to be in a table>row>cell context, etc.)
  • ./list-roles.html (this PR)
  • ./listbox-roles.html
  • ./menu-roles.html
  • ./roles.html (this PR)
  • ./tab-roles.html
  • ./tree-roles.html

If there a preference in WPT for smaller patches (easier to review) or one large patch?

I'm wondering if I should jam all these files in one branch, or split into smaller issues?

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Mar 10, 2023

/wai-aria/role/roles.html is the first file in the Interop 2023 Accessibility project that will show some real user-impactful implementation differences, so there may be some benefit to getting it into wpt.fyi earlier for the build-specific results. But I could go either way.

@cookiecrook cookiecrook changed the title simple role cases and region special cases initial role verification cases Mar 10, 2023
@zcorpan
Copy link
Member

zcorpan commented Mar 10, 2023

Multiple smaller PRs seems preferable for review purposes.

@cookiecrook
Copy link
Contributor Author

Okay, the new helper functions in AriaUtils should now cover most other types of role tests. Moving this PR out of Draft, and filing the remaining items listed above as separate issues.

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is great! Good start.

This is a not-exactly-specified thing we are testing here, as I think you know.. But motivated by this PR and your work, here is a draft for adding the concept of "computed roles" to the ARIA specs: w3c/core-aam#167 Possibly these things should land at the same time.

I'm also wondering if the implementation details for the browsers might run into some weird scenarios, where the browsers will have to do some extra work to return the correct "computed role" to webdriver.. essentially obfuscating their internal representation for the roles (instead of getting to the goal of normalizing the internal representation across browsers). I guess we will only come across these cases by seeing them, and deal with them when that happens, but here is one example from HTML AAM:

Footer has different HTML-AAM mappings when in the context of main or not. Internally, Chrome uses the roles "FooterAsNonLandmark" and "Footer" to represent the difference (codepen example
). I'd guess that WebKit doesn't need to have this distinction internally, though I'm not sure?

Anyway I'm not sure this is the right place to raise this concern, it's just on my mind as I add "computedRoles" to the CORE-AAM mapping tables.

I'm curious though, did you run these tests? Do all of the tests in this PR pass on chrome and webkit?

wai-aria/role/list-roles.html Show resolved Hide resolved
@cookiecrook
Copy link
Contributor Author

cookiecrook commented Mar 20, 2023

@spectranaut wrote:

I'm curious though, did you run these tests? Do all of the tests in this PR pass on chrome and webkit?

I ran them locally against Safari and in WPT against Chrome. You can see the results for each in the "All checks have passed" section below. Click "Show all checks" then pick your implementation (E.g., "safari[experimental]"), and select Details. In the resulting screen, click "Visual comparison of the results." If you want to dig into each subtest, you can click on the file name header for each row in the table. I plan to cover this in the May ARIA F2F session.

For this set, roles.html results differ: Chrome 56/56 and Safari is 51/56, which is accurate for Canary and STP. I used the missing 5 roles to find this old patch that stalled in review. Of note, generic landed in https://webkit.org/b/253751 code and textbox also landed. emphasis and strong are still pending.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Mar 21, 2023

I think I've removed the reviewers and keywords that were auto-added in the bad merge. Please re-add if relevant to do so. The last commit addresses @zcorpan's review feedback.

@zcorpan
Copy link
Member

zcorpan commented Mar 21, 2023

My workflow is:

  • clone the repo from web-platform-tests/wpt. No fork.
  • git checkout master
  • git pull --prune
  • git checkout my-branch
  • git rebase master

Doing something similar with a fork should work ok too. Maybe the issue is "update fork on website" (does it do a merge commit?).

@cookiecrook
Copy link
Contributor Author

Thanks.

Maybe the issue is "update fork on website" (does it do a merge commit?).

I thought I had it set to rebase rather than merge. I assumed updating the fork was necessary to have git fetch contain the original repo updates in an fetch from the fork origin, but maybe that's a bad assumption.

@zcorpan
Copy link
Member

zcorpan commented Mar 21, 2023

You can fetch from origin instead and use git reset --hard origin/master, see https://www.freecodecamp.org/news/git-reset-origin-how-to-reset-a-local-branch-to-remote-tracking-branch/#reset

Pushing the new master to your fork isn't strictly necessary.

@gsnedders
Copy link
Member

My workflow is:

  • clone the repo from web-platform-tests/wpt. No fork.
  • git checkout master
  • git pull --prune
  • git checkout my-branch
  • git rebase master

Doing something similar with a fork should work ok too. Maybe the issue is "update fork on website" (does it do a merge commit?).

You don't need to do anything near so much, assuming you already have my-branch checked out:

  • git fetch origin
  • git rebase origin/master
  • [verify with git log origin/master..HEAD]

Copy link
Member

@zcorpan zcorpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nit

"definition",
"deletion",
"dialog",
// "directory" -> FAIL. WONTFIX. Deprecated in ARIA 1.2; re-mapped to list role.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this still be tested to check that computed role is list? (i.e. synonym-roles.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsnedders gsnedders removed their request for review March 23, 2023 15:31
@cookiecrook cookiecrook merged commit e364f98 into web-platform-tests:master Mar 28, 2023
@cookiecrook cookiecrook deleted the computed-roles-v1 branch March 28, 2023 23:18
@cookiecrook
Copy link
Contributor Author

cookiecrook commented Mar 28, 2023

@gsnedders wrote:

You don't need to do anything near so much, assuming you already have my-branch checked out:

git fetch origin
git rebase origin/master

But that only fetches changes from my fork origin (/cookiecrook/wpt), not those that have since merged into the upstream origin (/web-platform-tests/wpt)

cookiecrook added a commit to cookiecrook/wpt that referenced this pull request Mar 29, 2023
* simple role cases and region special cases

* synonym roles tests, reviewer+, and minor errata

* synonym roles tests, reviewer+, and minor errata

* use assert_equals instead of assert_true

* use tagName with implicit role to test non-generic fallback

* remove synonym role test; will spawn a new issue

* add list roles and break scripts into a shared utils file

* m. whitespace.

* new roles ReadMe

* clarity in attr name and removing code duplication

* m. lint tabs -> spaces

* review feedback from @zcorpan; include a permanent and unique test name for dashboard/logs.

* Gecko-specific fix from @zcorpan
@zcorpan
Copy link
Member

zcorpan commented Mar 30, 2023

@cookiecrook in the future I would prefer if you can avoid at-mentioning in commit messages, since it leads to notification spam. Thanks!

@zcorpan
Copy link
Member

zcorpan commented Mar 30, 2023

But that only fetches changes from my fork origin (/cookiecrook/wpt), not those that have since merged into the upstream origin (/web-platform-tests/wpt)

Remotes can be called anything; in @gsnedders case the upstream repo is called origin and the fork is called something else, but maybe you have your fork called origin and upstream called upstream, in which case, use that name instead.

You can see how it's set up with git remote -v

@cookiecrook
Copy link
Contributor Author

Thanks. For future ref, I did:

git co master
git remote add upstream git@github.com:web-platform-tests/wpt.git
git fetch upstream
git rebase upstream/master
git push origin master

cookiecrook added a commit to cookiecrook/wpt that referenced this pull request Apr 8, 2023
* simple role cases and region special cases

* synonym roles tests, reviewer+, and minor errata

* synonym roles tests, reviewer+, and minor errata

* use assert_equals instead of assert_true

* use tagName with implicit role to test non-generic fallback

* remove synonym role test; will spawn a new issue

* add list roles and break scripts into a shared utils file

* m. whitespace.

* new roles ReadMe

* clarity in attr name and removing code duplication

* m. lint tabs -> spaces

* review feedback from @zcorpan; include a permanent and unique test name for dashboard/logs.

* Gecko-specific fix from @zcorpan
<body>

<!-- no label -->
<nav role="region" data-testname="region without label" data-expectedrole="navigation" class="ex">x</nav>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting case. In Gecko, computedRole currently returns "region" here. While it's clear from the HTML AAM spec that <section> without a label shouldn't return "region", I'm not sure where it's specified that role="region" without a label shouldn't map to "region". Core AAM says that region without a label shouldn't be exposed as a landmark to individual platforms, but it says nothing about the fact that it shouldn't be exposed as a "region" as far as ARIA (not the platform APIs) are concerned. You can intuit the latter from the former, but unless I'm missing something, there's no spec concept which allows a role to be completely ignored/invalidated based on a missing name. There's "Name required true", but that's specified for button as well, and I'm not sure we'd want to completely ignore the role for an unlabelled button.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly a mistake. Since this PR is already merged, I filed your comment as a new issue:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing the loop here too. The test is correct as written.

Certain landmark roles require names from authors. In situations where an author has not specified names for these landmarks, it is considered an authoring error. The user agent MUST treat such elements as if no role had been provided. If a valid fallback role had been specified, or if the element had an implicit ARIA role, then user agents would continue to expose that role, instead. Instances of such roles are as follows:

Citation: https://w3c.github.io/aria/#document-handling_author-errors_roles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Role Rules from the ARIA Spec (Fallback Role, Host Language Conflict, Invalid and Abstract Roles, etc.)
8 participants