Bug 40663

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

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

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

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.

Created attachment 58988 [details] Pixel test changes

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.

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.

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

Created attachment 58990 [details] Updated patch Fix build failure in release mode, fix style issues.

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.

(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.

Created attachment 59076 [details] Updated patch v2 Just uploading a new patch, without test results.

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 on attachment 58988 [details] Pixel test changes r=me

Created attachment 59083 [details] LayoutTests: expected.txt changes For completness, the expected.txt changes - that previously lived in "Updated patch".

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.

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

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!

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

I filed bug 40960 on text-shadow.