Bug 256896

<rdar://problem/109462537>

Created attachment 466386 [details]
Patch

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?

Created attachment 466422 [details]
Patch

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

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