Bug 239743 – [LBSE] Activate text rendering, by re-using RenderSVGText

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

We can easily bring in text support from legacy -> LBSE. No need to split up RenderSVGText in LegacyRenderSVGText / RenderSVGText (LBSE): the inheritance structure hasn't changed -- RenderSVGText always already inherited from RenderLayerModelObject in the legacy engine, it just force-disabled layer creation. Change that.

[Nikolas Zimmermann]

Created attachment 458312 [details]
Patch, v1 (depends on bug 239717)

[Nikolas Zimmermann]

Created attachment 458313 [details]
Patch, v1 (includes patch from bug 239717)

[Nikolas Zimmermann]

Want to get some EWS results overnight... let's hope I got it right ;-)

[Nikolas Zimmermann]

Created attachment 458315 [details]
Patch, v2 (includes patch from bug 239717)

[Nikolas Zimmermann]

Created attachment 458345 [details]
Patch, v3 (depends on bug 239717)

[Nikolas Zimmermann]

Created attachment 458346 [details]
Patch, v3 (includes patch from bug 239717)

[Nikolas Zimmermann]

Created attachment 458469 [details]
Patch, v4

[Radar WebKit Bug Importer]

<rdar://problem/92637427>

[Rob Buis]

Comment on attachment 458469 [details]
Patch, v4

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

> Source/WebCore/rendering/RenderObject.cpp:527
> +#endif

Why is this needed and why now?

> Source/WebCore/rendering/svg/RenderSVGBlock.cpp:51
> +#endif

Should this have been introduced in an earlier patch? What is the relation to RenderSVGText?

> Source/WebCore/rendering/svg/RenderSVGBlock.cpp:129
> +        if (style().visibility() != Visibility::Visible && !enclosingLayer()->hasVisibleContent())

Is the second condition not enough?

[Nikolas Zimmermann]

Comment on attachment 458469 [details]
Patch, v4

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

>> Source/WebCore/rendering/RenderObject.cpp:527
>> +#endif
> 
> Why is this needed and why now?

Now that <text> is enabled the code path is hit for the first time in LBSE. We'll assert without it in a few tests.
We currently disallow partial layouts, no SVG renderer can be a relayout boundary. A container position/size changes affects its parent, due to the way SVG containers are constructed.

>> Source/WebCore/rendering/svg/RenderSVGBlock.cpp:51
>> +#endif
> 
> Should this have been introduced in an earlier patch? What is the relation to RenderSVGText?

No, it's correct to introduce it here: Every transformable SVG renderer has to take care of updating these flags (in LBSE-only). Therefore renderers such as RenderSVGBlock/Text that become LBSE-aware, now need this logic.

>> Source/WebCore/rendering/svg/RenderSVGBlock.cpp:129
>> +        if (style().visibility() != Visibility::Visible && !enclosingLayer()->hasVisibleContent())
> 
> Is the second condition not enough?

No, it's not. The code here was originally based on RenderBox::clippedOverflowRect() ~ last rebased and compared in Oct 2021.

Today, RenderBox::clippedOverflowRect() looks different:

    LayoutRect RenderBox::clippedOverflowRect(const RenderLayerModelObject* repaintContainer, VisibleRectContext context) const
    {
        if (isInsideEntirelyHiddenLayer())
            return { };
         ...

To recap: RenderLayer queries the renderers clippedOverflowRectForRepaint() (== clippedOverflowRect(), with a predefined VisibleRectContext) to determine the repaint rect for the layer, in RenderLayer::computeRepaintRects().

The two checks "style().visibility() != Visibility::Visible" and "!enclosingLayer()->hasVisibleContent()" were encapsulated in a new 'isInsideEntirelyHiddenLayer()' helper recently introduced by Simon (https://bugs.webkit.org/show_bug.cgi?id=235242).

The style().visibility() check only considers the current renderer, whereas the enclosingLayer()->hasVisibleContent() in principle depends on the visibility status of all children of the enclosing layer.
An example to clarify this:
1) if 'visibility' is set to 'visible' the two checks are actually equivalent, RenderLayer::computeHasVisibleContent() returns true if 'visibility' is set to 'visible'.
2) If 'visibility' is set to 'hidden', we still might need to return a non-empty 'visual overflow rect', if the layers' renderer has children with 'visibility: visible' that themselves have no own layer.

I will adapt the code.

[Nikolas Zimmermann]

Created attachment 459413 [details]
Patch, v5

[Rob Buis]

Comment on attachment 459413 [details]
Patch, v5

Looks good, just the ChangeLog will have to be removed before landing.

[Nikolas Zimmermann]

Pull request: https://github.com/WebKit/WebKit/pull/687

[EWS]

Committed r294385 (250682@main): <https://commits.webkit.org/250682@main>

Reviewed commits have been landed. Closing PR #687 and removing active labels.