WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
Attachment 58987
[details]
did not build on mac: Build output:
http://webkit-commit-queue.appspot.com/results/3275296
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.
Top of Page
Format For Printing
XML
Clone This Bug