Bug 70044 – SVG Filter on a group doesn't invalidate when children are moved

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

[Tim Horton]

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.

[Tim Horton]

Created attachment 110886 [details]
repro

[Radar WebKit Bug Importer]

<rdar://problem/10281529>

[Radar WebKit Bug Importer]

<rdar://problem/10281530>

[Dirk Schulze]

Adding Reni to this bug.

[Tim Horton]

Created attachment 112042 [details]
preliminary (no changelog/test) patch

I don't like the "only call clientChildLayoutChanged if we didn't clientLayoutChanged" semantic.

[Nikolas Zimmermann]

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

[Tim Horton]

Created attachment 112583 [details]
patch

[Nikolas Zimmermann]

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.

[WebKit Review Bot]

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

[Tim Horton]

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

[Nikolas Zimmermann]

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

[Tim Horton]

Landed in r98989.

[John Gregg]

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.

[Tim Horton]

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

[Tim Horton]

Created attachment 113212 [details]
fix the test

[Tim Horton]

For whatever reason (because this was marked RESOLVED?) commit-queue never took the patch. Landed manually as r99017.

[Patrick R. Gansterer]

Landed build fix for !ENABLE(FILTERS) in http://trac.webkit.org/changeset/99056