Bug 36718 - SVG no AnimateColor for stroke or fill if they are set to none on target
Summary: SVG no AnimateColor for stroke or fill if they are set to none on target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-28 01:23 PDT by Dirk Schulze
Modified: 2010-04-20 10:32 PDT (History)
4 users (show)

See Also:

Attachments
Test. The rect should turn from red to green smoothly during a second. (406 bytes, image/svg+xml)
2010-03-28 01:23 PDT, Dirk Schulze 		no flags Details
Patch (19.08 KB, patch)
2010-04-12 23:55 PDT, Robin Cao 		no flags Details | Formatted Diff | Diff
Updated patch (42.33 KB, patch)
2010-04-20 03:04 PDT, Robin Cao 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-03-28 01:23:15 PDT 
Created attachment 51853 [details]
Test. The rect should turn from red to green smoothly during a second.

If stroke is not set or 'none', the animation of stroke doesn't work. The color, defined in 'from', is used for the whole animation. The same for fill, if fill is set to 'none' on the target.
Comment 1 Robin Cao 2010-04-12 23:55:05 PDT 
Created attachment 53228 [details]
Patch

Patch
Comment 2 Dirk Schulze 2010-04-13 00:28:16 PDT 
Why is the property type String? And why are both strings empty?
Comment 3 Robin Cao 2010-04-13 01:00:15 PDT 
(In reply to comment #2)
> Why is the property type String? And why are both strings empty?

The property is set to "StringProperty" in SVGAnimateElement::resetToBaseValue, because there is no valid color for a none target. The check for empty is to ensure that this animate element is not indeed type String.

I admit this patch looks ugly. Maybe the none target should be handled appropriately in SVGAnimateElement::resetToBaseValue. Do you have any suggestions?
Comment 4 Dirk Schulze 2010-04-13 01:18:15 PDT 
(In reply to comment #3)
> (In reply to comment #2)
> > Why is the property type String? And why are both strings empty?
> 
> The property is set to "StringProperty" in SVGAnimateElement::resetToBaseValue,
> because there is no valid color for a none target. The check for empty is to
> ensure that this animate element is not indeed type String.
> 
> I admit this patch looks ugly. Maybe the none target should be handled
> appropriately in SVGAnimateElement::resetToBaseValue. Do you have any
> suggestions?

Yes, if the property Color is set to String in resetToBaseValue, it should be fixed there.
Comment 5 Robin Cao 2010-04-20 03:04:47 PDT 
Created attachment 53791 [details]
Updated patch

Update patch according to Dirk's advice. Many thanks to Dirk for the review.
Comment 6 Dirk Schulze 2010-04-20 06:40:55 PDT 
Comment on attachment 53791 [details]
Updated patch

Great patch! r=me.
Comment 7 Dirk Schulze 2010-04-20 10:27:58 PDT 
(In reply to comment #6)
> (From update of attachment 53791 [details])
> Great patch! r=me.

Land the patches manually soon.
Comment 8 Dirk Schulze 2010-04-20 10:32:23 PDT 
Comment on attachment 53791 [details]
Updated patch

Clearing flags on attachment: 53791

Committed r57896: <http://trac.webkit.org/changeset/57896>
Comment 9 Dirk Schulze 2010-04-20 10:32:34 PDT 
All reviewed patches have been landed.  Closing bug.