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>
It looks like we're never clearing m_textPositions (never hitting clearTextPositioningElements()?) so we never end up at fillCharacterDataMap.
I have a very simple patch, I just have to make sure it doesn't break anything.
Created attachment 126611 [details] patch Will need baselines for other platforms.
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
*** Bug 78015 has been marked as a duplicate of this bug. ***
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.
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.
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
(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..)
(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.
(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...
> 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.
(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.
Landed in http://trac.webkit.org/changeset/107862
I've landed new Chromium baselines in http://trac.webkit.org/changeset/107958
*** Bug 78376 has been marked as a duplicate of this bug. ***