Bug 119719 – Suppress zero-length text SVG inline renderers

• Component: SVG
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/e4c158a33d34264796abfa683d0413568dfdcde0

The SVG text layout functions assert non-zero metrics for SVG inline text (see
SVGTextLayoutEngine::layoutTextOnLineOrPath, for example). Normally, we don't
instantiate renderers for zero-length inline text (see
Text::textRendererIsNeeded), but the RenderSVGInlineText constructor performs
whitespace filtering to support xml:space semantics
(http://www.w3.org/TR/SVG/struct.html#LangSpaceAttrs) and can end up with empty
text while still passing the non-zero length test in Text::textRendererIsNeeded
(for example a single newline gets deleted when xml:space="default").

Enter RenderText::positionLineBox() which attempts to fix things up by removing
zero length inline boxes. But removing boxes at this point is bad for business
because it invalidates (marks as dirty and clears computed metrics) parent
boxes and sibling boxes during layout, and throws subsequent computations off.

The patch adds isChildAllowed()-based validation for SVG inline text. This prevents zero-len-text
renderers from being inserted into the tree in the first place.

Most test result diffs are zero-lenght DRT RenderSVGInlineText variance, but
some reveal prior issues:

* svg/W3C-SVG-1.1/text-align-08-b-diffs.html has incorrect results checked in
  (one text group is missing due to this bug).
* svg/dom/altGlyph-dom.xhtml(altGlyph-dom.js) also has incorrect results
  hardcoded in.
* svg/animations/length-list-animation.svg is failing miserably without this
  patch, but the failure is only visible when testing manually because the
  reference SVG is failing in the same manner.

Also removing a redundant test in SVGTextLayoutEngine::currentVisualCharacterMetrics(): the while loop condition handles that case in an equivalent manner.

[Dirk Schulze]

IIRC xml:space won't make it into SVG2 in favor of space handling in HTML. Just as a note to this bug.

[Ahmad Saleem]

Took test from Blink commit: https://jsfiddle.net/365807gh/show

STP173 output is similar to Chrome Canary 117.

Although - we can get rid of this:

Also removing a redundant test in SVGTextLayoutEngine::currentVisualCharacterMetrics(): the while loop condition handles that case in an equivalent manner.

^ It can clean-up some code.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #2)
> Took test from Blink commit: https://jsfiddle.net/365807gh/show
> 
> STP173 output is similar to Chrome Canary 117.
> 
> Although - we can get rid of this:
> 
> Also removing a redundant test in
> SVGTextLayoutEngine::currentVisualCharacterMetrics(): the while loop
> condition handles that case in an equivalent manner.
> 
> ^ It can clean-up some code.

Looking at 'https://searchfox.org/wubkat/source/LayoutTests/svg/custom/invalid-text-content-expected.txt#18'

It seems we still don't suppress it.

RenderSVGInlineText {#text} at (0,0) size 0x0

[Ahmad Saleem]

I tried to merge this in the past: https://github.com/WebKit/WebKit/pull/4609

^ Honestly, my C++ skills were horrible at that time.

____________

Following compiles:

>> Source/WebCore/rendering/RenderText.h (on Line 89):

bool hasEmptyText() const { return m_text.isEmpty(); }

>> Source/WebCore/rendering/svg/RenderSVGInline.cpp (after namespace)

bool RenderSVGInline::isChildAllowed(const RenderObject& child, const RenderStyle& style) const
{
    if (SVGRenderSupport::isEmptySVGInlineText(&child))
        return false;
    
    return RenderElement::isChildAllowed(child, style);
}

>> Source/WebCore/rendering/svg/RenderSVGInline.h (on Line 36):

bool isChildAllowed(const RenderObject&, const RenderStyle&) const override;

>> Source/WebCore/rendering/svg/RenderSVGText.cpp (Line 79):

return child.isInline() && !SVGRenderSupport::isEmptySVGInlineText(&child);;

>> Source/WebCore/rendering/svg/SVGRenderSupport.cpp (Adding header and function at the end):

bool SVGRenderSupport::isEmptySVGInlineText(const RenderObject* object)
{
    // RenderSVGInlineText performs whitespace filtering in order to support xml:space
    // (http://www.w3.org/TR/SVG/struct.html#LangSpaceAttrs), and can end up with an empty string
    // even when its original constructor argument is non-empty.
    return object->isSVGInlineText() && downcast<RenderSVGInlineText>(object)->hasEmptyText();
}

>> Source/WebCore/rendering/svg/SVGRenderSupport.h:

static bool isEmptySVGInlineText(const RenderObject*);

>> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp (in function: SVGTextLayoutEngine::currentVisualCharacterMetrics):

Delete Line 363 & 364

__________

I tested 'svg/custom/invalid-text-content-expected.svg' post above and it does remove following line:

RenderSVGInlineText {#text} at (0,0) size 0x0

[Ahmad Saleem]

Please refer to following PR attempt: SVG is going through lot of moving parts, so it would be good to fix it later: https://github.com/WebKit/WebKit/pull/18358

[Radar WebKit Bug Importer]

<rdar://problem/134851694>

[Ahmad Saleem]

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

[EWS]

Committed 282894@main (bce920e27ee7): <https://commits.webkit.org/282894@main>

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