Bug 76446 – RenderSVGRoot should inherit from RenderReplaced

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 122756 [details]
Patch v1

[WebKit Review Bot]

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

[Dirk Schulze]

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?

[Nikolas Zimmermann]

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

[Zoltan Herczeg]

> 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 :)

[Zoltan Herczeg]

Comment on attachment 122756 [details]
Patch v1

Everything was disscussed twice so r=me
Please check the bots after landing.

[Nikolas Zimmermann]

Committed r105247: <http://trac.webkit.org/changeset/105247>

[James Robinson]

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

[James Robinson]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

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

[Pavel Feldman]

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

[Nikolas Zimmermann]

Created attachment 123102 [details]
Patch

[Nikolas Zimmermann]

Committed r105401: <http://trac.webkit.org/changeset/105401>

[Stephen Chenney]

Committed r113439: <http://trac.webkit.org/changeset/113439>

[Stephen Chenney]

Committed r113452: <http://trac.webkit.org/changeset/113452>