feat(checks/label): support ARIA element internals properties#5170
Conversation
Convert aria-describedby and aria-labelledby attribute reads in title-only, help-same-as-label, and multiple-label checks to use getResolvedRefs, hasAriaValue, and getAriaValue so that values set via ElementInternals are also detected.
chutchins25
left a comment
There was a problem hiding this comment.
Nice work — clean migration to the element-internals helpers, and the new testutils-element tests cover the internals path for each check. Two low-severity questions below, neither blocking.
| const labelText = labelVirtual(virtualNode); | ||
| const title = titleText(virtualNode); | ||
| const ariaDescribedBy = virtualNode.attr('aria-describedby'); | ||
| const ariaDescribedBy = hasAriaValue(virtualNode, 'aria-describedby'); |
There was a problem hiding this comment.
Not a blocker, just a consistency question — hasAriaValue is presence-based (it returns true for an empty aria-describedby=""), whereas the previous .attr('aria-describedby') truthy check treated an empty attribute as absent. So an element with an empty aria-describedby and no title/label would now flip title-only from false to true. Such an element has no accessible name anyway, so the practical impact is negligible — was the presence-based semantics (vs. value-truthy) intended here?
There was a problem hiding this comment.
Agreed. This isn't right.
There was a problem hiding this comment.
Resolved — switched to getAriaValue and check .value, so an empty aria-describedby is treated as absent again.
| // make sure the ONE AT visible label is in the list of idRefs of aria-labelledby | ||
| const labelledby = idrefs(node, 'aria-labelledby'); | ||
| return !labelledby.includes(ATVisibleLabels[0]) ? undefined : false; | ||
| const vNode = getNodeFromTree(node); |
There was a problem hiding this comment.
Not a blocker, just a simplification — getResolvedRefs accepts a raw DOM node and calls nodeLookup internally, so you can pass node directly and drop the getNodeFromTree import/call (const labelledby = getResolvedRefs(node, 'aria-labelledby')). Up to you if the intermediate vNode reads more clearly.
There was a problem hiding this comment.
Resolved — passing node directly to getResolvedRefs and dropped the getNodeFromTree call.
…a-describedby Use getAriaValue instead of hasAriaValue so an empty aria-describedby is treated as absent, and simplify multiple-label to pass the DOM node directly to getResolvedRefs.
WilcoFiers
left a comment
There was a problem hiding this comment.
LGTM. I would suggest pulling the comment, but I don't feel strongly about that. Couple issues to open though.
| // ariaLabelledByElements (reflected AOM property) coverage adapted from | ||
| // @jcfranco's work in #5187 (issue #4943) |
There was a problem hiding this comment.
| // ariaLabelledByElements (reflected AOM property) coverage adapted from | |
| // @jcfranco's work in #5187 (issue #4943) |
We can do attribution in other ways. Doesn't belong in the code.
| const ATVisibleLabels = labels.filter(label => | ||
| isVisibleToScreenReaders(label) | ||
| ); | ||
| // more than 1 AT visible label will fail IOS/Safari/VO even with aria-labelledby |
There was a problem hiding this comment.
I'm skeptical this is still true. Would you mind opening an issue for us to investigate?
| const ariaDescribedBy = getAriaValue(virtualNode, 'aria-describedby'); | ||
|
|
||
| return !labelText && !!(title || ariaDescribedBy); | ||
| return !labelText && !!(title || ariaDescribedBy?.value); |
There was a problem hiding this comment.
This is buggy (not new). We should consider building a proper accessibleDescription method and use that. Would you mind opening an issue for this?
There was a problem hiding this comment.
Opened #5212 to track building a proper accessibleDescription method and switching this check to use it.
Updates the
checks/labeldirectory to use the newgetAriaValue,hasAriaValue, andgetResolvedRefsfunctions where it makes sense.Here's a report of the full directory:
checks/label/help-same-as-label- aria-describedby: replacednode.getAttribute+idrefs()withhasAriaValue+getResolvedRefs; sincegetResolvedRefsreturns VirtualNodes, updatedaccessibleText(thing)toaccessibleText(thing.actualNode)checks/label/multiple-label- aria-labelledby: replacedidrefs()withgetResolvedRefs; since VirtualNodes are returned, updated.includes(domNode)to.some(ref => ref?.actualNode === domNode)checks/label/title-only- aria-describedby: replaced.attr()truthy check withhasAriaValuesince only presence is neededchecks/label/alt-space-value- checks alt attribute, not an ARIA propchecks/label/duplicate-img-label- compares accessible names, no direct ARIA attr accesschecks/label/explicit- checks label[for] association, no ARIA attr accesschecks/label/hidden-explicit-label- checks label visibility, no ARIA attr accesschecks/label/implicit- checks implicit label wrapping, no ARIA attr accesschecks/label/label-content-name-mismatch- computes accessible names, no direct ARIA attr accesschecks/label/no-implicit-explicit-label- no direct ARIA attr access (in aria directory, not label)Closes: #5146