Bug 70044 - SVG Filter on a group doesn't invalidate when children are moved
Summary: SVG Filter on a group doesn't invalidate when children are moved
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-10-13 12:19 PDT by Tim Horton
Modified: 2011-11-02 07:19 PDT (History)
8 users (show)

See Also:


Attachments
repro (490 bytes, image/svg+xml)
2011-10-13 12:20 PDT, Tim Horton
no flags Details
preliminary (no changelog/test) patch (2.59 KB, patch)
2011-10-21 16:36 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (18.09 KB, patch)
2011-10-26 13:17 PDT, Tim Horton
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff
fix the test (1.16 KB, patch)
2011-11-01 14:07 PDT, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-10-13 12:19:49 PDT
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.
Comment 1 Tim Horton 2011-10-13 12:20:05 PDT
Created attachment 110886 [details]
repro
Comment 2 Radar WebKit Bug Importer 2011-10-13 12:20:28 PDT
<rdar://problem/10281529>
Comment 3 Radar WebKit Bug Importer 2011-10-13 12:20:32 PDT
<rdar://problem/10281530>
Comment 4 Dirk Schulze 2011-10-13 12:41:35 PDT
Adding Reni to this bug.
Comment 5 Tim Horton 2011-10-21 16:36:57 PDT
Created attachment 112042 [details]
preliminary (no changelog/test) patch

I don't like the "only call clientChildLayoutChanged if we didn't clientLayoutChanged" semantic.
Comment 6 Nikolas Zimmermann 2011-10-22 00:12:43 PDT
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().
Comment 7 Tim Horton 2011-10-26 13:17:46 PDT
Created attachment 112583 [details]
patch
Comment 8 Nikolas Zimmermann 2011-10-27 00:29:37 PDT
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 9 WebKit Review Bot 2011-10-27 07:01:48 PDT
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
Comment 10 Tim Horton 2011-10-27 11:05:41 PDT
(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).
Comment 11 Nikolas Zimmermann 2011-10-27 23:55:21 PDT
(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 :-)
Comment 12 Tim Horton 2011-11-01 12:14:09 PDT
Landed in r98989.
Comment 13 John Gregg 2011-11-01 13:25:52 PDT
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.
Comment 14 Tim Horton 2011-11-01 13:29:44 PDT
(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.
Comment 15 Tim Horton 2011-11-01 14:07:14 PDT
Created attachment 113212 [details]
fix the test
Comment 16 Tim Horton 2011-11-01 17:34:33 PDT
For whatever reason (because this was marked RESOLVED?) commit-queue never took the patch. Landed manually as r99017.
Comment 17 Patrick R. Gansterer 2011-11-02 07:19:23 PDT
Landed build fix for !ENABLE(FILTERS) in http://trac.webkit.org/changeset/99056