-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 tests for accnames from hidden labels, captions, and legends #44965
base: master
Are you sure you want to change the base?
Add tests for accnames from hidden labels, captions, and legends #44965
Conversation
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.
Looks okay to me, but not merging immediately since we only got one CI run.
would this PR be the place to also create the tests for how browsers should use other naming mechanisms if these elements are hidden? e.g.,
|
So... I took ☝🏻 this ball and ran with it... maybe in the wrong direction. 😵💫 I focused on HTML-AAM’s “4.1 Accname computation by element” section and wrote/copypasta’d a boatload of new tests for each specific element identified there. Every test specimen follows the same pattern: I simultaneously throw all possible accname techniques at the element, then I assert which one should win based on the conditionals described in the spec. Next, I eliminate 1 technique at a time for subsequent test specimens, rinse and repeat until the element has no remaining accname candidates. I took a few notes along the way for a few tweaks I might propose in the HTML-AAM spec. I also left 2 sections incomplete:
Anyhow, I’ll pause here. The test results are interesting. Please let me know what you think, @scottaohara / @cookiecrook. |
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.
Updating because this PR increased in scope since my last review. I shared offline feedback with Adam: mainly that the numbered directories might be fragile (as the spec changes) and that they may complicate the browsing experience on wpt.fyi. He also plans to utilize a new JavaScript convenience method to reduce some of the markup duplication.
Alrightey, @cookiecrook and @scottaohara, I just committed a monster of an update. 😬 This change:
Now that the subtests are being built dynamically, I was able to quickly introduce a bunch more name-supporting HTML elements that I‘d neglected in my previous push (e.g., As a result, the number of subtests have almost doubled from 276 to 510. I also scattered a few
There are also a number of tests that are failing because of web driver errors, such as At the end of all of this, I’m looking at HTML-AAM’s Accessible Name Computations By HTML Element section and thinking it could be simplified. Most of the computation steps follow a very linear path through a prioritized set of potential naming sources. My gut says we can define a very small number of “rule sets” and then make a table to pair each nameable HTML element with one of those rule sets. cc: @rahimabdi |
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.
incomplete review, and I plan to come back to it, but wanted to share feedback as I can since my work time is limited.
<h2>Input with hidden label</h2> | ||
<label id="hidden-label" for="input-01" hidden>Hidden label</label> | ||
<input | ||
id="input-01" | ||
class="ex-label" | ||
data-testname="Input that is aria-labelledby hidden label" | ||
data-expectedlabel="Hidden label" | ||
aria-labelledby="hidden-label" | ||
> | ||
|
||
<h2>Table with hidden caption</h2> | ||
<table | ||
class="ex-label" | ||
data-testname="Table that is aria-labelledby hidden caption" | ||
data-expectedlabel="Hidden caption" | ||
aria-labelledby="hidden-caption" | ||
> | ||
<caption id="hidden-caption" hidden>Hidden caption</caption> | ||
</table> | ||
|
||
<h2>Fieldset with hidden legend</h2> | ||
<fieldset | ||
class="ex-label" | ||
data-testname="Fieldset that is aria-labelledby hidden legend" | ||
data-expectedlabel="Hidden legend" | ||
aria-labelledby="hidden-legend" | ||
> | ||
<legend id="hidden-legend" hidden>Hidden 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.
New tests to a file tracked in the current Interop project would only be accepted if they didn't introduce new failures mid-year. In this case, the 3 new tests are all passing in Chromium, Gecko, and WebKit.... (Edge unknown but likely passing?) so I think it's okay to include this in the PR... With the caveat that we could roll it back in the unlikely chance it causes a failure for Edge.
In the future, it may be easier to include Interop-tracked changes as a separate PR from ones that do not affect the current Interop project.
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.
Ah, understood, thanks. I’ll make that distinction for future PRs and check in with you if I have doubts.
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.
In cases where you need to add a new valid-but-failing tests, add a new file that uses the same file name +".tentative" and have it only include the new failing tests... e.g. comp_hidden_not_referenced.tentative.html
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.
Got it. 👍🏻 I did just manually confirm that latest Edge is passing all 3 new tests, so... no harm no foul? But I’ll stay ready to roll it back regardless.
<h2>Input with hidden label</h2> | ||
<label id="hidden-label" for="input-01" hidden>Hidden label</label> | ||
<input | ||
id="input-01" | ||
class="ex-label" | ||
data-testname="Input that is aria-labelledby hidden label" | ||
data-expectedlabel="Hidden label" | ||
aria-labelledby="hidden-label" | ||
> | ||
|
||
<h2>Table with hidden caption</h2> | ||
<table | ||
class="ex-label" | ||
data-testname="Table that is aria-labelledby hidden caption" | ||
data-expectedlabel="Hidden caption" | ||
aria-labelledby="hidden-caption" | ||
> | ||
<caption id="hidden-caption" hidden>Hidden caption</caption> | ||
</table> | ||
|
||
<h2>Fieldset with hidden legend</h2> | ||
<fieldset | ||
class="ex-label" | ||
data-testname="Fieldset that is aria-labelledby hidden legend" | ||
data-expectedlabel="Hidden legend" | ||
aria-labelledby="hidden-legend" | ||
> | ||
<legend id="hidden-legend" hidden>Hidden 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.
In cases where you need to add a new valid-but-failing tests, add a new file that uses the same file name +".tentative" and have it only include the new failing tests... e.g. comp_hidden_not_referenced.tentative.html
'thead': { parents: ['table'] }, | ||
'tbody': { parents: ['table'] }, | ||
'tfoot': { parents: ['table'] }, |
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.
I'm not certain these tests would be valid w/o the relevant descendants, would they? IIRC, engines invalidate some containers if other required context is missing or invalid, so a table
that contained no cell children would not be a table, and therefore your downstream thead
expectations may be invalid.
Note: I'm reading this first one as testing <table><thead></thead></table>
so if I'm missing some cell additions later, please correct.
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.
Oof, I didn’t even consider that. I’ve filled out the table structures for all the table
-related subtests.
I did a quick once-over for the other similarly non-trivial markup structures — area
, ruby
, summary
, etc. — and I believe those are all sufficiently complete.
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.
Now that’s interesting. After implementing your suggestion here, Chrome has 2 new failures and Firefox has 1.
With the previous (incomplete) subtests, everything passed in flying colors.
With the table
elements now fully fleshed out, Chrome is happy to let both th
and td
get their names from their contents, which the HTML-AAM spec doesn’t seem to permit. This is also true for Firefox, but only for th
.
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.
I would then recommend marking this file (or just those tests in a new file) as tentative. Then spawn a new ARIA issue for the WG to determine what the "correct" behavior should be.
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, @cookiecrook. I’ve marked that file as tentative.
I also found that issue appears to exist already in w3c/html-aam#543, so I left a comment to mention this test.
] | ||
} | ||
HtmlAamAccnameUtils.buildNameComputationTests(testConfig); | ||
|
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.
After a shorter example source (e.g. "from title" only), it may be good to show the rendered DOM element(s) that will be generated from that source. E.g. img
with parent a href
using 'from title`
PS. can you use a href
in the parent params, or would it need to be an unlinked anchor? Same question on img
with an input type=button
parent.
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.
If I’m understanding correctly, this might be pushing my scope even further beyond the original intent? 😅
It’s my own damn fault, really, but — at least for polishing off this PR — I’ve mainly been focused on generating “simple” subtests for the relatively straightforward conditions specified in HTML-AAM 4.1, particularly in terms of the absence of potential naming sources. Just fleshing out those alone created 500+ subtests. 🤯 And I’m quite certain I’ve even missed some HTML elements as it is.
PS. can you use a href in the parent params, or would it need to be an unlinked anchor? Same question on img with an input type=button parent.
I’m afraid not, currently — same answer as above. But I’d like to add that! I’d like to go even more deeply into interesting subtree-sensitive permutations like a[href][title] > img
, but maybe that could be a 2.0?
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.
Fair enough. This suggestion is still reasonable though, right?
After a shorter example source (e.g. "from title" only), it may be good to show the rendered DOM element(s) that will be generated from that source. E.g. img with parent a href using 'from title`
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.
Yes, certainly! I’ve added an unnamed image to have something visible to render in tests that would otherwise be completely invisible. Here’s a screen cap of how that appears locally in Safari:
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.
follow-up comments interspersed
<body> | ||
|
||
<h1>Accname tests for hidden elements that would otherwise provide names to an associated element</h1> | ||
<p>These tests are a complement to <a href="https://github.com/w3c/html-aam/pull/533">w3c/html-aam#533</a>.</p> |
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.
re-reading the PR, i'm worried that one of the intended updates was not clear, as i don't see it represented in these tests (unless i missed it - i'm just reviewing the tests via github diff right now)
but for the hidden caption/legend examples, i would have liked there to be another instance where there was a hidden instance, and then a second instance that should then name the table/fieldset.
e.g.,
<table>
<caption hidden>not the name</caption>
<caption>is the name</caption> <!-- this SHOULD be the name now -->
<!-- rest of the table stuff goes here -->
</table>
right now, i wouldn't be surprised if this fails in some/all browsers, so per @cookiecrook's prior comment, maybe this needs to just be a second PR.
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, @scottohara. I’ve added two new subtests for that case in the static hidden-names.html
test file. As you suspected, the browser results are interesting.
In my auto-generated subtests for table
/caption
and fieldset
/legend
, I also test HTML-AAM’s clause about using “the subtree of the first such element”. All 3 browsers do pass for both the fieldset
and the table
.
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.
Maybe just rename the hidden-names file to hidden-names-tentative?
Accname tests to support w3c/html-aam#533’s clarification that hidden
label
s,caption
s, andlegend
s will not provide an accname to their naturally associated elements unless they are explicitly referenced usingaria-labelledby
.