Bug 217745

Summary: Fix for multiple accessibility layout tests in isolated tree mode.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: New BugsAssignee: Andres Gonzalez <andresg_22>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Andres Gonzalez 2020-10-14 19:05:06 PDT
Fix for multiple accessibility layout tests in isolated tree mode.
Comment 1 Andres Gonzalez 2020-10-14 19:16:24 PDT
Created attachment 411394 [details]
Patch
Comment 2 chris fleizach 2020-10-14 22:41:09 PDT
Comment on attachment 411394 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1142
> +    auto* axObject = associatedAXObject();

I think we would need to retrieve this on the main thread

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1971
> +    auto* backingObject = self.axBackingObject;

so are we no longer able to use a cached frame?
Comment 3 Andres Gonzalez 2020-10-15 04:11:03 PDT
(In reply to chris fleizach from comment #2)
> Comment on attachment 411394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411394&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1142
> > +    auto* axObject = associatedAXObject();
> 
> I think we would need to retrieve this on the main thread

The caller needs to dispatch this call the main thread because it returns a SimpleRange. So it doesn't make sense to dispatch here. in Debug mode this is enforced with the assert in associatedAXObject.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1971
> > +    auto* backingObject = self.axBackingObject;
> 
> so are we no longer able to use a cached frame?

At least not to compute the position. It seems that the only rect we are able to cache is the elementRect, which in my understanding is the intrinsic coordinates of the element in the layout. Coordinates relative to a view or to the screen need to be corrected by their corresponding offsets and that cannot be cached in the isolated object.
Comment 4 Andres Gonzalez 2020-10-15 04:16:18 PDT
Caching the layout coordinates and making the necessary transformation to other coordinate system on demand, will also eliminate the need of special casing the handling of the returned coordinates in Appkit when in isolated tree mode.
Comment 5 chris fleizach 2020-10-15 11:31:18 PDT
Comment on attachment 411394 [details]
Patch

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

>>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1142
>>> +    auto* axObject = associatedAXObject();
>> 
>> I think we would need to retrieve this on the main thread
> 
> The caller needs to dispatch this call the main thread because it returns a SimpleRange. So it doesn't make sense to dispatch here. in Debug mode this is enforced with the assert in associatedAXObject.

should we put an assert mainThread() here just in case? it wasn't clear to me this method would require main thread usage already
Comment 6 EWS 2020-10-15 11:43:34 PDT
Committed r268541: <https://trac.webkit.org/changeset/268541>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411394 [details].
Comment 7 Radar WebKit Bug Importer 2020-10-15 11:44:18 PDT
<rdar://problem/70345631>