| Summary: | AX: AXObjectCache::setIsolatedTreeFocusedObject should handle the same special cases as focusedObjectForPage. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||||||
| Component: | Accessibility | Assignee: | Andres Gonzalez <andresg_22> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Andres Gonzalez
2023-06-05 20:05:27 PDT
Created attachment 466599 [details]
Patch
Comment on attachment 466599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466599&action=review Nice! Two questions: 1. Do we need to re-compute setIsolatedTreeFocusedObject after aria-activedescendant changes? 2. Does this change make any new tests pass? If not, I bet we could craft one based on aria-activedescendant that would not have passed in ITM before your change. > Source/WebCore/accessibility/AXObjectCache.cpp:485 > + return focusedObjectForNode(static_cast<Node*>(document)); I know this static_cast existed before your patch, but is it necessary? Document subclasses ContainerNode, which in turn subclasses Node, so we shouldn't need to cast, right? Created attachment 466604 [details]
Patch
Comment on attachment 466604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466604&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:-498 > - if (focus->accessibilityIsIgnored()) Do we still need this? What if the focused element is ignored? (In reply to chris fleizach from comment #5) > Comment on attachment 466604 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=466604&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:-498 > > - if (focus->accessibilityIsIgnored()) > > Do we still need this? What if the focused element is ignored? No, we decided that if it is focused cannot be ignored. Comment on attachment 466604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466604&action=review Thanks for doing this! > LayoutTests/accessibility/active-descendant-changes-result-in-focus-changes.html:23 > + await waitFor(() => accessibilityController.focusedElement.domIdentifier == "listbox"); > + output += expect("accessibilityController.focusedElement.domIdentifier", "'listbox'"); Nit for future patches, not worth changing for this one. This can be written more succinctly using expectAsync. output += await expectAsync("accessibilityController.focusedElement.domIdentifier", "'listbox'"); Created attachment 466609 [details]
Patch
Comment on attachment 466609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=466609&action=review > Source/WebCore/accessibility/AXObjectCache.h:526 > + AccessibilityObject* focusedObjectForNode(Node*); Something I just remembered — I think we need to add a no-op stub for this in the #if !ENABLE(ACCESSIBILITY) part of this file. Otherwise we'll break the Playstation build. Created attachment 466615 [details]
Patch
Reverting change to form-control-value-settable.html because it doesn't help with GTK failure.
Committed 264952@main (b4bbe36cdab2): <https://commits.webkit.org/264952@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466615 [details]. |