[Webkit-unassigned] [Bug 78385] REGRESSION(r105057): Dynamically changing <tspan> offsets is broken

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78385


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #126611|review?                     |review-
               Flag|                            |




--- Comment #6 from Nikolas Zimmermann <zimmermann at kde.org>  2012-02-11 15:41:28 PST ---
(From update of attachment 126611)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.