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

Ignore <option> element while building accessibility tree when <select> is hidden #4630

Merged

Conversation

obyknovenius
Copy link
Contributor

@obyknovenius obyknovenius commented Sep 23, 2022

a9c99d1

Ignore <option> element while building accessibility tree when <select> is hidden
https://bugs.webkit.org/show_bug.cgi?id=245578

Reviewed by Carlos Garcia Campos.

When we insert `AccessibilityObject`'s child, we check that its
`parentObject` is the `AccessiblityObject` we're inserting it into.

In normal circumstances `AccessibilityObject`'s subclass representing
`HTMLSelectElement` is `AccessibilityMenuList`. It creates
`AccessiblityMenuListPopup` as its only accessibility child. All
`HTMLSelectElement`'s node children accessibility objects are inserted
into the popup.

For non-multiple `HTMLSelectElement` the only possible node child class
that participates in accessibility tree is `HTMLOptionElement` with its
accessibility counterpart `AccessibilityMenuListOption`.
`AccessibilityMenuListOption`'s `parentObject` is set explicitly to
`AccessiblityMenuListPopup` when it's being inserted into one.

Situation changes when `HTMLSelectElement` is hidden but `aria-hidden`
attribute set to `false`
(like in `accessibility/aria-visible-element-roles.html` layout test).
In this case, we don't have a renderer object associated with
`HTMLSelectElement` and simply create a regular `AccessiblityNodeObject`
for it. Then we insert `AccessibilityMenuListOption` as
`AccessiblityNodeObject`'s child without setting its `parentObject`.
Thus, `child->parentObject() == this` assertion fails.

Since we can't present a menu without `HTMLSelectElement`'s renderer
anyway (and this is what Chromium does), it's safe to skip
`HTMLSelectElement` while building the accessibility tree in such cases.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::getOrCreate):

Canonical link: https://commits.webkit.org/254970@main

624bac7

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe ❌ πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@carlosgcampos
Copy link
Contributor

Please, mention the commit message what this is fixing. I think it's an assert in a particular test, right? Does it happen in any platform? because the change is in cross-platform code.

@obyknovenius
Copy link
Contributor Author

Please, mention the commit message what this is fixing. I think it's an assert in a particular test, right? Does it happen in any platform? because the change is in cross-platform code.

I've updated the commit message. How do I update the PR description?

@obyknovenius obyknovenius requested a review from a team as a code owner September 26, 2022 16:42
@obyknovenius obyknovenius changed the title AX: Skip <option> element while building accessibility tree when <select> is hidden [ATSPI] Ignore <option> element while building accessibility tree when <select> is hidden Sep 26, 2022
@obyknovenius
Copy link
Contributor Author

I found out that there is a platform-specific hook for this purpose in AccessibilityObject. Made this PR AT-SPI specific and reused the solution from mac.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 26, 2022
@obyknovenius obyknovenius changed the title [ATSPI] Ignore <option> element while building accessibility tree when <select> is hidden Ignore <option> element while building accessibility tree when <select> is hidden Sep 27, 2022
Copy link
Contributor

@carlosgcampos carlosgcampos left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@aperezdc aperezdc added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Sep 28, 2022
…t> is hidden

https://bugs.webkit.org/show_bug.cgi?id=245578

Reviewed by Carlos Garcia Campos.

When we insert `AccessibilityObject`'s child, we check that its
`parentObject` is the `AccessiblityObject` we're inserting it into.

In normal circumstances `AccessibilityObject`'s subclass representing
`HTMLSelectElement` is `AccessibilityMenuList`. It creates
`AccessiblityMenuListPopup` as its only accessibility child. All
`HTMLSelectElement`'s node children accessibility objects are inserted
into the popup.

For non-multiple `HTMLSelectElement` the only possible node child class
that participates in accessibility tree is `HTMLOptionElement` with its
accessibility counterpart `AccessibilityMenuListOption`.
`AccessibilityMenuListOption`'s `parentObject` is set explicitly to
`AccessiblityMenuListPopup` when it's being inserted into one.

Situation changes when `HTMLSelectElement` is hidden but `aria-hidden`
attribute set to `false`
(like in `accessibility/aria-visible-element-roles.html` layout test).
In this case, we don't have a renderer object associated with
`HTMLSelectElement` and simply create a regular `AccessiblityNodeObject`
for it. Then we insert `AccessibilityMenuListOption` as
`AccessiblityNodeObject`'s child without setting its `parentObject`.
Thus, `child->parentObject() == this` assertion fails.

Since we can't present a menu without `HTMLSelectElement`'s renderer
anyway (and this is what Chromium does), it's safe to skip
`HTMLSelectElement` while building the accessibility tree in such cases.

* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::getOrCreate):

Canonical link: https://commits.webkit.org/254970@main
@webkit-commit-queue
Copy link
Collaborator

Committed 254970@main (a9c99d1): https://commits.webkit.org/254970@main

Reviewed commits have been landed. Closing PR #4630 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit a9c99d1 into WebKit:main Sep 28, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 28, 2022
@obyknovenius obyknovenius deleted the select-aria-hidden-false branch September 29, 2022 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants