Bug 40663 – Modernize SVG Text code, following the HTML design

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

The SVG Text code from 2007 was a prototype. We should fix it to match the HTML design. Painting is done only from SVGRootInlineBox, which keeps track of all characters, and draws them. Instead we should divide the work through SVGInlineFlowBox/SVGInlineTextBox.

The rewrite is done, and it's really large. It fixes numerous problems with SVG text selection, provides sub-pixel positioning support, integrated properly within the SVGRenderBase::computeRepaintRect() logic etc.
This is the master bug tracking the work.

[Nikolas Zimmermann]

*** Bug 15644 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 15386 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 12172 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 58987 [details]
Initial patch

Sorry for this uber-large patch, but I'm including all layout test changes, so EWS can run - at least for mac it should turn green.
All other platforms, with platform specific results, for any SVG test containing text will fail. I need to fix them after landing, as I do not have access to all platforms.

Uploading the pixel test changes in a seperated patch, as the full patch is 12mb.

[Nikolas Zimmermann]

Created attachment 58988 [details]
Pixel test changes

[WebKit Review Bot]

Attachment 58987 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGTextContentElement.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/SVGTextChunkLayoutInfo.h:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/InlineBox.h:146:  More than one command on the same line  [whitespace/newline] [4]
WebCore/rendering/RenderSVGText.cpp:206:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 726 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58988 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 58987 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3275296

[Nikolas Zimmermann]

Created attachment 58990 [details]
Updated patch

Fix build failure in release mode, fix style issues.

[Dirk Schulze]

Comment on attachment 58990 [details]
Updated patch

WebCore/rendering/RenderBlock.h:232
 +      // Only used by RenderSVGText, which explicitely overrides RenderBlock::layoutBlock(), do NOT use for anything else.
Can you surround it with #if ENABLE(SVG)?

WebCore/rendering/RenderBlockLineLayout.cpp:750
 +                          if (!isSVGRootInlineBox)
Can you use:
#if ENABLE(SVG)
    if (lineBox->isSVGRootInlineBox())
#endif
        computeHorizontalPositionsForLine(lineBox, firstLine, resolver.firstRun(), trailingSpaceRun, end.atEnd(), textBoxDataMap);

instead?

WebCore/rendering/RenderSVGInlineText.cpp: 
 +      // FIXME: That's not sufficient. The localCaretRect function is also used for selection.
Do or don't we need caret rects for selecting texts?

WebCore/rendering/RenderSVGText.cpp:185
 +      FloatRect boundingBox = objectBoundingBox();
Can you name it strokeRect or strokeBoundaries please? This would match the naming in other renderers.

WebCore/rendering/RenderSVGText.cpp:207
 +   
No need for this white space :-)

WebCore/rendering/SVGInlineFlowBox.cpp:44
 +      FloatRect boundingBox = boxRenderer->repaintRectInLocalCoordinates();
same. boundingBox is not accurate enough, use repaintRect here

WebCore/rendering/SVGInlineTextBox.cpp:310
 +          ASSERT_NOT_REACHED();
I think this the assertion should be removed. At the moment our default fallback is black. This will change and if fill or stroke are not valid, we shouldn't draw text/decorations/under-/over-/through-line at all. 
WebCore/rendering/SVGInlineTextBox.cpp:414
 +      ASSERT_NOT_REACHED();
IIRC there are also other decorations like blink. Maybe you shouldn't use the assert here.

I did not check the pixel results yet, but the DRT looking at the DRT results, the text moves more than 30px sometimes. You checked all DRT tests manually with such a huge change?

I would like to look a second time on the patch, once you replied to this review.

[Nikolas Zimmermann]

(In reply to comment #10)
> (From update of attachment 58990 [details])
> WebCore/rendering/RenderBlock.h:232
>  +      // Only used by RenderSVGText, which explicitely overrides RenderBlock::layoutBlock(), do NOT use for anything else.
> Can you surround it with #if ENABLE(SVG)?
> 
> WebCore/rendering/RenderBlockLineLayout.cpp:750
>  +                          if (!isSVGRootInlineBox)
> Can you use:
> #if ENABLE(SVG)
>     if (lineBox->isSVGRootInlineBox())
> #endif
>         computeHorizontalPositionsForLine(lineBox, firstLine, resolver.firstRun(), trailingSpaceRun, end.atEnd(), textBoxDataMap);
> 
> instead?

You must have misread this part, we always have to call computeHorizontalPositionsForLine, except for SVG. I'm also saving the isSVGRootInlineBox() virtual call result in a temporary variable, as it's used again, some lines below, to determine whether computePerCharacterLayoutInformation should be called, thus saving another virtual function call.

> 
> WebCore/rendering/RenderSVGInlineText.cpp: 
>  +      // FIXME: That's not sufficient. The localCaretRect function is also used for selection.
> Do or don't we need caret rects for selecting texts?
> 
> WebCore/rendering/RenderSVGText.cpp:185
>  +      FloatRect boundingBox = objectBoundingBox();
> Can you name it strokeRect or strokeBoundaries please? This would match the naming in other renderers.

Fixed, named it "strokeBoundaries".

> WebCore/rendering/RenderSVGText.cpp:207
>  +   
> No need for this white space :-)

Right, fixed.

> 
> WebCore/rendering/SVGInlineFlowBox.cpp:44
>  +      FloatRect boundingBox = boxRenderer->repaintRectInLocalCoordinates();
> same. boundingBox is not accurate enough, use repaintRect here

Fixed.
 
> WebCore/rendering/SVGInlineTextBox.cpp:310
>  +          ASSERT_NOT_REACHED();
> I think this the assertion should be removed. At the moment our default fallback is black. This will change and if fill or stroke are not valid, we shouldn't draw text/decorations/under-/over-/through-line at all. 

No, the assertion is valid all the time. It only assures that m_paintingResourceMode has been set to ApplyToFillMode _or_ ApplyToStrokeMode before calling acquirePaintingResource(). I'm not asserting that the acquired painting resource is not null. So it's fine :-)

> WebCore/rendering/SVGInlineTextBox.cpp:414
>  +      ASSERT_NOT_REACHED();
> IIRC there are also other decorations like blink. Maybe you shouldn't use the assert here.

positionOffsetForDecoration() is only called by paintDecorationWithStyle(), which is called by paintDecoration(), which is called by paint(), and _only_ for UNDERLINE, OVERLINE and STRIKE-THROUGH. I'm not handling any other decoration for SVG, per spec. So the assertion is valid.
 
> I did not check the pixel results yet, but the DRT looking at the DRT results, the text moves more than 30px sometimes. You checked all DRT tests manually with such a huge change?
Oh, this is because the postions are correct now! Previously the RenderSVGText had x/y positions, and the RenderSVGInlineText mostly had negative y offsets, which was a relict of the old layouting code - I've checked most results by hand, and they're just fine now. If you make up equivalent HTML testcases (using an absolute positioned dev, that contains text, you'll get the same style of results).

> 
> I would like to look a second time on the patch, once you replied to this review.
Sure! Thanks for that!

Building, running tests and uploading a new version.

[Nikolas Zimmermann]

Created attachment 59076 [details]
Updated patch v2

Just uploading a new patch, without test results.

[Dirk Schulze]

Comment on attachment 59076 [details]
Updated patch v2

WebCore/rendering/SVGRootInlineBox.cpp:53
 +      FloatRect boundingBox = boxRenderer->repaintRectInLocalCoordinates();
forgto to mention this boundingBox. -> repaintRect

Just fix this name and i'm fine with it. You moved the LayoutTest results out now?

[Dirk Schulze]

Comment on attachment 58988 [details]
Pixel test changes

r=me

[Nikolas Zimmermann]

Created attachment 59083 [details]
LayoutTests: expected.txt changes

For completness, the expected.txt changes - that previously lived in "Updated patch".

[Nikolas Zimmermann]

Landed in r61393. This will definately break bots, because I need to rebaseline all platform specific results. Waiting for the results now, and fixing incrementally.

[WebKit Review Bot]

http://trac.webkit.org/changeset/61393 might have broken Qt Linux Release

[Nikolas Zimmermann]

Rebaseline fixes:
r61396: Qt
r61398: Mac Tiger
r61399: Mac Snowleopard
r61401: Win
r61403: Gtk (by Dirk, thanks)

Notified chromium sheriff prior to landing (tkent), who will take care of the Chromium rebaseline.
That's it, closing bug!

[Simon Fraser (smfr)]

This broke CSS text-shadow applied to SVG (added via bug 30479)

[Simon Fraser (smfr)]

I filed bug 40960 on text-shadow.