Bug 257739 - AX: AXObjectCache::setIsolatedTreeFocusedObject should handle the same special cases as focusedObjectForPage.
Summary: AX: AXObjectCache::setIsolatedTreeFocusedObject should handle the same specia...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-06-05 20:05 PDT by Andres Gonzalez
Modified: 2023-06-07 12:25 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2023-06-05 20:16 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (9.27 KB, patch)
2023-06-06 13:19 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (9.85 KB, patch)
2023-06-06 19:07 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (8.96 KB, patch)
2023-06-07 06:02 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2023-06-05 20:05:27 PDT
This is necessary to retrieve the focused object from the Isolated tree without hitting the main thread: https://bugs.webkit.org/show_bug.cgi?id=256761
Comment 1 Radar WebKit Bug Importer 2023-06-05 20:05:41 PDT
<rdar://problem/110292074>
Comment 2 Andres Gonzalez 2023-06-05 20:16:06 PDT
Created attachment 466599 [details]
Patch
Comment 3 Tyler Wilcock 2023-06-05 21:10:07 PDT
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?
Comment 4 Andres Gonzalez 2023-06-06 13:19:00 PDT
Created attachment 466604 [details]
Patch
Comment 5 chris fleizach 2023-06-06 13:21:41 PDT
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?
Comment 6 Andres Gonzalez 2023-06-06 13:28:24 PDT
(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 7 Tyler Wilcock 2023-06-06 14:26:04 PDT
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'");
Comment 8 Andres Gonzalez 2023-06-06 19:07:41 PDT
Created attachment 466609 [details]
Patch
Comment 9 Tyler Wilcock 2023-06-06 23:08:18 PDT
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.
Comment 10 Andres Gonzalez 2023-06-07 06:02:33 PDT
Created attachment 466615 [details]
Patch

Reverting change to form-control-value-settable.html because it doesn't help with GTK failure.
Comment 11 EWS 2023-06-07 12:25:25 PDT
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].