-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Tentative: match new PR about 2E "unless the element is marked as presentational" #50002
base: master
Are you sure you want to change the base?
Conversation
Hi @giacomo-petri, I'm coming from the review of PR 2405. This looks good to me for the most part, Scott's addressed some of the initial confusion. I noted some semantic confusion in the naming of the test sections vs what is being tested. While Unless there's something I'm missing, the heading and test setup should match to avoid confusion for folks when reading. Thank you for this work! |
Done |
Thanks @giacomo-petri! You have some lint errors, once you've addressed those I'll be happy to approve this. |
Thanks all:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Almost ready!
Co-authored-by: James Craig <[email protected]>
Co-authored-by: James Craig <[email protected]>
- added readonly testcases with and w/o disabled - replaced [role none] witg [role=none] in data-testname
replaced role none with role=none
added cross reference comment
add cross reference comment
expectations moved from svg to circle
table fix (added THs and TDs)
Fix the lint errors and the bots will kick off another attempt to get that last CI result. |
Lint errors fixed! |
<h2>HTML fieldset role="none" wrapping legend with role="none"</h2> | ||
<fieldset role="none" data-expectedlabel="" data-testname="html: fieldset[role=none] wrapping legend[role=none]" class="ex-label"><legend role="none">legend</legend></fieldset> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: These comments are out of order on the Conversation tab... Review comments in order on the Files Changed tab.
</table> | ||
|
||
<h2>HTML table role="none" wrapping caption with role="none"</h2> | ||
<table role="none" data-expectedlabel="" data-testname="html: table[role=none] wrapping caption[role=none]" class="ex-label"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
</table> | ||
|
||
<h2>HTML table with role="none" wrapping caption</h2> | ||
<table role="none" data-expectedlabel="" data-testname="html: table[role=none] wrapping caption" class="ex-label"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as "invalidated" comment above.
<h3>HTML label encapsulation</h3> | ||
<label> | ||
<span>input label</span> | ||
<input type="text" data-expectedlabel="input label" data-testname="html: input[role=none] with label encapsulation" class="ex-label" role="none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
<h2>HTML input with role="none" and label with role none (presentational roles conflict resolution)</h2> | ||
<h3>HTML input label/for</h3> | ||
<label role="none" for="lirp">input label</label> | ||
<input id="lirp" type="text" data-expectedlabel="input label" data-testname="html: label[for][role=none] input[role=none]" class="ex-label" role="none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
<h2>HTML input with role="none" and disabled with label associated</h2> | ||
<h3>HTML input label/for</h3> | ||
<label for="irpd">input label</label> | ||
<input id="irpd" type="text" data-expectedlabel="" data-testname="html: label[for]input[role=none][disabled]" class="ex-label" role="none" disabled> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
<h3>HTML label encapsulation</h3> | ||
<label> | ||
<span>input label</span> | ||
<input type="text" data-expectedlabel="" data-testname="html: input[role=none][disabled] with label encapsulation" class="ex-label" role="none" disabled> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
</label> | ||
|
||
<h2>HTML img with role="none" and non empty alt</h2> | ||
<img src="https://www.w3.org/assets/logos/w3c/w3c-no-bars.svg" role="none" alt="w3c logo" data-expectedlabel="" data-testname="html: img[role=none][non-empty alt]" class="ex-label"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
<h2>HTML input with role="none" and label associated (presentational roles conflict resolution)</h2> | ||
<h3>HTML input label/for</h3> | ||
<label for="irn">input label</label> | ||
<input id="irn" type="text" data-expectedlabel="input label" data-testname="html: label[for]input[role=none]" class="ex-label" role="none"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal here that the none
role is invalidated by the input's focusability? If so, say so in the comment or test name. (FYI, test name is not a valid CSS selector. IIRC, you'd need a +
before input
to match the adjacent sibling selector.)
Perhaps data-testname="html:label[for]+input[role=none] (role invalidated due to focusability)"
Otherwise, if none
is the expected role, I don't think this is a valid test, because there is no mandate in WebDriver to have a backing object on an ignored element or to return a label from the non-existent object; not even an empty string. If label from a 'none' role is the intention, I would remove this test and possible refile as a new "blocked by testability" issue.
<table data-expectedlabel="caption" data-testname="html: table wrapping caption[role=none]" class="ex-label"> | ||
<caption role="none">caption</caption> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scottaohara Do you concur with this test expectation?
Closes: web-platform-tests/interop-accessibility#167
Relates and match: w3c/aria#2405
Ambiguity: https://www.w3.org/TR/accname-1.2/#comp_host_language_label
This PR adds tests for:
Note: since I created tests for acc name related to presentational roles conflict resolution, I have also included tests to verify that the computed role aligns with the expected result.