SVG Animation doesn't respect 'currentColor'

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Animations in SVG don't respect the 'currentColor' of an element. The third rect in the example above should be filled to currentColor (green) but stays black. The SVGAnimateElement trys to parse currentColor directly with the CSS parser. But currentColor is part of CSS3 and not implemented for CSS yet.

[Dirk Schulze]

Created attachment 51833 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 51833 [details]
Patch

Patch looks fine, the test is flawed though.
New SVG animation tests (not part of SVG 1.1 testsuite) need to go into svg/animations and need to be implemented using the SVG animation specific DRT commands, sampling the animation values at certain times.
When running your new test through DRT, the animation is not executed at all - the png even reflects this, as only one green rects is shown.

Happy to review the follow-up :-)

[Dirk Schulze]

(In reply to comment #2)
> (From update of attachment 51833 [details])
> Patch looks fine, the test is flawed though.
> New SVG animation tests (not part of SVG 1.1 testsuite) need to go into
> svg/animations and need to be implemented using the SVG animation specific DRT
> commands, sampling the animation values at certain times.
> When running your new test through DRT, the animation is not executed at all -
> the png even reflects this, as only one green rects is shown.
> 
> Happy to review the follow-up :-)

It reflects it. It's not realy neccessary to know what happens during the animation. It's just neccessary if the correct color for 'currentColor' is detected. And with the current trunk, this test would fail.

[Nikolas Zimmermann]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 51833 [details] [details])
> > Patch looks fine, the test is flawed though.
> > New SVG animation tests (not part of SVG 1.1 testsuite) need to go into
> > svg/animations and need to be implemented using the SVG animation specific DRT
> > commands, sampling the animation values at certain times.
> > When running your new test through DRT, the animation is not executed at all -
> > the png even reflects this, as only one green rects is shown.
> > 
> > Happy to review the follow-up :-)
> 
> It reflects it. It's not realy neccessary to know what happens during the
> animation. It's just neccessary if the correct color for 'currentColor' is
> detected. And with the current trunk, this test would fail.

Hm, I'm confused. So the testcase should only show one green rect? Not four rects at all?
It's not clear on first sight, so you might want to add comments.

[Dirk Schulze]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 51833 [details] [details] [details])
> > > Patch looks fine, the test is flawed though.
> > > New SVG animation tests (not part of SVG 1.1 testsuite) need to go into
> > > svg/animations and need to be implemented using the SVG animation specific DRT
> > > commands, sampling the animation values at certain times.
> > > When running your new test through DRT, the animation is not executed at all -
> > > the png even reflects this, as only one green rects is shown.
> > > 
> > > Happy to review the follow-up :-)
> > 
> > It reflects it. It's not realy neccessary to know what happens during the
> > animation. It's just neccessary if the correct color for 'currentColor' is
> > detected. And with the current trunk, this test would fail.
> 
> Hm, I'm confused. So the testcase should only show one green rect? Not four
> rects at all?
> It's not clear on first sight, so you might want to add comments.

4 green rects, that form one rect of the size 100x100 :-)

[Nikolas Zimmermann]

Comment on attachment 51833 [details]
Patch

Jeez, misread the testcase! :-) Fine with me as is, r=me.

[Nikolas Zimmermann]

Hm, I recall a W3C test that should be fixed by this. Is it part of the 2nd edition and not present in trunk?

[Dirk Schulze]

(In reply to comment #7)
> Hm, I recall a W3C test that should be fixed by this. Is it part of the 2nd
> edition and not present in trunk?

The bad thing about W3C tests is, that they not only test one thing, but different parts of SVG per test. :-(
Two tests on the test suite look better with the patch, but still don't pass. Already opened bug reports about the issues and added you to the CC lists :-)

[Dirk Schulze]

Comment on attachment 51833 [details]
Patch

Clearing flags on attachment: 51833

Committed r56775: <http://trac.webkit.org/changeset/56775>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

svg/custom/animation-currentColor.svg has been failing on teh Gtk 32-bit Debug builder since this change.  Please fix. :)

[Dirk Schulze]

Commited DRT update in http://trac.webkit.org/changeset/56971