fix(aria): prevent getOwnedVirtual from returning duplicate nodes#4987
fix(aria): prevent getOwnedVirtual from returning duplicate nodes#4987WilcoFiers merged 3 commits intodequelabs:developfrom
Conversation
When a child element is also referenced via aria-owns, getOwnedVirtual returned the same node twice. Filter aria-owns references that already exist in children using Set. Closes: dequelabs#4840
|
|
WilcoFiers
left a comment
There was a problem hiding this comment.
@camiha Thank you for this pull request. Really appreciate it. The difficulty with this, as is the case for most axe-core issues is that there are subtle edge cases to consider. Do you have access to screen readers to be able to test for the cases I mentioned? If not I can see if we can do the testing.
| const childNodeSet = new Set(children.map(child => child.actualNode)); | ||
| const uniqueOwns = owns.filter(own => !childNodeSet.has(own.actualNode)); | ||
|
|
||
| return [...children, ...uniqueOwns]; |
There was a problem hiding this comment.
I'm not sure the order is correct this way. This needs to be tested, but I'd wager that if you have child A and B, and you then use aria-owns on A, it'll change the owned elements order to B, A.
There was a problem hiding this comment.
I've verified with Safari + VoiceOver and updated accordingly. I also added a test case for this behavior (with aria-owns="c a" and children a, b, c, the order becomes b → c → a).
| const childNodeSet = new Set(children.map(child => child.actualNode)); | ||
| const uniqueOwns = owns.filter(own => !childNodeSet.has(own.actualNode)); |
There was a problem hiding this comment.
We don't need actualNode or Set here. This can be done with children.includes(owned)
There was a problem hiding this comment.
Simplified to use includes() directly.
| if (virtualNode.hasAttr('aria-owns')) { | ||
| const owns = idrefs(actualNode, 'aria-owns') | ||
| .filter(element => !!element) | ||
| .map(element => axe.utils.getNodeFromTree(element)); |
There was a problem hiding this comment.
I don't think this fully solves the problem of ensuring this function doesn't return the same node twice. You've solved it for returning a child that's also aria-owns, but not for the same node getting picked by aria-owns twice. I think to solve that you need a different solution.
What I don't know is if I do aria-owns="A B A" whether the order I get in the accessibility tree is "A B" or "B A". This should be tested before implementing this, and we'll probably want to leave a comment about it in the code to have a record of that.
There was a problem hiding this comment.
Updated to deduplicate by first occurrence and added a comment documenting this behavior.
- Deduplicate aria-owns references by first occurrence - Move owned children to end to match browser accessibility tree behavior
|
@WilcoFiers I tested the edge cases you mentioned with Safari (26.2) + VoiceOver.
I've updated the implementation accordingly. I don't have access to JAWS or NVDA testing environments, so if the behavior differs, please let me know and I'll make adjustments. |
WilcoFiers
left a comment
There was a problem hiding this comment.
Excellent work. Thank you @camiha!
For the books; reviewed for security.
) When a child element is also referenced via aria-owns, getOwnedVirtual returned the same node twice. Filter aria-owns references that already exist in children using Set. Closes: #4840 --------- Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
) When a child element is also referenced via aria-owns, getOwnedVirtual returned the same node twice. Filter aria-owns references that already exist in children using Set. Closes: #4840 --------- Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
### [4.11.2](v4.11.1...v4.11.2) (2026-03-30) ### Bug Fixes - **aria-valid-attr-value:** handle multiple aria-errormessage IDs ([#4973](#4973)) ([9322148](9322148)) - **aria:** prevent getOwnedVirtual from returning duplicate nodes ([#4987](#4987)) ([99d1e77](99d1e77)), closes [#4840](#4840) - **DqElement:** avoid calling constructors with cloneNode ([#5013](#5013)) ([88bc57f](88bc57f)) - **existing-rule:** aria-busy now shows an error message for a use with unallowed children ([#5017](#5017)) ([dded75a](dded75a)) - **scrollable-region-focusable:** clarify the issue is in safari ([#4995](#4995)) ([2567afd](2567afd)), closes [WebKit#190870](https://github.com/dequelabs/WebKit/issues/190870) [WebKit#277290](https://github.com/dequelabs/WebKit/issues/277290) - **scrollable-region-focusable:** do not fail scroll areas when all content is visible without scrolling ([#4993](#4993)) ([240f8b5](240f8b5)) - **target-size:** determine offset using clientRects if target is display:inline ([#5012](#5012)) ([69d81c1](69d81c1)) - **target-size:** ignore widgets that are inline with other inline elements ([#5000](#5000)) ([cf8a3c0](cf8a3c0))
When a child element is also referenced via aria-owns, getOwnedVirtual returned the same node twice. Filter aria-owns references that already exist in children using Set.
Closes: #4840