Bug 21586 – SVG CSS properties don't work with CSS transitions

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Jonathon Jongsma (jonner)]

Trying to use CSS transitions with SVG content doesn't work with (at least some) CSS properties that seem like they should be animatable.

[Dave Hyatt]

http://trac.webkit.org/changeset/37579

I added support for a few.

[Rob Buis]

Created attachment 58332 [details]
Work in progress patch

I am adding this work-in-progress patch to get some feedback and because some things are still unclear.
What I tried were text-anchor, stroke-linejoin and stroke-linecap, those worked.
I then tried stroke-linewidth but there is a bug because it interpolates between two equal float values. Can this be because
the animation framework relies on PODs where here we use CSSValue pointers? I am basically not sure where RenderStyle *a
and *b come from. Note that this code is messy due to float <-> CSSValue* inconsistencies, but making that nicer could involve
changing RenderStyle.h/SVGRenderStyle.h a lot, which I want to prevent right now.
For the remaining SVG properties, I doubt some are very useful, like pointer-events. For properties that reference urls like
markers I am not sure how the smooth progress would go, I assume it is rather a sort of on/off switch. Finally I am not sure
how SVGPaint in stroke and fills should provide smooth progress, for instance when it starts with red paint but transforms to
some gradient? It would be great if this was documented somewhere, or if there already is a reference implementation.
Cheers,

Rob.

[Dirk Schulze]

It's also the quastion how CSS transitions work together with SMIL animation? One value can be animated by both at te same time. Did someone test it?

[Dirk Schulze]

Created attachment 58361 [details]
Test with CSS transistions and SMIL Animation

This is a quiete harmless test. But even this tests shows the problem. WebKit is jumping between the differnent animations (CSS and SVG Animation). Also results differ a bit between WebKit based browsers. On Gtk the opacity is between 0.5 and 0 all the time, oscilating from one to the other. Chromium animates from 1->0 than from 0 ->0.5 jumps to 1 and restarts the process. It should be tested what happens on additive sum, different animation functions and so on.

[Simon Fraser (smfr)]

This describes which properties transition:
http://dev.w3.org/csswg/css3-transitions/#animatable-properties-

How SMIL and CSS animations interact is currently unspecified.

[Simon Fraser (smfr)]

The patch looks reasonable, BTW.

[Rob Buis]

Hello Simon,

(In reply to comment #5)
> This describes which properties transition:
> http://dev.w3.org/csswg/css3-transitions/#animatable-properties-
> 
> How SMIL and CSS animations interact is currently unspecified.

Thanks a lot! I was looking at the wrong spec then, it had no explanation about the SVG properties at all.
Cheers,

Rob.

[Dirk Schulze]

(In reply to comment #6)
> The patch looks reasonable, BTW.

If I compare the list of animated SVG attributes (that can be set by CSS), with the list at http://dev.w3.org/csswg/css3-transitions/#animatable-properties, we can just use a few transitions in SVG, mainly some font-settings and the color-attribute/property. Means, even the current SVG attributes defined for transitions in http://trac.webkit.org/browser/trunk/WebCore/page/animation/AnimationBase.cpp#L677 are not allowed.

But maybe the spec can change in that way, not to define the property names, but define transitions by the data type, see: http://www.w3.org/TR/SVG/animate.html#Animatable

[Dean Jackson]

You're right - the specification should define the animatable properties by datatype. This is my mistake.

[Dirk Schulze]

How do we test CSS Transitions in DRT?

[Simon Fraser (smfr)]

See LayoutTests/transitions

[Eric Mill]

Is there any renewed interest in adding more supported attributes? "fill" is a big one, for example. 

I'm doing some SVG animations with Raphael, but using CSS animations for effects where possible makes things much faster, and makes more interesting work possible.

[Dirk Schulze]

(In reply to comment #12)
> Is there any renewed interest in adding more supported attributes? "fill" is a big one, for example. 
> 
> I'm doing some SVG animations with Raphael, but using CSS animations for effects where possible makes things much faster, and makes more interesting work possible.

There is still a lot of interest in supporting more SVG-CSS values for CSS transitions and animations. Most SVG people are just busy with fixing other issues. So we are always searching for volunteers. Do you want to help? The attached patch should help to add more CSS properties, including fill and stroke. Just needs some testing. But I am sure we can help on testing.

[Dean Jackson]

I updated Rob's patch recently but it is on a machine that is powered down on the other side of the world :) I plan to submit it for review next week. 

I was taking the approach that SMIL + CSS animations on the same property just don't mix. I think that's an ok situation for now.

[Eric Mill]

(In reply to comment #13)
> There is still a lot of interest in supporting more SVG-CSS values for CSS transitions and animations. Most SVG people are just busy with fixing other issues. So we are always searching for volunteers. Do you want to help? The attached patch should help to add more CSS properties, including fill and stroke. Just needs some testing. But I am sure we can help on testing.

I've never done a Webkit patch before, but I might give it a shot. I might be more useful as a tester, though.

[Dean Jackson]

Created attachment 104327 [details]
Not quite done patch

Attaching a not quite ready patch that can animate/transition SVG properties for opacity, length, colour and a few others. It will also transition fill and stroke paints as long as both ends of the animation are colours. I expect to finish this off today, then write tests.
(I notice there is a typo where the line-cap and line-join properties are referencing the wrong base)

[Dirk Schulze]

Comment on attachment 104327 [details]
Not quite done patch

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

General question: Do you respect fallback colors for fill and stroke? fill="url(#invalid) green" this should take a the solid color green.

> Source/WebCore/svg/SVGLength.h:141
> +            float fromPercent = from.valueAsPercentage();
> +            float toPercent = valueAsPercentage();
> +            length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent + (toPercent - fromPercent) * progress, ec);

That means if you animate from cm to percentage, the calculated css value will be in percentage for the whole animation? That is different to SMIL animation, where we switch the unit type after 50% of the animation. If this is the way to go in CSS animations/transitions in general I'm fine. Should be discussed in publicFX as well.

> Source/WebCore/svg/SVGLength.h:145
> +            float fromValue = from.valueInSpecifiedUnits();
> +            float toValue = valueInSpecifiedUnits();
> +            length.newValueSpecifiedUnits(resultType, fromValue + (toValue - fromValue) * progress, ec);

Same like above.

> Source/WebCore/svg/SVGLength.h:152
> +

skip this line.

[Dean Jackson]

Created attachment 104486 [details]
Patch

[Dean Jackson]

Hi Dirk,

I didn't see this before I uploaded the most recent patch.

> General question: Do you respect fallback colors for fill and stroke? fill="url(#invalid) green" this should take a the solid color green.

I don't do anything here. If the style system resolves to give a Color to fill, then it can be transitioned (as long as the destination is also a Color). So in theory, this should work.

> > Source/WebCore/svg/SVGLength.h:141
> > +            float fromPercent = from.valueAsPercentage();
> > +            float toPercent = valueAsPercentage();
> > +            length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent + (toPercent - fromPercent) * progress, ec);
> 
> That means if you animate from cm to percentage, the calculated css value will be in percentage for the whole animation? That is different to SMIL animation, where we switch the unit type after 50% of the animation. If this is the way to go in CSS animations/transitions in general I'm fine. Should be discussed in publicFX as well.

Yes, it should be discussed. In most cases we don't specify what should happen.

[Dirk Schulze]

Comment on attachment 104486 [details]
Patch

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

Some snippets and comments.

> LayoutTests/transitions/svg-transitions.html:35
> +    #rect2 {
> +      fill: rgb(0, 0, 255);
> +      stroke: rgb(255, 0, 0);
> +    }
> +    .animating #rect2 {
> +      fill: rgb(0, 255, 0);
> +      stroke: rgb(0, 0, 0);
> +    }

Can you add a test with fill: url(#invalid) green;  to fill: url(#invalid) blue; please? Even if does not pass at the beginning, or at least not animate.

> Source/WebCore/page/animation/AnimationBase.cpp:221
> +static inline SVGLength blendFunc(const AnimationBase*, const SVGLength& from, const SVGLength& to, double progress)

Can you call it blendSVGLength?

> Source/WebCore/svg/SVGLength.h:125
> +        if (!from.isZero() && !isZero() && from.unitType() != unitType())
> +            return *this;

Why don't you allow animation between different unit types? We are doing it with SVGAnimatedLengthAnimator as well. You may need a new private value() function.

> Source/WebCore/svg/SVGLength.h:128
> +        if (from.isZero() && isZero())
> +            return *this;

You need an early return for LengthTypeUnknown as well, no?

> Source/WebCore/svg/SVGLength.h:140
> +        if (resultType == LengthTypePercentage) {
> +            float fromPercent = from.valueAsPercentage();
> +            float toPercent = valueAsPercentage();
> +            length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent + (toPercent - fromPercent) * progress, ec);

Ah ok, animation between different unit types is difficult if one of the values is percentage since you need the viewport. You can still do it with animation between other unit types. But for percentage animation we need access to the SVGElement. I don't know another way to get the information about the viewPort :/

[Dean Jackson]

(In reply to comment #19)

> > General question: Do you respect fallback colors for fill and stroke? fill="url(#invalid) green" this should take a the solid color green.
> 
> I don't do anything here. If the style system resolves to give a Color to fill, then it can be transitioned (as long as the destination is also a Color). So in theory, this should work.

I take this back. It's not so straightforward to handle this case. At the SVGRenderStyle level all we have is a paint type of SVG_PAINTTYPE_URI_RGBCOLOR, but we don't yet know if the URI is invalid.

I'm going to leave this type of paint out of the patch for now. We can add transitions of arbitrary paints later.

[Dean Jackson]

Created attachment 104805 [details]
Patch

[Dean Jackson]

Comment on attachment 104805 [details]
Patch

Removing review?. Forgot to add the test Dirk requested.

[Dean Jackson]

Created attachment 104814 [details]
Patch

[Dean Jackson]

(In reply to comment #20)
> (From update of attachment 104486 [details])

> Can you add a test with fill: url(#invalid) green;  to fill: url(#invalid) blue; please? Even if does not pass at the beginning, or at least not animate.

Done. It currently FAILs, as explained above.

> > Source/WebCore/page/animation/AnimationBase.cpp:221
> > +static inline SVGLength blendFunc(const AnimationBase*, const SVGLength& from, const SVGLength& to, double progress)
> 
> Can you call it blendSVGLength?

The name 'blendFunc' is used for all the typed blending methods with parameter overloading.

> > Source/WebCore/svg/SVGLength.h:125
> > +        if (!from.isZero() && !isZero() && from.unitType() != unitType())
> > +            return *this;
> 
> Why don't you allow animation between different unit types? We are doing it with SVGAnimatedLengthAnimator as well. You may need a new private value() function.

Done. There are some restrictions. I can't currently animate between units that are relative (ems or exes). I can animate between percentages, but not between a percentage and not a percentage. There are also a couple of zero cases.

> 
> > Source/WebCore/svg/SVGLength.h:128
> > +        if (from.isZero() && isZero())
> > +            return *this;
> 
> You need an early return for LengthTypeUnknown as well, no?

Done.

> 
> > Source/WebCore/svg/SVGLength.h:140
> > +        if (resultType == LengthTypePercentage) {
> > +            float fromPercent = from.valueAsPercentage();
> > +            float toPercent = valueAsPercentage();
> > +            length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent + (toPercent - fromPercent) * progress, ec);
> 
> Ah ok, animation between different unit types is difficult if one of the values is percentage since you need the viewport. You can still do it with animation between other unit types. But for percentage animation we need access to the SVGElement. I don't know another way to get the information about the viewPort :/

Right. We suffer the same problems in the non-SVG case, and if you find the bug about animating CSS gradients you see similar issues. I think this is a good start.

[Dean Jackson]

(In reply to comment #25)
> I can animate between percentages, but not between a percentage and not a percentage. 

Err.. not overload, sorry.

% -> %,  YES
0 -> %, YES
% -> 0, YES
everything else -> %, NO
% -> everything else, NO

[Nikolas Zimmermann]

Comment on attachment 104814 [details]
Patch

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

Looks sane, still some comments from my side:

> Source/WebCore/svg/SVGLength.cpp:223
> +        break;

All these breaks can be removed.

> Source/WebCore/svg/SVGLength.cpp:255
> -    ASSERT_NOT_REACHED();
> +    ec = NOT_SUPPORTED_ERR;

This can stay as-is, it's not possible to reach this state.

> Source/WebCore/svg/SVGLength.cpp:271
>          break;

These breaks are superfluous now. you can remove them.

> Source/WebCore/svg/SVGLength.cpp:298
>      }
> +    ec = NOT_SUPPORTED_ERR;
> +    return 0;

Same as above, after removing all breaks; you can also reintroduce ASSERT_NOT_REACHED()  here, as it's not possible to have any other types than those handled in the switch. The compiler would warn about any missing, as there's no default: case.

> Source/WebCore/svg/SVGLength.h:142
> +            length.newValueSpecifiedUnits(LengthTypePercentage, fromPercent + (toPercent - fromPercent) * progress, ec);
> +        } else {

Early return here, instead of adding an else branch.
if (ec)
    return SVGLength();
return length;
}
should be sufficient.

> Source/WebCore/svg/SVGLength.h:150
> +                    length.newValueSpecifiedUnits(toType, fromValue + (toValue - fromValue) * progress, ec);
> +            } else {

Same here, early-exit instead of branching.

> Source/WebCore/svg/SVGLength.h:159
> +                if (!ec) {
> +                    float fromValue = convertValueFromUserUnits(fromValueInUserUnits, toType, 0, ec);
> +                    if (!ec) {
> +                        float toValue = valueInSpecifiedUnits();
> +                        length.newValueSpecifiedUnits(toType, fromValue + (toValue - fromValue) * progress, ec);
> +                    }

turn each of these branches in early returns if (ec) return SVGLength()...

[Dean Jackson]

Created attachment 105100 [details]
Patch

[Dean Jackson]

(In reply to comment #27)

Thanks Nikolas. All your comments addressed.

[Simon Fraser (smfr)]

Comment on attachment 105100 [details]
Patch

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

> LayoutTests/transitions/resources/transition-test-helpers.js:88
> +        if (isCloseEnough(parseInt(computedValue.red.cssText), expectedValue[0], tolerance) &&
> +            isCloseEnough(parseInt(computedValue.green.cssText), expectedValue[1], tolerance) &&
> +            isCloseEnough(parseInt(computedValue.blue.cssText), expectedValue[2], tolerance))

You could put these into a compareRGBColor function.

> LayoutTests/transitions/svg-transitions-expected.txt:12
> +FAIL - "fill" property for "rect7" element at 1s expected: 0,0,127 but saw: #invalid #0000ff

Did you mean to check in a failing result?

> Source/WebCore/page/animation/AnimationBase.cpp:674
> +class PropertyWrapperSVGPaint : public PropertyWrapperBase {

The name is awkward.

[Dean Jackson]

(In reply to comment #30)

> Did you mean to check in a failing result?

Yes. I'll make a comment in the change log.

> 
> > Source/WebCore/page/animation/AnimationBase.cpp:674
> > +class PropertyWrapperSVGPaint : public PropertyWrapperBase {
> 
> The name is awkward.

Yeah, but they all are. PropertyWrapperAcceleratedOpacity, PropertyWrapperAcceleratedTransform, PropertyWrapperMaybeInvalidColor.

Well, nearly all. We're inconsistent: FillLayersPropertyWrapper

[Dean Jackson]

	M	LayoutTests/ChangeLog
	M	LayoutTests/transitions/resources/transition-test-helpers.js
	A	LayoutTests/transitions/svg-transitions-expected.txt
	A	LayoutTests/transitions/svg-transitions.html
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/page/animation/AnimationBase.cpp
	M	Source/WebCore/rendering/style/RenderStyle.h
	M	Source/WebCore/rendering/style/SVGRenderStyle.h
	M	Source/WebCore/svg/SVGLength.cpp
	M	Source/WebCore/svg/SVGLength.h
Committed r93807

http://trac.webkit.org/changeset/93807