Bug 268035 – AX: Serve initial AXRelativeFrame for elements without cached frames off of the main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

We should be serving a rough relative frame from the secondary thread until the frame is cached during painting.

[Radar WebKit Bug Importer]

<rdar://problem/121554364>

[Joshua Hoffman]

rdar://120764228

[Joshua Hoffman]

Created attachment 469535 [details]
Patch

[Joshua Hoffman]

Created attachment 469536 [details]
Patch

[Joshua Hoffman]

Created attachment 469539 [details]
Patch

[Joshua Hoffman]

Created attachment 469540 [details]
Patch

[Joshua Hoffman]

Created attachment 469548 [details]
Patch

[Joshua Hoffman]

Created attachment 469764 [details]
Patch

[Joshua Hoffman]

Created attachment 469765 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469765 [details]
Patch

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

To reduce memory consumption, when a frame is painted via `AXIsolatedTree::updateFrame`, can we also remove AXPropertyName::InitialFrameRect since it presumably wouldn't be needed anymore?

propertyMap.set(AXPropertyName::RelativeFrame, WTFMove(newFrame));
// Now that we have a cached relative frame, the initial frame rect rough estimate frame is no longer necessary.
propertyMap.set(AXPropertyName::InitialFrameRect, FloatRect());

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:874
> +bool AccessibilityRenderObject::isSVGRoot() const
> +{
> +    RenderObject* renderObject = renderer();
> +
> +    if (!renderObject)
> +        return false;
> +
> +    return renderObject->isRenderOrLegacyRenderSVGRoot();
> +}

This can be written more simply as:

auto* renderer = this->renderer();
return renderer ? renderer->isRenderOrLegacyRenderSVGRoot() : false;

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2736
> +    auto* box = dynamicDowncast<RenderBox>(renderer());
> +    if (!box)
> +        return { };
> +
> +    return convertFrameToSpace(box->frameRect(), AccessibilityConversionSpace::Page);

Can use the same trick as above:

auto* box = dynamicDowncast<RenderBox>(renderer());
return box ? convertFrameToSpace(box->frameRect(), AccessibilityConversionSpace::Page) : FloatRect();

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2739
> +

Extra newline here I think

> Source/WebCore/accessibility/AccessibilityRenderObject.h:61
> +    FloatRect frameRect() const override;
> +    bool isSVGRoot() const override;

Can these be final?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1248
> +    if (!AXObjectCache::shouldServeInitialCachedFrame() || isMockObject() || isSVGRoot()) {

What about other SVG objects that aren't the SVG root? I don't think we gather frames for those during paint either unless you've found otherwise? Could we add a comment explaining why these ones hit the main-thread?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1259
> +    auto* ancestor = Accessibility::findAncestor<AXIsolatedObject>(*this, false, [] (auto& object) {

Can this be `const auto& object`?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1263
> +    auto frameRect = rectAttributeValue<FloatRect>(AXPropertyName::InitialFrameRect);
> +    if (ancestor && frameRect.location() == FloatPoint())

If we are only going to use the result of the ancestor traversal if frameRect.location() == FloatPoint(), we also shouldn't spend time doing it if frameRect.location() == FloatPoint().

How often do these frame rects have a 0,0 location vs. not? Why do we decide to "trust" the frame rects location if it happens to not be 0,0? Did this give better results?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:82
> +    bool getsGeometryFromChildren() const { return m_getsGeometryFromChildren; }

Is this method used?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:83
> +    bool isSVGRoot() const { return boolAttributeValue(AXPropertyName::IsSVGRoot); }

Can this be private instead of public?

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:221
> +    bool isRelativeFrameCachedByPaint() const { return optionalAttributeValue<IntRect>(AXPropertyName::RelativeFrame).has_value(); }

Maybe hasCachedRelativeFrame?

[Joshua Hoffman]

Comment on attachment 469765 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.h:61
>> +    bool isSVGRoot() const override;
> 
> Can these be final?

Yes, will update that.

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1248
>> +    if (!AXObjectCache::shouldServeInitialCachedFrame() || isMockObject() || isSVGRoot()) {
> 
> What about other SVG objects that aren't the SVG root? I don't think we gather frames for those during paint either unless you've found otherwise? Could we add a comment explaining why these ones hit the main-thread?

That is a good point. I will make isSVGRoot more encompassing then (isSVGObject()). 

And yeah we can include a comment, probably even a good place for a FIXME referencing.

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:1263
>> +    if (ancestor && frameRect.location() == FloatPoint())
> 
> If we are only going to use the result of the ancestor traversal if frameRect.location() == FloatPoint(), we also shouldn't spend time doing it if frameRect.location() == FloatPoint().
> 
> How often do these frame rects have a 0,0 location vs. not? Why do we decide to "trust" the frame rects location if it happens to not be 0,0? Did this give better results?

Frame rects will always be 0,0 before they are painted. So if we are at this point with a 0,0 frame, that can mean two things: (1) the frame is truly at 0,0, but hasn't been painted yet (this is okay because the "offset" from it's ancestor will just be 0) or (2) the frame is unpainted, as all FrameRects are initialized at 0,0.

We "trust" them using the assumption that 0,0 means one of the above. Does that make sense?

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:82
>> +    bool getsGeometryFromChildren() const { return m_getsGeometryFromChildren; }
> 
> Is this method used?

Ah this was leftover from an older implementation I had—I will remove it.

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:83
>> +    bool isSVGRoot() const { return boolAttributeValue(AXPropertyName::IsSVGRoot); }
> 
> Can this be private instead of public?

Yes

>> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:221
>> +    bool isRelativeFrameCachedByPaint() const { return optionalAttributeValue<IntRect>(AXPropertyName::RelativeFrame).has_value(); }
> 
> Maybe hasCachedRelativeFrame?

I like that—I will change it.

[Joshua Hoffman]

Created attachment 469796 [details]
Patch

[EWS]

Committed 274472@main (e6c6fde68060): <https://commits.webkit.org/274472@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469796 [details].