feImage doesn't invalidate when its target SVG element is animated

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

Created attachment 117934 [details]
repro

This is blocking SVG Wow!'s "Twirl" demo, and potentially others.

If you have an <feImage> that references a SVG element (in my case, a <rect>), and the target element is animated (I have an <animateTransform>), the feImage doesn't invalidate along with the animation.

See the attached test-case in WebKit vs. Opera. (in Opera, the rectangle rotates, as expected; in WebKit, it stays still).

[Radar WebKit Bug Importer]

<rdar://problem/10529537>

[Tim Horton]

feImage is rather unfortunate; it's supposed to act like <use> in many cases, but we have special-cases dotted throughout the code for <use> that can't be used for feImage...

[Nikolas Zimmermann]

I have a fix for this. Will be uploaded soon on bug 10403.

[Nikolas Zimmermann]

Reversing the logic, let 10403 depend on this bug report, as its better suited for the upcoming fix.

[Nikolas Zimmermann]

Created attachment 124351 [details]
Patch

[Nikolas Zimmermann]

Note, that this only fully fixes bug 10403 modulo the unload issue. SVG Twirl now starts reacting, but there's still another bug besides the repro that Tim found out. The ChangeLog is a bit incorrect, please ignore the sentence that it also fixes SVG Twirl demo fully.

[Tim Horton]

(In reply to comment #6)
> Note, that this only fully fixes bug 10403 modulo the unload issue. SVG Twirl now starts reacting, but there's still another bug besides the repro that Tim found out. The ChangeLog is a bit incorrect, please ignore the sentence that it also fixes SVG Twirl demo fully.

IIRC the next issue with Twirl is that feDisplacement is broken somehow; I have a bunch of bugs written down that I need to file.

This looks like a reasonable approach to fixing this; I haven't tried it out, but it looks good to me!

[WebKit Review Bot]

Comment on attachment 124351 [details]
Patch

Attachment 124351 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11191856

New failing tests:
svg/filters/feImage-target-attribute-change.svg
svg/filters/feImage-late-indirect-update.svg
svg/filters/feImage-target-inline-style-change.svg
svg/filters/feImage-target-attribute-change-with-use-indirection.svg
svg/filters/feImage-target-style-change.svg
svg/filters/feImage-target-attribute-change-with-use-indirection-2.svg
svg/filters/feImage-mutliple-targets-id-change.svg
svg/filters/feImage-animated-transform-on-target-rect.svg
svg/filters/feImage-target-property-change.svg

[Dirk Schulze]

Comment on attachment 124351 [details]
Patch

LGTM. r=me! We talked about the 2 setTimeouts per test. Niko wants to avoid that before landing. Please check chromium, I forgot to do that.

[Nikolas Zimmermann]

Created attachment 126045 [details]
Patch v2

[Dirk Schulze]

Comment on attachment 126045 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=126045&action=review

Still looks good to me ;) r=me. Great when it is fixed.

> LayoutTests/svg/filters/feImage-animated-transform-on-target-rect.svg:32
> +          if (window.layoutTestController)
> +            layoutTestController.notifyDone();

Niko already noted, that he will remove them on landing, since they are unnecessary.

[Nikolas Zimmermann]

Comment on attachment 126045 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=126045&action=review

>> LayoutTests/svg/filters/feImage-animated-transform-on-target-rect.svg:32
>> +            layoutTestController.notifyDone();
> 
> Niko already noted, that he will remove them on landing, since they are unnecessary.

For clarification, that notifyDone() has to stay, we're using setTimeouts() to poll the animation status...

> LayoutTests/svg/filters/feImage-change-target-id.svg:20
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();

... this one is obsolete, and will be removed.

[Nikolas Zimmermann]

Committed r107067: <http://trac.webkit.org/changeset/107067>