Bug 76446 - RenderSVGRoot should inherit from RenderReplaced
Summary: RenderSVGRoot should inherit from RenderReplaced
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 76447
  Show dependency treegraph
 
Reported: 2012-01-17 06:06 PST by Nikolas Zimmermann
Modified: 2012-04-06 10:22 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (283.63 KB, patch)
2012-01-17 06:25 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2012-01-19 04:15 PST, Nikolas Zimmermann
pfeldman: 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 2012-01-17 06:06:10 PST
We currently have lots of duplicated code, because RenderSVGRoot tries to mimic a RenderReplaced which can also have children.
It's easy to move towards RenderReplaced, which solves code duplications, so I've done that.
Comment 1 Nikolas Zimmermann 2012-01-17 06:25:50 PST
Created attachment 122756 [details]
Patch v1
Comment 2 WebKit Review Bot 2012-01-17 07:09:45 PST
Comment on attachment 122756 [details]
Patch v1

Attachment 122756 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11117205

New failing tests:
svg/text/select-x-list-4.svg
Comment 3 Dirk Schulze 2012-01-17 15:43:12 PST
Comment on attachment 122756 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=122756&action=review

The pitch looks good to me. Just a  question to the code. But what about the test results? That can't be true? r- because of the results.

> Source/WebCore/ChangeLog:14
> +        It allows us to simplify the painting, as outlines, etc. are painted by RenderReplaced now.
> +        While I was it, speed up the local to border box computations by caching the result.

We haven't done this? :P

> Source/WebCore/rendering/RenderReplaced.cpp:125
> +    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseSelection && !canHaveChildren())

Is that just a performance improvement? Why do you need it in your patch?

> LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:8
> +      RenderSVGText {text} at (100,9) size 188x14 contains 1 chunk(s)
> +        RenderSVGInlineText {#text} at (0,0) size 188x14
> +          chunk 1 text run 1 at (100.00,20.00) startOffset 0 endOffset 41 width 188.00: "Some circles with ids, for linking tests."

Why do we get these big changes?

> LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:11
> +      RenderSVGText {text} at (203,87) size 39x14 contains 1 chunk(s)
> +        RenderSVGInlineText {#text} at (0,0) size 39x14

To be honest, I don#t understand why just RenderSVGText is affected?

> LayoutTests/platform/mac/svg/custom/mouse-move-on-svg-container-expected.txt:14
> -selection start: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of body
> -selection end:   position 24 of child 0 {#text} of child 3 {text} of child 1 {svg} of body
> +caret: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of body

Ouch, that looks like a regression. Please compare the image results. All text selection changes are gone. Have you checked that you got a clean DRT directory? Maybe one of your text patches caused that?
Comment 4 Nikolas Zimmermann 2012-01-18 00:28:04 PST
(In reply to comment #3)
> (From update of attachment 122756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122756&action=review
> 
> The pitch looks good to me. Just a  question to the code. But what about the test results? That can't be true? r- because of the results.
I'll explain the changes.

> 
> > Source/WebCore/ChangeLog:14
> > +        It allows us to simplify the painting, as outlines, etc. are painted by RenderReplaced now.
> > +        While I was it, speed up the local to border box computations by caching the result.
> 
> We haven't done this? :P
No :-)

> 
> > Source/WebCore/rendering/RenderReplaced.cpp:125
> > +    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseSelection && !canHaveChildren())
> 
> Is that just a performance improvement? Why do you need it in your patch?
This preserves the old behavior. RenderReplaced usually doesn't have any kids, and thus doesn't care about other phases as Foreground/Selection. Now RenderSVGRoot inherits from RenderReplaced, but has children, so we need to propagate the other phases as well. It's really straight-forward.

> 
> > LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:8
> > +      RenderSVGText {text} at (100,9) size 188x14 contains 1 chunk(s)
> > +        RenderSVGInlineText {#text} at (0,0) size 188x14
> > +          chunk 1 text run 1 at (100.00,20.00) startOffset 0 endOffset 41 width 188.00: "Some circles with ids, for linking tests."
> 
> Why do we get these big changes?
Easy. The RenderSVGRoot got a new viewBox, but it wasn't laid out again. That means the scaledFont in the RenderSVGInlineText objects weren't updated, so we ended up scaling up the font, instead of choosing a new font at the right size. I fixed that to properly relayout, and now kerning etc. changes.
 
> > LayoutTests/platform/mac/svg/custom/linking-a-03-b-all-expected.txt:11
> > +      RenderSVGText {text} at (203,87) size 39x14 contains 1 chunk(s)
> > +        RenderSVGInlineText {#text} at (0,0) size 39x14
> 
> To be honest, I don#t understand why just RenderSVGText is affected?
See last question.

> 
> > LayoutTests/platform/mac/svg/custom/mouse-move-on-svg-container-expected.txt:14
> > -selection start: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of body
> > -selection end:   position 24 of child 0 {#text} of child 3 {text} of child 1 {svg} of body
> > +caret: position 0 of child 0 {#text} of child 3 {text} of child 1 {svg} of body
> 
> Ouch, that looks like a regression. Please compare the image results. All text selection changes are gone. Have you checked that you got a clean DRT directory? Maybe one of your text patches caused that?
Heh, this is completely wanted. These tests click and hold the left mouse button over a circle, that gets moved! It should never trigger a selection, but we did previously, that's now fixed. Did you read my ChangeLog? I wrote most of the things there :(
Comment 5 Zoltan Herczeg 2012-01-18 00:31:25 PST
> Heh, this is completely wanted. These tests click and hold the left mouse button over a circle, that gets moved! It should never trigger a selection, but we did previously, that's now fixed. Did you read my ChangeLog? I wrote most of the things there :(

I had the same questions in #ksvg yesterday :)
Comment 6 Zoltan Herczeg 2012-01-18 00:34:37 PST
Comment on attachment 122756 [details]
Patch v1

Everything was disscussed twice so r=me
Please check the bots after landing.
Comment 7 Nikolas Zimmermann 2012-01-18 00:45:38 PST
Committed r105247: <http://trac.webkit.org/changeset/105247>
Comment 8 James Robinson 2012-01-18 11:44:06 PST
Did you intend to break svg/text/select-x-list-4.svg as well?  Because you did and I don't see any updates to the baseline or explanation in the changelog in this test:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Ftext%2Fselect-x-list-4.svg&showExpectations=true
Comment 9 James Robinson 2012-01-18 11:44:46 PST
(In reply to comment #2)
> (From update of attachment 122756 [details])
> Attachment 122756 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11117205
> 
> New failing tests:
> svg/text/select-x-list-4.svg

Look - the EWS bot clearly stated that this test was going to fail but as far as I can tell everyone ignored it on this bug.
Comment 10 Nikolas Zimmermann 2012-01-18 12:09:33 PST
(In reply to comment #2)
> (From update of attachment 122756 [details])
> Attachment 122756 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11117205
> 
> New failing tests:
> svg/text/select-x-list-4.svg(In reply to comment #9)
> (In reply to comment #2)
> > (From update of attachment 122756 [details] [details])
> > Attachment 122756 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11117205
> > 
> > New failing tests:
> > svg/text/select-x-list-4.svg
> 
> Look - the EWS bot clearly stated that this test was going to fail but as far as I can tell everyone ignored it on this bug.

We thought it was related to my previous checkin, not related to this bug report? Am I wrong?
Comment 11 Nikolas Zimmermann 2012-01-18 12:12:29 PST
(In reply to comment #8)
> Did you intend to break svg/text/select-x-list-4.svg as well?  Because you did and I don't see any updates to the baseline or explanation in the changelog in this test:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Ftext%2Fselect-x-list-4.svg&showExpectations=true

Oh indeed, thanks for the link. Reopening. Will look at it tomorrow after I got some sleep.
Comment 12 Nikolas Zimmermann 2012-01-18 12:14:53 PST
(In reply to comment #11)
> (In reply to comment #8)
> > Did you intend to break svg/text/select-x-list-4.svg as well?  Because you did and I don't see any updates to the baseline or explanation in the changelog in this test:
> > 
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Ftext%2Fselect-x-list-4.svg&showExpectations=true
> 
> Oh indeed, thanks for the link. Reopening. Will look at it tomorrow after I got some sleep.
Ah forgot to say, it works fine on Mac - and there's nothing Chromium specific in the test that could affect it. If that test fails, x-list-3/2/1.svg should fail as well. It must be related to the coordinates that are used in that testcase, and a selection behaviour difference between Mac/Chromium in a corner case.

IIRC that test selects text, by going to the start position on screen, clicking and holding the left mouse button, then moving to the end of the text. That should select all text, but apparently the selection gets reset after a certain point on Chromium. And that might be related to the RenderSVGRoot -> RenderReplaced change, which changed the canBeSelectionLeaf method.
Comment 13 Nikolas Zimmermann 2012-01-19 03:46:00 PST
(In reply to comment #12)
> IIRC that test selects text, by going to the start position on screen, clicking and holding the left mouse button, then moving to the end of the text. That should select all text, but apparently the selection gets reset after a certain point on Chromium. And that might be related to the RenderSVGRoot -> RenderReplaced change, which changed the canBeSelectionLeaf method.

Hm, its indeed related to the change - it exposes a general text selection problem, but its easier visible now. When selection "Foobar", and going past the last "r" after some pixels it will deselect Foobar, and start selecting the text on the next line. The metrics differ between Chromium/Mac that I obtain via the SVG text DOM apis (getStartPositionOfChar/getEndPositionOfChar). These positions are used to drive the mouse. Here's a trick, that maybe works:
diff --git a/LayoutTests/svg/text/resources/SelectionTestCase.js b/LayoutTests/svg/text/resources/SelectionTestCase.js
index f94832b..a02319d 100644
--- a/LayoutTests/svg/text/resources/SelectionTestCase.js
+++ b/LayoutTests/svg/text/resources/SelectionTestCase.js
@@ -66,6 +66,7 @@ function selectRange(id, start, end, expectedText) {
 
         var absStartPos = toAbsoluteCoordinates(startPos, element);
         var absEndPos = toAbsoluteCoordinates(endPos, element);
+        absEndPos.x -= 1;
 
         // Move to selection origin and hold down mouse
         eventSender.mouseMoveTo(absStartPos.x, absStartPos.y);

It need testing, this way it at least still works on Mac.
Context:
        // Trigger 'partial glyph selection' code, by adjusting the end x position by half glyph width
        var xOld = endPos.x;
        endPos.x -= endExtent.width / 2 - 1;
...

endPos.x is changed, before we transform it to absolute coordinates. That may produce the problems.
It needs a volunteer with Chromium to fix :(
Comment 14 Pavel Feldman 2012-01-19 04:11:23 PST
>          var absStartPos = toAbsoluteCoordinates(startPos, element);
>          var absEndPos = toAbsoluteCoordinates(endPos, element);
> +        absEndPos.x -= 1;

I applied this change to the ToT WebKit vs ToT Chromium and select-x-list-4.svg is now passing with --tolerance 0. It did not regress any of the other tests in the svg/text folder that are currently enabled for Chromium (~100 tests).
Comment 15 Nikolas Zimmermann 2012-01-19 04:15:14 PST
Created attachment 123102 [details]
Patch
Comment 16 Nikolas Zimmermann 2012-01-19 04:17:32 PST
Committed r105401: <http://trac.webkit.org/changeset/105401>
Comment 17 Stephen Chenney 2012-04-06 08:29:29 PDT
Committed r113439: <http://trac.webkit.org/changeset/113439>
Comment 18 Stephen Chenney 2012-04-06 10:22:54 PDT
Committed r113452: <http://trac.webkit.org/changeset/113452>