Bug 257739 – AX: AXObjectCache::setIsolatedTreeFocusedObject should handle the same special cases as focusedObjectForPage.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

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

[Radar WebKit Bug Importer]

<rdar://problem/110292074>

[Andres Gonzalez]

Created attachment 466599 [details]
Patch

[Tyler Wilcock]

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?

[Andres Gonzalez]

Created attachment 466604 [details]
Patch

[chris fleizach]

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?

[Andres Gonzalez]

(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.

[Tyler Wilcock]

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'");

[Andres Gonzalez]

Created attachment 466609 [details]
Patch

[Tyler Wilcock]

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.

[Andres Gonzalez]

Created attachment 466615 [details]
Patch

Reverting change to form-control-value-settable.html because it doesn't help with GTK failure.

[EWS]

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].