Bug 78385 – REGRESSION(r105057): Dynamically changing <tspan> offsets is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

Created attachment 126577 [details]
repro

After 105057, you can no longer change the x attribute of <tspan> (unless you're in an onload handler).

Attaching a simple repro.

<rdar://problem/10832932>

[Tim Horton]

It looks like we're never clearing m_textPositions (never hitting clearTextPositioningElements()?) so we never end up at fillCharacterDataMap.

[Tim Horton]

I have a very simple patch, I just have to make sure it doesn't break anything.

[Tim Horton]

Created attachment 126611 [details]
patch

Will need baselines for other platforms.

[WebKit Review Bot]

Comment on attachment 126611 [details]
patch

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

New failing tests:
svg/text/tspan-dynamic-positioning.svg

[Philip Rogers]

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

[Nikolas Zimmermann]

Comment on attachment 126611 [details]
patch

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

Thanks Tim for your quick investigation! That saved me a lot time, as I could easily grasp the problem:

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:1
> +<svg onload="load()" xmlns="http://www.w3.org/2000/svg" version="1.1" width="600" height="500">

Pixel tests should use layoutTestController.display() now, see eg. bug 77736.
Please import fast/repaint/resources/repaint.js, and attach <svg onload="runRepaintTest()".

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:5
> +        function move()

s/move/repaintTest/ - which gets invoked by runRepaintTest, once if finished layout and an initial paint, which is what you want here.

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:11
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();

This is then unnecessary.

> LayoutTests/svg/text/tspan-dynamic-positioning.svg:19
> +        function load()
> +        {
> +            if (window.layoutTestController)
> +                layoutTestController.waitUntilDone();
> +            setTimeout(move, 0);
> +        }

Ditto, can be removed as well - also it gives you the nice gray overlay rect, so we can really be sure that repainting worked :-)
Please always use this for future tests, and tell anyone to do so.

> Source/WebCore/ChangeLog:13
> +        Test: svg/text/tspan-dynamic-positioning.svg.

Trailing period.

> Source/WebCore/rendering/svg/RenderSVGText.cpp:220
> +        m_layoutAttributesBuilder.clearTextPositioningElements();

While this works for sure, it would regress performance. The text positioning elements cache should only ever change when textDOMChanged() gets called.
We only need to invalidate the text positioning elements list, if a) the text length of any RenderSVGInlineText changes b) the <text> DOM subtree mutates in some way. This is already captured correctly, and we have tests covering that.

When reading through the code, I just realized that buildLayoutAttributesIfNeeded is wrong. It shouldn't early exit if m_textPositions is not empty, we still need to call buildLayoutAttributes!
The only thing we can save is the collectTextPositioningElements and thus a full walk of the SVG <text> subtree - - we just forgot to layout in that case.

A shame, we didn't have a test yet covering just that.

[Tim Horton]

Created attachment 126799 [details]
new patch

I like the repaint test helper! I had used .display() before, I'd just kind of forgotten about it. If that's what we're to use, I'll keep that in mind.

The rest of your comments make sense; here's a new patch.

[WebKit Review Bot]

Comment on attachment 126799 [details]
new patch

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

New failing tests:
svg/text/tspan-dynamic-positioning.svg

[Nikolas Zimmermann]

(In reply to comment #7)
> The rest of your comments make sense; here's a new patch.
Thanks a lot, please don't forget to mark the test as IMAGE(+TEXT, not sure?) in platform/chromium/test_expectations.txt, as that keeps their bot green, until it's rebaselined. (I was told that's the most polite way..)

[Tim Horton]

(In reply to comment #9)
> (In reply to comment #7)
> > The rest of your comments make sense; here's a new patch.
> Thanks a lot, please don't forget to mark the test as IMAGE(+TEXT, not sure?) in platform/chromium/test_expectations.txt, as that keeps their bot green, until it's rebaselined. (I was told that's the most polite way..)

Is that what we're supposed to do? I've never gotten a clear and consistent answer on that point.

[Nikolas Zimmermann]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > The rest of your comments make sense; here's a new patch.
> > Thanks a lot, please don't forget to mark the test as IMAGE(+TEXT, not sure?) in platform/chromium/test_expectations.txt, as that keeps their bot green, until it's rebaselined. (I was told that's the most polite way..)
> 
> Is that what we're supposed to do? I've never gotten a clear and consistent answer on that point.

Well that's the purpose of the cr-linux bot running tests - you can immediately see that cr-linux is red, if you don't touch the expectations. I hope we'll all switch to expectations file at some point...

[Adam Barth]

> Is that what we're supposed to do? I've never gotten a clear and consistent answer on that point.

Yes.  It's also helpful if you follow through, either by landing the correct baselines yourself or by letting the current gardener that they'll need to do that.

[Tim Horton]

(In reply to comment #12)
> > Is that what we're supposed to do? I've never gotten a clear and consistent answer on that point.
> 
> Yes.  It's also helpful if you follow through, either by landing the correct baselines yourself or by letting the current gardener that they'll need to do that.

This is a new test, it doesn't have any baselines yet. Is that still the thing to do? If yes, it seems like everything could be a bit smarter in the new-test case.

[Tim Horton]

Landed in http://trac.webkit.org/changeset/107862

[James Robinson]

I've landed new Chromium baselines in http://trac.webkit.org/changeset/107958

[Ryosuke Niwa]

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