Focus tracking support in the accessibility isolatedtree.
Created attachment 384195 [details] Patch
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
Created attachment 384240 [details] Patch
(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.
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
Created attachment 384648 [details] Patch
Created attachment 384661 [details] Patch
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
(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.
Created attachment 384751 [details] Patch
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?
(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.
Comment on attachment 384751 [details] Patch Clearing flags on attachment: 384751 Committed r253088: <https://trac.webkit.org/changeset/253088>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57612538>