Skip to content
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

LibWeb: Compute accessible names for hidden/hidden-but-referenced nodes #2294

Conversation

sideshowbarker
Copy link
Contributor

@sideshowbarker sideshowbarker commented Nov 12, 2024

This change implements full support for the “A. Hidden Not Referenced” step at https://w3c.github.io/accname/#step2A in the “Accessible Name and Description Computation” spec — including handling all hidden nodes that must be ignored, as well as handling hidden nodes that, for the purposes of accessible-name computation, must not be ignored (due to having aria-labelledby/aria-describedby references from other nodes).

Otherwise, without this change, not all cases of hidden nodes get ignored as expected, while cases of nodes that are hidden but that have aria-labelledby/aria-describedby references from other nodes get unexpectedly ignored.

For the tests at https://wpt.fyi/results/accname/name?product=ladybird, this change gets us passing:

  • all 27 subtests in the comp_labelledby_hidden_nodes.html test
  • all 5 subtests in the comp_hidden_not_referenced.html test

Otherwise, without this change, we fail all those tests.

Additionally, a separate commit here makes us give the value of the aria-label attribute the correct precedence for accessible-name computation required by the “Accessible Name and Description Computation” and HTML-AAM specs and by the corresponding WPT tests.

That gets us passing all 131 subtests in the comp_label.html test at https://wpt.fyi/results/accname/name?product=ladybird

@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch from 610f287 to 6cac6d8 Compare November 16, 2024 00:59
@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch 2 times, most recently from 862716e to 4c06624 Compare November 18, 2024 05:06
@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch 2 times, most recently from cf7b9dd to 3e68e9d Compare November 21, 2024 16:10
@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch 7 times, most recently from 29a6a58 to 3e92cde Compare November 26, 2024 11:33
Libraries/LibWeb/DOM/Element.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/DOM/Element.cpp Show resolved Hide resolved
Libraries/LibWeb/DOM/Node.cpp Outdated Show resolved Hide resolved
@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch from 3e92cde to 20fe6ba Compare November 27, 2024 09:20
@sideshowbarker sideshowbarker requested a review from gmta November 27, 2024 09:21
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you opened spec issues about the problems you've found? There's a reasonable chance that we're the only project implementing it literally so the problems might not be known about.

{
bool is_referenced = false;
if (id().has_value()) {
this->root().for_each_in_inclusive_subtree_of_type<HTML::HTMLElement>([&](auto& element) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is important, but this loop will also iterate over this element. Should an element referencing itself behave the same as a different element referencing it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also nit-pick: I don't think you need the this-> here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is important, but this loop will also iterate over this element. Should an element referencing itself behave the same as a different element referencing it?

I don’t now recall intentionally choosing for_each_in_inclusive_subtree_of_type rather than for_each_in_subtree_of_type and I don’t believe the spec requirements necessitate that, and changing it doesn’t cause any tests to fail — so I’ve gone ahead and changed it to using for_each_in_subtree_of_type.

Libraries/LibWeb/DOM/Element.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/DOM/Node.cpp Show resolved Hide resolved
@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch from 20fe6ba to 7cc65bc Compare November 27, 2024 12:23
@sideshowbarker
Copy link
Contributor Author

Have you opened spec issues about the problems you've found? There's a reasonable chance that we're the only project implementing it literally so the problems might not be known about.

Yes, I’ve opened both a spec issue and several spec PRs.

@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch from 7cc65bc to e07be89 Compare November 27, 2024 13:35
@sideshowbarker
Copy link
Contributor Author

sideshowbarker commented Nov 27, 2024

Have you opened spec issues about the problems you've found? There's a reasonable chance that we're the only project implementing it literally so the problems might not be known about.

Yes, I’ve opened both a spec issue and several spec PRs.

Actually, it seems I’d missed a few — but I’ve now added links to the additional ones in the code comments.

This change implements full support for the “A. Hidden Not Referenced”
step at https://w3c.github.io/accname/#step2A in the “Accessible Name
and Description Computation” spec — including handling all hidden nodes
that must be ignored, as well as handling hidden nodes that, for the
purposes of accessible-name computation, must not be ignored (due to
having aria-labelledby/aria-describedby references from other nodes).

Otherwise, without this change, not all cases of hidden nodes get
ignored as expected, while cases of nodes that are hidden but that have
aria-labelledby/aria-describedby references from other nodes get
unexpectedly ignored.
This change makes Ladybird give the value of the aria-label attribute
the correct precedence for accessible-name computation required by the
“Accessible Name and Description Computation” and HTML-AAM specs and by
the corresponding WPT tests.

Otherwise, without this change, Ladybird fails some of the WPT subtests
of the test at https://wpt.fyi/results/accname/name/comp_label.html.
@sideshowbarker sideshowbarker force-pushed the accname-name-comp_hidden_not_referenced branch from e07be89 to b775866 Compare November 27, 2024 15:08
@sideshowbarker
Copy link
Contributor Author

I think I’ve responded to (and hopefully resolved) all the review comments received on this so far. But if I missed anything, please let me know

@AtkinsSJ AtkinsSJ merged commit 6bfc35b into LadybirdBrowser:master Nov 29, 2024
5 of 6 checks passed
@sideshowbarker sideshowbarker deleted the accname-name-comp_hidden_not_referenced branch November 30, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants