Repro attached; the filter is attached to a <g>, we're moving children of the <g> (a <circle>) from within JS. When the filter is applied, the filter never invalidates.
Created attachment 110886 [details] repro
<rdar://problem/10281529>
<rdar://problem/10281530>
Adding Reni to this bug.
Created attachment 112042 [details] preliminary (no changelog/test) patch I don't like the "only call clientChildLayoutChanged if we didn't clientLayoutChanged" semantic.
Comment on attachment 112042 [details] preliminary (no changelog/test) patch View in context: https://bugs.webkit.org/attachment.cgi?id=112042&action=review > Source/WebCore/rendering/svg/RenderSVGContainer.cpp:73 > + if (m_everHadLayout) { > + if (selfNeedsLayout()) > + SVGResourcesCache::clientLayoutChanged(this); > + else if(needsLayout()) > + SVGResourcesCache::clientChildLayoutChanged(this); > + } In general the idea looks sane to me. I'd propose to not do any selfNeedsLayout/needsLayout checks on the call site at all but leave that to clientLayoutChanged().
Created attachment 112583 [details] patch
Comment on attachment 112583 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=112583&action=review r=me, assuming no pixel regressions. Some comments on the test: > LayoutTests/svg/filters/invalidate-on-child-layout.svg:1 > +<svg xmlns="http://www.w3.org/2000/svg" onload="setup()"> The onload="setup()" is not really needed. You can just use setTimeout(jump, 0) in the <script> directly. Also I'd rename jump to executeTest. > LayoutTests/svg/filters/invalidate-on-child-layout.svg:6 > + layoutTestController.display(); Why is display() needed? I guess you want to test repainting, but in the expected.png I can not see any gray overlay, which should be produced by display(). Can it be removed? > LayoutTests/svg/filters/invalidate-on-child-layout.svg:16 > + a.setAttributeNS(null,"cx",100); > + a.setAttributeNS(null,"cy",100); setAttribute("cx", "100") ... > LayoutTests/svg/filters/invalidate-on-child-layout.svg:29 > +<g filter="url(#filt)"> > + <circle id="c" r="50" cx="50" cy="50" /> Nitpick, I'd name the resources "filter" and "circle", to avoid confusion.
Comment on attachment 112583 [details] patch Attachment 112583 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10226697 New failing tests: svg/filters/invalidate-on-child-layout.svg
(In reply to comment #8) > (From update of attachment 112583 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112583&action=review > > Why is display() needed? I guess you want to test repainting, but in the expected.png I can not see any gray overlay, which should be produced by display(). Can it be removed? Is there a way to force a repaint without doing the grey wash thing? Without the display(), I was seeing (before the fix) the test *pass* sometimes because the animation would happen before the first paint. (i.e. without the display(), before this fix, the test is a flaky fail).
(In reply to comment #10) > Is there a way to force a repaint without doing the grey wash thing? Without the display(), I was seeing (before the fix) the test *pass* sometimes because the animation would happen before the first paint. (i.e. without the display(), before this fix, the test is a flaky fail). Hm, good question. Without knowing exactly what's causing the pass theres probably no other way. Anyhow, it's fine with me, this shouldn't block your patch. Let's live with it :-)
Landed in r98989.
In your test, you removed the setup() method but left the call to it in onload. This causes chromium to fail your layout test because it throws a ReferenceError in the output.
(In reply to comment #13) > In your test, you removed the setup() method but left the call to it in onload. This causes chromium to fail your layout test because it throws a ReferenceError in the output. Whoops! I'll fix that right away.
Created attachment 113212 [details] fix the test
For whatever reason (because this was marked RESOLVED?) commit-queue never took the patch. Landed manually as r99017.
Landed build fix for !ENABLE(FILTERS) in http://trac.webkit.org/changeset/99056