WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
256896
AX: Move [WebAccessibilityObjectWrapper windowElement:] off the main thread.
https://bugs.webkit.org/show_bug.cgi?id=256896
Summary
AX: Move [WebAccessibilityObjectWrapper windowElement:] off the main thread.
Andres Gonzalez
Reported
2023-05-17 09:12:58 PDT
Part of the ITM effort to improve responsiveness to AT.
Attachments
Patch
(4.12 KB, patch)
2023-05-17 09:21 PDT
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Patch
(4.12 KB, patch)
2023-05-19 09:06 PDT
,
Andres Gonzalez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2023-05-17 09:13:13 PDT
<
rdar://problem/109462537
>
Andres Gonzalez
Comment 2
2023-05-17 09:21:45 PDT
Created
attachment 466386
[details]
Patch
Tyler Wilcock
Comment 3
2023-05-17 11:03:13 PDT
Comment on
attachment 466386
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=466386&action=review
> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1217 > + // FIXME: Remove the following methods from the AXCoreObject intterface and instead use methods such as axScrollView() if needed.
Typo: intterface vs. interface
> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1488 > - return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = retainPtr(self)] () -> RetainPtr<id> { > - auto* backingObject = protectedSelf.get().axBackingObject; > - if (!backingObject) > - return nil; > - > - if (auto* fv = backingObject->documentFrameView()) > - return [fv->platformWidget() window]; > - > - return nil; > - }); > + RefPtr axScrollView = self.axBackingObject->axScrollView(); > + return axScrollView ? [axScrollView->platformWidget() window] : nil;
So the lines above this call self.remoteAccessibilityParentObject which is defined as: - (id)remoteAccessibilityParentObject { auto* backingObject = self.axBackingObject; return backingObject ? backingObject->remoteParentObject() : nil; } And in turn, AXIsolatedObjectMac::remoteParentObject is defined as: RemoteAXObjectRef AXIsolatedObject::remoteParentObject() const { auto* scrollView = Accessibility::findAncestor<AXCoreObject>(*this, true, [] (const AXCoreObject& object) { return object.isScrollView(); }); return is<AXIsolatedObject>(scrollView) ? downcast<AXIsolatedObject>(scrollView)->m_remoteParent.get() : nil; } So when we run your new self.axBackingObject->axScrollView(), we will have already iterated for and failed to find a scroll view. So your new codepath seems redundant from what I can tell. Do you read that the same way?
Andres Gonzalez
Comment 4
2023-05-19 09:06:02 PDT
Created
attachment 466422
[details]
Patch
Andres Gonzalez
Comment 5
2023-05-19 09:15:20 PDT
(In reply to Tyler Wilcock from
comment #3
)
> Comment on
attachment 466386
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=466386&action=review
> > > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1217 > > + // FIXME: Remove the following methods from the AXCoreObject intterface and instead use methods such as axScrollView() if needed. > > Typo: intterface vs. interface
Fixed.
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1488 > > - return Accessibility::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = retainPtr(self)] () -> RetainPtr<id> { > > - auto* backingObject = protectedSelf.get().axBackingObject; > > - if (!backingObject) > > - return nil; > > - > > - if (auto* fv = backingObject->documentFrameView()) > > - return [fv->platformWidget() window]; > > - > > - return nil; > > - }); > > + RefPtr axScrollView = self.axBackingObject->axScrollView(); > > + return axScrollView ? [axScrollView->platformWidget() window] : nil; > > So the lines above this call self.remoteAccessibilityParentObject which is > defined as: > > - (id)remoteAccessibilityParentObject > { > auto* backingObject = self.axBackingObject; > return backingObject ? backingObject->remoteParentObject() : nil; > } > > And in turn, AXIsolatedObjectMac::remoteParentObject is defined as: > > RemoteAXObjectRef AXIsolatedObject::remoteParentObject() const > { > auto* scrollView = Accessibility::findAncestor<AXCoreObject>(*this, > true, [] (const AXCoreObject& object) { > return object.isScrollView(); > }); > return is<AXIsolatedObject>(scrollView) ? > downcast<AXIsolatedObject>(scrollView)->m_remoteParent.get() : nil; > } > > So when we run your new self.axBackingObject->axScrollView(), we will have > already iterated for and failed to find a scroll view. So your new codepath > seems redundant from what I can tell. Do you read that the same way?
No, self.remoteAccessibilityParentObject returns false not because it doesn't find the ScrollView, but because m_remoteParent is null in return is<AXIsolatedObject>(scrollView) ? downcast<AXIsolatedObject>(scrollView)->m_remoteParent.get() : nil; and thus we were hitting the main thread. You can have a ScrollView that has no remote parent by still must have a platform widget.
EWS
Comment 6
2023-05-19 10:49:54 PDT
Committed
264266@main
(cb345f2eccf0): <
https://commits.webkit.org/264266@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 466422
[details]
.
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