Bug 73860 - feImage doesn't invalidate when its target SVG element is animated
Summary: feImage doesn't invalidate when its target SVG element is animated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on:
Blocks: 10403
  Show dependency treegraph
 
Reported: 2011-12-05 13:32 PST by Tim Horton
Modified: 2012-02-08 04:12 PST (History)
5 users (show)

See Also:


Attachments
repro (525 bytes, image/svg+xml)
2011-12-05 13:32 PST, Tim Horton
no flags Details
Patch (93.38 KB, patch)
2012-01-27 12:33 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (123.97 KB, patch)
2012-02-08 03:52 PST, Nikolas Zimmermann
krit: review+
krit: commit-queue-
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-12-05 13:32:32 PST
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).
Comment 1 Radar WebKit Bug Importer 2011-12-05 13:33:19 PST
<rdar://problem/10529537>
Comment 2 Tim Horton 2011-12-05 14:48:30 PST
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...
Comment 3 Nikolas Zimmermann 2012-01-27 02:13:06 PST
I have a fix for this. Will be uploaded soon on bug 10403.
Comment 4 Nikolas Zimmermann 2012-01-27 06:33:41 PST
Reversing the logic, let 10403 depend on this bug report, as its better suited for the upcoming fix.
Comment 5 Nikolas Zimmermann 2012-01-27 12:33:42 PST
Created attachment 124351 [details]
Patch
Comment 6 Nikolas Zimmermann 2012-01-27 12:35:19 PST
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.
Comment 7 Tim Horton 2012-01-27 12:38:45 PST
(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!
Comment 8 WebKit Review Bot 2012-01-27 13:47:14 PST
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
Comment 9 Dirk Schulze 2012-02-01 02:39:15 PST
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.
Comment 10 Nikolas Zimmermann 2012-02-08 03:52:40 PST
Created attachment 126045 [details]
Patch v2
Comment 11 Dirk Schulze 2012-02-08 04:00:51 PST
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.
Comment 12 Nikolas Zimmermann 2012-02-08 04:08:06 PST
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.
Comment 13 Nikolas Zimmermann 2012-02-08 04:12:56 PST
Committed r107067: <http://trac.webkit.org/changeset/107067>