Bug 40663 - Modernize SVG Text code, following the HTML design
Summary: Modernize SVG Text code, following the HTML design
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nikolas Zimmermann
URL:
Keywords:
: 12172 15386 15644 (view as bug list)
Depends on: 40665 40676
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-16 02:14 PDT by Nikolas Zimmermann
Modified: 2010-06-21 17:20 PDT (History)
8 users (show)

See Also:


Attachments
Initial patch (deleted)
2010-06-17 05:54 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Pixel test changes (deleted)
2010-06-17 05:58 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
Updated patch (deleted)
2010-06-17 06:22 PDT, Nikolas Zimmermann
krit: review-
Details | Formatted Diff | Diff
Updated patch v2 (183.48 KB, patch)
2010-06-18 00:43 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff
LayoutTests: expected.txt changes (deleted)
2010-06-18 01:40 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-06-16 02:14:22 PDT
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.
Comment 1 Nikolas Zimmermann 2010-06-17 01:39:15 PDT
*** Bug 15644 has been marked as a duplicate of this bug. ***
Comment 2 Nikolas Zimmermann 2010-06-17 01:39:24 PDT
*** Bug 15386 has been marked as a duplicate of this bug. ***
Comment 3 Nikolas Zimmermann 2010-06-17 01:40:14 PDT
*** Bug 12172 has been marked as a duplicate of this bug. ***
Comment 4 Nikolas Zimmermann 2010-06-17 05:54:47 PDT
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.
Comment 5 Nikolas Zimmermann 2010-06-17 05:58:42 PDT
Created attachment 58988 [details]
Pixel test changes
Comment 6 WebKit Review Bot 2010-06-17 06:03:03 PDT
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.
Comment 7 WebKit Review Bot 2010-06-17 06:07:36 PDT
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.
Comment 8 Eric Seidel (no email) 2010-06-17 06:09:17 PDT
Attachment 58987 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3275296
Comment 9 Nikolas Zimmermann 2010-06-17 06:22:07 PDT
Created attachment 58990 [details]
Updated patch

Fix build failure in release mode, fix style issues.
Comment 10 Dirk Schulze 2010-06-17 08:11:04 PDT
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.
Comment 11 Nikolas Zimmermann 2010-06-17 23:59:48 PDT
(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.
Comment 12 Nikolas Zimmermann 2010-06-18 00:43:51 PDT
Created attachment 59076 [details]
Updated patch v2

Just uploading a new patch, without test results.
Comment 13 Dirk Schulze 2010-06-18 01:05:10 PDT
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?
Comment 14 Dirk Schulze 2010-06-18 01:19:59 PDT
Comment on attachment 58988 [details]
Pixel test changes

r=me
Comment 15 Nikolas Zimmermann 2010-06-18 01:40:38 PDT
Created attachment 59083 [details]
LayoutTests: expected.txt changes

For completness, the expected.txt changes - that previously lived in "Updated patch".
Comment 16 Nikolas Zimmermann 2010-06-18 04:21:10 PDT
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.
Comment 17 WebKit Review Bot 2010-06-18 04:39:57 PDT
http://trac.webkit.org/changeset/61393 might have broken Qt Linux Release
Comment 18 Nikolas Zimmermann 2010-06-18 07:39:05 PDT
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!
Comment 19 Simon Fraser (smfr) 2010-06-21 17:17:09 PDT
This broke CSS text-shadow applied to SVG (added via bug 30479)
Comment 20 Simon Fraser (smfr) 2010-06-21 17:20:23 PDT
I filed bug 40960 on text-shadow.