fix: honor ariaLabelledByElements in label resolution - #5187
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR updates axe-core’s label/accessibility-name resolution to honor the ARIA AOM ariaLabelledByElements (and, more generally, prefer reflected idrefs properties via getResolvedRefs) so elements referenced through the AOM are used when computing labels.
Changes:
- Switched
aria-labelledbyresolution in label/name helpers from attribute-onlyidrefstodom.getResolvedRefs(which prefers AOM idrefs properties likeariaLabelledByElements). - Updated the
multiple-labelcheck evaluation to use resolved refs (including AOM overrides) when validating which label is referenced. - Added/extended unit tests across commons and checks to cover
ariaLabelledByElementsprecedence scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/commons/aria/label-virtual.js | Use getResolvedRefs for aria-labelledby so ariaLabelledByElements is honored. |
| lib/commons/aria/arialabelledby-text.js | Resolve labelledby references via getResolvedRefs (supports AOM property even when attribute is unset). |
| lib/checks/label/multiple-label-evaluate.js | Use getResolvedRefs when checking whether the AT-visible label is referenced. |
| test/commons/text/label-virtual.js | Add tests ensuring ariaLabelledByElements overrides aria-labelledby/aria-label in label resolution. |
| test/commons/dom/get-resolved-refs.js | Add test asserting idrefs property is preferred over attribute for aria-labelledby. |
| test/commons/aria/label-virtual.js | Add tests ensuring ARIA label virtual prefers ariaLabelledByElements. |
| test/commons/aria/arialabelledby-text.js | Add tests covering ariaLabelledByElements when attribute is unset and when both are present. |
| test/checks/label/multiple-label.js | Add check-level tests for ariaLabelledByElements overriding attribute behaviour. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| vNode.props.nodeType !== 1 || | ||
| context.inLabelledByContext || | ||
| context.inControlContext || | ||
| !vNode.attr('aria-labelledby') | ||
| context.inControlContext | ||
| ) { | ||
| return ''; | ||
| } |
|
Thanks for submitting a patch. Right now we have a lot of work going on for Element Internals support. So I think we first need to validate that this is not overlapping with any of that. The person driving that effort is out this week, so he will check into this sometime next week to see what should be done. |
|
@Garbee Sounds good. Thanks for the heads-up! |
Add consumer-level tests for the reflected ariaLabelledByElements property (precedence over aria-labelledby and aria-label) in arialabelledbyText and labelVirtual. getResolvedRefs already honors the property; this regression- tests the #4943 scenario. Tests adapted from #5187. Co-authored-by: JC Franco <[email protected]>
Add coverage for the reflected ariaLabelledByElements property overriding aria-labelledby in the multiple-label check. getResolvedRefs already honors the property; this regression-tests the #4943 scenario. Tests adapted from #5187. Co-authored-by: JC Franco <[email protected]>
|
Thanks for this, @jcfranco — nice instinct landing on the same Your consumer-level tests were exactly what those PRs were missing, so I've folded them into both — the Since the code's already covered, this PR will likely be closed as redundant once #5170/#5171 land — but I'll leave that call (and how we close out #4943) to @straker, who's coordinating the ElementInternals work. Either way, your tests live on in those PRs. Thanks again! |
|
Since this work was pulled into other prs I'm going to close this one. Thanks for contributing. |
Add consumer-level tests for the reflected ariaLabelledByElements property (precedence over aria-labelledby and aria-label) in arialabelledbyText and labelVirtual. getResolvedRefs already honors the property; this regression- tests the #4943 scenario. Tests adapted from #5187. Co-authored-by: JC Franco <[email protected]>
Summary
This PR updates label resolution to include
ariaLabelledByElements.Disclaimer: I used Copilot to help drive this PR. The code looks good from what I can tell and tests are passing locally, but this is my first time contributing, so I could be missing something. Happy to make any requested changes!
Closes: #4943