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

Add link, clarifying justification for orphaned role tests #45032

Merged

Conversation

nmlapre
Copy link
Contributor

@nmlapre nmlapre commented Mar 11, 2024

UPDATE

This PR adds comments explaining the purpose of and justification for the orphaned role tests in various files. This comment includes a link to the relevant piece of the CORE-AAM specification.

The previous version of this PR moved the orphaned tests. See that description below.


Original Intent

Closes web-platform-tests/interop-accessibility#109

See longer description of the issue: web-platform-tests/interop-accessibility#109
See test change proposal issue: web-platform-tests/interop#645
See related ARIA issue: w3c/aria#2137

This PR moves disputed/invalid orphaned role tests to a unified tentative test file. These cases are considered author errors, which, as of the current ARIA spec language, is explicitly not the responsibility of user agents to remedy.

This PR does move some tests that weren't previously running in their source files (due to lack of verify[Generic]RolesBySelector calls). I presume those were meant to be running. Either way, they suffer from the same issues and should be grouped in the tentative file.

By my count, this should remove 17 currently-running tests from WPT. All tested browsers are currently failing these tests, so I expect a ~1.4% increase in the accessibility focus area score across the board when this merges.

This is my first non-automated WPT PR, so please assign reviewers as needed. Thanks!

@cookiecrook
Copy link
Contributor

Requesting @scottaohara's review in case there are some orphaning rules in HTML-AAM that we need to keep tests for that I'm not remembering. I'll do my best to review in the context of the ARIA Core spec.

@cookiecrook
Copy link
Contributor

And thanks for posting this PR!

Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Looks okay to me once @scottaohara approves for HTML-AAM.

@@ -18,12 +18,9 @@
<div role="listitem" data-testname="last simple listitem" data-expectedrole="listitem" class="ex">x</div>
</div>

<div role="listitem" data-testname="orphan div with listitem role" class="ex-generic">x</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch that this one was not running due to the lack of verifyGenericRoleByselector();!

Copy link
Contributor

@cookiecrook cookiecrook Mar 18, 2024

Choose a reason for hiding this comment

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

Just a reminder that your prior PR fixed (in a new file) the lack of verifyGenericRoleByselector() in a few files. You can add those back in here if you like, or we can handle it in:

Either is fine with me. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, thank you! I think I'll handle it in web-platform-tests/interop-accessibility#111, just to separate things a bit between (1) resolution of spec question and (2) enabling tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed (2) in the PR here: #45174

@@ -82,13 +82,9 @@
<div role="tabpanel" data-testname="role is tabpanel as sibling to ul with child role none li elements" data-expectedrole="tabpanel" class="ex">Tab one's stuff</div>
<div role="tabpanel" data-testname="role is tabpanel as sibling to ul with child role none li elements (duplicate)" data-expectedrole="tabpanel" class="ex">Tab two's stuff</div>

<!--orphan tab semantics -->
<button role="tab" data-testname="orphan button with tab role" data-expectedrole="button" class="ex">x</button>
<span role="tab" data-testname="orphan span with tab role" class="ex-generic">x</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch that this one was not running due to the lack of verifyGenericRoleByselector();!

Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Hang on.... there are some relevant places in Core-AAM...

When an element has a role but is not contained in the required context (for example, an orphaned listitem without the required accessible parent of role list), User Agents MUST ignore the role token, and return the computedrole as if the ignored role token had not been included.

<div role="listitem"> <!-- Author error: orphaned listitem. computedrole returns "generic" -->

<div role="list"> <!-- computedrole returns "list" -->
  <div role="listitem"> <!-- computedrole returns "listitem" in the required context. -->

Cite: https://w3c.github.io/core-aam/#roleMappingComputedRole

So this needs more discussion and consideration before landing.

@cookiecrook
Copy link
Contributor

@twilco brought up a related case where the list markup is invalid, but one or more engines still render the bullet, and therefore may need an exception in the spec, if the implementations agree.

<ul>
  <div>
    <li>one</li>
  </div>
  <li>two</li>
  <li>three</li>
</ul>

And I'm still looking for an RFC-2119 statement that covers the native containment case without an explicitly-defined role attr.

@nmlapre
Copy link
Contributor Author

nmlapre commented Mar 15, 2024

Hang on.... there are some relevant places in Core-AAM...

Ah! Thanks for finding that. That language seems pretty cut-and-dry to me.

I suppose I'm wondering who wins out between the ARIA's "authors MUST" / "user agents are not responsible" language and CORE-AAM's "user agents MUST" wording. Does that tension require a spec clarification? Either way, I now think this PR is probably not the right thing to do.

@cookiecrook
Copy link
Contributor

cookiecrook commented Mar 15, 2024

Yeah, there is still some spec clean-up to do, but I agree the PR can't land as is. Still looking for some other cross-references between other meetings.

If nothing else, the PR could just add a commented link to that section of Core-AAM near the orphaned case, so it will be obvious to the next engineer who would otherwise ask about it.

@cookiecrook
Copy link
Contributor

That language seems pretty cut-and-dry to me.

And FWIW, if there is a significant implementation challenge or cost, we can still raise that for spec reconsideration.

@nmlapre nmlapre force-pushed the orphaned-role-tests-tentative branch from c93b909 to b9e34c5 Compare March 15, 2024 22:20
This commit adds comments explaining the purpose of and justification
for the orphaned role tests in various files. This comment includes a
link to the relevant piece of the CORE-AAM specification.
@nmlapre nmlapre force-pushed the orphaned-role-tests-tentative branch from b9e34c5 to c53ceb2 Compare March 15, 2024 22:27
@nmlapre nmlapre changed the title Move orphaned role tests to tentative file Add link clarifying justification for orphaned role tests Mar 15, 2024
@nmlapre nmlapre changed the title Add link clarifying justification for orphaned role tests Add link, clarifying justification for orphaned role tests Mar 15, 2024
@nmlapre
Copy link
Contributor Author

nmlapre commented Mar 15, 2024

If nothing else, the PR could just add a commented link to that section of Core-AAM near the orphaned case, so it will be obvious to the next engineer who would otherwise ask about it.

Alrighty, done! Updated this PR with comments to that effect.

And FWIW, if there is a significant implementation challenge or cost, we can still raise that for spec reconsideration.

The patch for this in Gecko was simple enough to write, but it's definitely more tree walking. I haven't profiled it to see whether it's a serious performance problem. See my comment in w3c/aria#2137 (comment).

@@ -18,12 +18,9 @@
<div role="listitem" data-testname="last simple listitem" data-expectedrole="listitem" class="ex">x</div>
</div>

<div role="listitem" data-testname="orphan div with listitem role" class="ex-generic">x</div>
Copy link
Contributor

@cookiecrook cookiecrook Mar 18, 2024

Choose a reason for hiding this comment

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

Just a reminder that your prior PR fixed (in a new file) the lack of verifyGenericRoleByselector() in a few files. You can add those back in here if you like, or we can handle it in:

Either is fine with me. Thanks.

@nmlapre nmlapre merged commit 4efaaf8 into web-platform-tests:master Mar 18, 2024
19 checks passed
@nmlapre nmlapre deleted the orphaned-role-tests-tentative branch March 18, 2024 17:28
BruceDai pushed a commit to BruceDai/wpt that referenced this pull request Mar 25, 2024
…orm-tests#45032)

This commit adds comments explaining the purpose of and justification
for the orphaned role tests in various files. This comment includes a
link to the relevant piece of the CORE-AAM specification.

Co-authored-by: Nathan LaPre <nathan@nathanlapre.com>
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.

Remove orphaned children accessible tests, or clarify ARIA spec
4 participants