RESOLVED FIXED 40663
Modernize SVG Text code, following the HTML design
https://bugs.webkit.org/show_bug.cgi?id=40663
Summary Modernize SVG Text code, following the HTML design
Nikolas Zimmermann
Reported 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.
Attachments
Initial patch (deleted)
2010-06-17 05:54 PDT, Nikolas Zimmermann
no flags
Pixel test changes (deleted)
2010-06-17 05:58 PDT, Nikolas Zimmermann
krit: review+
Updated patch (deleted)
2010-06-17 06:22 PDT, Nikolas Zimmermann
krit: review-
Updated patch v2 (183.48 KB, patch)
2010-06-18 00:43 PDT, Nikolas Zimmermann
krit: review+
LayoutTests: expected.txt changes (deleted)
2010-06-18 01:40 PDT, Nikolas Zimmermann
krit: review+
Nikolas Zimmermann
Comment 1 2010-06-17 01:39:15 PDT
*** Bug 15644 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 2 2010-06-17 01:39:24 PDT
*** Bug 15386 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 3 2010-06-17 01:40:14 PDT
*** Bug 12172 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 4 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.
Nikolas Zimmermann
Comment 5 2010-06-17 05:58:42 PDT
Created attachment 58988 [details] Pixel test changes
WebKit Review Bot
Comment 6 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.
WebKit Review Bot
Comment 7 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.
Eric Seidel (no email)
Comment 8 2010-06-17 06:09:17 PDT
Nikolas Zimmermann
Comment 9 2010-06-17 06:22:07 PDT
Created attachment 58990 [details] Updated patch Fix build failure in release mode, fix style issues.
Dirk Schulze
Comment 10 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.
Nikolas Zimmermann
Comment 11 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.
Nikolas Zimmermann
Comment 12 2010-06-18 00:43:51 PDT
Created attachment 59076 [details] Updated patch v2 Just uploading a new patch, without test results.
Dirk Schulze
Comment 13 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?
Dirk Schulze
Comment 14 2010-06-18 01:19:59 PDT
Comment on attachment 58988 [details] Pixel test changes r=me
Nikolas Zimmermann
Comment 15 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".
Nikolas Zimmermann
Comment 16 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.
WebKit Review Bot
Comment 17 2010-06-18 04:39:57 PDT
http://trac.webkit.org/changeset/61393 might have broken Qt Linux Release
Nikolas Zimmermann
Comment 18 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!
Simon Fraser (smfr)
Comment 19 2010-06-21 17:17:09 PDT
This broke CSS text-shadow applied to SVG (added via bug 30479)
Simon Fraser (smfr)
Comment 20 2010-06-21 17:20:23 PDT
I filed bug 40960 on text-shadow.
Note You need to log in before you can comment on or make changes to this bug.