Bug 73860

Summary: feImage doesn't invalidate when its target SVG element is animated
Product: WebKit Reporter: Tim Horton <thorton>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, simon.fraser, webkit-bug-importer, webkit.review.bot, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 10403    
Attachments:
Description Flags
repro
none
Patch
none
Patch v2 krit: review+, krit: commit-queue-

Tim Horton
Reported 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).
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
DetailsFormatted DiffDiff
Patch v2 (123.97 KB, patch)
2012-02-08 03:52 PST, Nikolas Zimmermann 		krit: review+
krit: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2011-12-05 13:33:19 PST
<rdar://problem/10529537>
Tim Horton
Comment 2 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...
Nikolas Zimmermann
Comment 3 2012-01-27 02:13:06 PST
I have a fix for this. Will be uploaded soon on bug 10403.
Nikolas Zimmermann
Comment 4 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.
Nikolas Zimmermann
Comment 5 2012-01-27 12:33:42 PST
Created attachment 124351 [details] Patch
Nikolas Zimmermann
Comment 6 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.
Tim Horton
Comment 7 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!
WebKit Review Bot
Comment 8 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
Dirk Schulze
Comment 9 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.
Nikolas Zimmermann
Comment 10 2012-02-08 03:52:40 PST
Created attachment 126045 [details] Patch v2
Dirk Schulze
Comment 11 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.
Nikolas Zimmermann
Comment 12 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.
Nikolas Zimmermann
Comment 13 2012-02-08 04:12:56 PST
Committed r107067: <http://trac.webkit.org/changeset/107067>
Note You need to log in before you can comment on or make changes to this bug.