Bug 204535 – Focus tracking support in the accessibility isolatedtree.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andres Gonzalez]

Focus tracking support in the accessibility isolatedtree.

[Andres Gonzalez]

Created attachment 384195 [details]
Patch

[chris fleizach]

Comment on attachment 384195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384195&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:362
> +    AXCoreObject* focus = axObjectCache->getOrCreate(focusedElement ? static_cast<Node*>(focusedElement) : static_cast<Node*>(&document));

is it necessary to cast as a Node. focusedElement is already a Element

> Source/WebCore/accessibility/AXObjectCache.cpp:367
> +        if (AXCoreObject* descendant = focus->activeDescendant())

auto* descendant

> Source/WebCore/accessibility/AXObjectCache.cpp:399
> +    ASSERT(false);

ASSERT_NOT_REACHED()

> Source/WebCore/accessibility/AXObjectCache.cpp:411
> +        tree->setFocusedNodeID(getOrCreate(focusedNode)->objectID());

I think it's possible this might return nullptr > getOrCreate(focusedNode)

> Source/WebCore/accessibility/AXObjectCache.cpp:417
> +    if (!gAccessibilityEnabled)

will this method be accessed on the AX thread? if so I suspect calling this chain will fail

page->focusController().focusedOrMainFrame().document()

> Source/WebCore/accessibility/AXObjectCache.cpp:3098
> +    AccessibilityObject* axRoot = axObjectCache->getOrCreate(document.view());

auto* axRoot

> Source/WebCore/accessibility/AXObjectCache.cpp:3103
> +    AXCoreObject* axFocus = axObjectCache->focusedObject(document);

auto* axFocus

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:272
> +    WebCore::Document* focusedDocument = page->focusController().focusedOrMainFrame().document();

auto* focusedDocument

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:282
> +    WebCore::AXCoreObject* focusedObject = axObjectCache->focusedUIElementForPage(page);

auto

[Andres Gonzalez]

Created attachment 384240 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Comment on attachment 384195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384195&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:362
> > +    AXCoreObject* focus = axObjectCache->getOrCreate(focusedElement ? static_cast<Node*>(focusedElement) : static_cast<Node*>(&document));
> 
> is it necessary to cast as a Node. focusedElement is already a Element

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:367
> > +        if (AXCoreObject* descendant = focus->activeDescendant())
> 
> auto* descendant

Done.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:399
> > +    ASSERT(false);
> 
> ASSERT_NOT_REACHED()

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:411
> > +        tree->setFocusedNodeID(getOrCreate(focusedNode)->objectID());
> 
> I think it's possible this might return nullptr > getOrCreate(focusedNode)

Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:417
> > +    if (!gAccessibilityEnabled)
> 
> will this method be accessed on the AX thread? if so I suspect calling this
> chain will fail
> 
> page->focusController().focusedOrMainFrame().document()

Have to revisit this. Have to see where to get the Page or the Document object in a way that can be run on either thread.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:3098
> > +    AccessibilityObject* axRoot = axObjectCache->getOrCreate(document.view());
> 
> auto* axRoot

Done.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:3103
> > +    AXCoreObject* axFocus = axObjectCache->focusedObject(document);
> 
> auto* axFocus

Done.
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:272
> > +    WebCore::Document* focusedDocument = page->focusController().focusedOrMainFrame().document();
> 
> auto* focusedDocument

Done.
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:282
> > +    WebCore::AXCoreObject* focusedObject = axObjectCache->focusedUIElementForPage(page);
> 
> auto

Done.

[chris fleizach]

Comment on attachment 384240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384240&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:429
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)

why do we expect the isolate tree calls to come into here? I tried to look at the code to see if I could check how we'd come in to here but not sure. I wouldn't expect that in isolated tree mode we call into this. I would expect that we get the isolated tree for the page, then we get the focused element.

I think we should assert here that we're on the main thread to make sure we don't come through this path

[Andres Gonzalez]

Created attachment 384648 [details]
Patch

[Andres Gonzalez]

Created attachment 384661 [details]
Patch

[chris fleizach]

Comment on attachment 384661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384661&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:385
> +    auto tree = AXIsolatedTree::treeForPageID(*pageID);

if there's no isolated tree do we even need to make when asking for focused object?

> Source/WebCore/accessibility/AXObjectCache.cpp:390
> +        _AXUIElementUseSecondaryAXThread(true);

why do we want to move this to the secondary thread in WebCore again? it seems like a level of detail that WK2 would handle instead of WebCore

> Source/WebCore/accessibility/AXObjectCache.cpp:405
> +    if (!focus)

if there's no focusedNode (or its nil) then we should unset focused from the tree

> Source/WebCore/accessibility/AXObjectCache.cpp:413
> +        tree->setFocusedNodeID(focus->objectID());

inside this method
setFocusedNodeID()

are we putting the focusedNodeID behind the lock and then pulling it out when we append changes?

> Source/WebCore/accessibility/AXObjectCache.cpp:429
> +    if (clientSupportsIsolatedTree())

how are we coming into this code here? I don't think we would ever want to come in here on the secondary thread

> Source/WebCore/accessibility/AXObjectCache.cpp:3077
> +    axObjectCache->attachWrapper(&isolatedTreeNode.get());

we're going to have to revisit this. I don't think this plan will work if we have one wrapper for AccessibilityObject and a different wrapper for isolated tree node

[Andres Gonzalez]

(In reply to chris fleizach from comment #8)
> Comment on attachment 384661 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384661&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:385
> > +    auto tree = AXIsolatedTree::treeForPageID(*pageID);
> 
> if there's no isolated tree do we even need to make when asking for focused
> object?

That is my assumption since there is no rule on which client request is coming through first. There are at least three client requests that can come in in any order which require the generation of the isolated tree:
1. root element for a given page.
2. Focused element for a given page/document.
3. Element at a given point.
Once a client holds a reference to an object returned by any of these three calls, the client is free to walk the whole hierarchy of accessible objects and query for any property of any object. So the isolated tree should be ready to handle those requests.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:390
> > +        _AXUIElementUseSecondaryAXThread(true);
> 
> why do we want to move this to the secondary thread in WebCore again? it
> seems like a level of detail that WK2 would handle instead of WebCore

My understanding is that this would signal the accessibility run-time to start making all accessibility property requests on the AX secondary thread. So, once the isolated tree is generated and ready to use, it is appropriate to set the system flag. If the flag was already set on by a previous call, this would be a no-op. Please clarify if that is not the case.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:405
> > +    if (!focus)
> 
> if there's no focusedNode (or its nil) then we should unset focused from the
> tree

Done.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:413
> > +        tree->setFocusedNodeID(focus->objectID());
> 
> inside this method
> setFocusedNodeID()
> 
> are we putting the focusedNodeID behind the lock and then pulling it out
> when we append changes?

Yes, to be precise, we are doing:

    LockHolder locker { m_changeLogLock };
    m_pendingFocusedNodeID = axID;

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:429
> > +    if (clientSupportsIsolatedTree())
> 
> how are we coming into this code here? I don't think we would ever want to
> come in here on the secondary thread

Right, this will happen in the main thread. If the client supports the isolated tree, then we return the isolatedObject with focus, so that any subsequent call into that object uses the isolated tree in the secondary thread.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:3077
> > +    axObjectCache->attachWrapper(&isolatedTreeNode.get());
> 
> we're going to have to revisit this. I don't think this plan will work if we
> have one wrapper for AccessibilityObject and a different wrapper for
> isolated tree node

Agree, that is the change I"m making now, to swap the underlying AXCoreObject in the same wrapper when needed.

[Andres Gonzalez]

Created attachment 384751 [details]
Patch

[chris fleizach]

Comment on attachment 384751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384751&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:387
> +        tree = generateIsolatedTree(*pageID, document);

so are we going to have to have this block of code 3 times for each of the entry points?

[Andres Gonzalez]

(In reply to chris fleizach from comment #11)
> Comment on attachment 384751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384751&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:387
> > +        tree = generateIsolatedTree(*pageID, document);
> 
> so are we going to have to have this block of code 3 times for each of the
> entry points?

Will wrap it all into generateIsolatedTree.

[WebKit Commit Bot]

Comment on attachment 384751 [details]
Patch

Clearing flags on attachment: 384751

Committed r253088: <https://trac.webkit.org/changeset/253088>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57612538>