WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
204535
Focus tracking support in the accessibility isolatedtree.
https://bugs.webkit.org/show_bug.cgi?id=204535
Summary
Focus tracking support in the accessibility isolatedtree.
Andres Gonzalez
Reported
2019-11-22 13:58:57 PST
Focus tracking support in the accessibility isolatedtree.
Attachments
Patch
(14.55 KB, patch)
2019-11-22 14:13 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(14.60 KB, patch)
2019-11-23 10:06 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(14.67 KB, patch)
2019-12-02 12:24 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(14.94 KB, patch)
2019-12-02 14:45 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(14.93 KB, patch)
2019-12-03 13:40 PST
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Andres Gonzalez
Comment 1
2019-11-22 14:13:54 PST
Created
attachment 384195
[details]
Patch
chris fleizach
Comment 2
2019-11-23 00:17:04 PST
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
Comment 3
2019-11-23 10:06:30 PST
Created
attachment 384240
[details]
Patch
Andres Gonzalez
Comment 4
2019-11-23 10:14:23 PST
(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 5
2019-11-24 23:26:23 PST
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
Comment 6
2019-12-02 12:24:00 PST
Created
attachment 384648
[details]
Patch
Andres Gonzalez
Comment 7
2019-12-02 14:45:39 PST
Created
attachment 384661
[details]
Patch
chris fleizach
Comment 8
2019-12-03 10:56:21 PST
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
Comment 9
2019-12-03 13:25:21 PST
(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
Comment 10
2019-12-03 13:40:20 PST
Created
attachment 384751
[details]
Patch
chris fleizach
Comment 11
2019-12-03 14:36:31 PST
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
Comment 12
2019-12-03 18:46:48 PST
(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 13
2019-12-03 19:45:01 PST
Comment on
attachment 384751
[details]
Patch Clearing flags on attachment: 384751 Committed
r253088
: <
https://trac.webkit.org/changeset/253088
>
WebKit Commit Bot
Comment 14
2019-12-03 19:45:03 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15
2019-12-03 19:46:19 PST
<
rdar://problem/57612538
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug