Bug 118574 – SVG stroke-dasharray is not animatable

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

[Lea Verou]

Even though WebKit allows many other SVG properties to be animatable through CSS transitions and animations, stroke-dasharray seems to be the odd one out. All the other stroke properties are animatable.

Firefox animates it.

IE10 doesn’t support CSS transitions & animations in any SVG property it seems.

Issue present in both WebKit nightlies and Blink too.

[Rob Buis]

Created attachment 208136 [details]
Patch

[Tim Horton]

<rdar://problem/14610309>

[Dirk Schulze]

Comment on attachment 208136 [details]
Patch

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

Looks great in general. Some notes inside.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> +    if (from.size() != to.size())
> +        return to;

Do we really just want to animate lists of same length? I wonder if it would make sense to 0 the other values.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:238
> +    Vector<SVGLength> ret(len);

please use a more meaningful name. result?

> LayoutTests/transitions/svg-transitions.html:18
> +      stroke-dasharray: 10 10;

more tests would be great. Especially with different values, length.

[Dean Jackson]

Comment on attachment 208136 [details]
Patch

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

>> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
>> +        return to;
> 
> Do we really just want to animate lists of same length? I wonder if it would make sense to 0 the other values.

I think it would make more sense to repeat values e.g.
5 10 -> 10 20 15 25 would be the same as 5 10 5 10 -> 10 20 15 25
and 5 10 15 20 -> 10 20 would animate as 5 10 15 20 -> 10 20 10 20

However, giving that makes the intermediate getComputedStyle() values a bit confusing, I think it's ok to also say that we don't animate lists of different lengths.

[Rob Buis]

Created attachment 208139 [details]
Patch

[Tim Horton]

Comment on attachment 208139 [details]
Patch

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

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:55
> +using namespace std;

Please use the namespace explicitly instead of doing this. (http://www.webkit.org/coding/coding-style.html#using-in-cpp)

[Rob Buis]

Hi Dean,

(In reply to comment #4)
> (From update of attachment 208136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208136&action=review
> 
> >> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> >> +        return to;
> > 
> > Do we really just want to animate lists of same length? I wonder if it would make sense to 0 the other values.
> 
> I think it would make more sense to repeat values e.g.
> 5 10 -> 10 20 15 25 would be the same as 5 10 5 10 -> 10 20 15 25
> and 5 10 15 20 -> 10 20 would animate as 5 10 15 20 -> 10 20 10 20

Right, the current FireFox implementation seems to do that.

> However, giving that makes the intermediate getComputedStyle() values a bit confusing, I think it's ok to also say that we don't animate lists of different lengths.

I am not in favour of either solution, but for completeness (and because FF supports it) I just uploaded a patch that does the repeating. Worst case we can just revert to my initial patch and fix Dirks's remaining points(variable rename and more tests). Let me know :)

[Rob Buis]

(In reply to comment #6)
> (From update of attachment 208139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208139&action=review
> 
> > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:55
> > +using namespace std;
> 
> Please use the namespace explicitly instead of doing this. (http://www.webkit.org/coding/coding-style.html#using-in-cpp)

Strange, I had that earlier but I am pretty sure it gave me a style error at the time :( Will fix.

[Dirk Schulze]

(In reply to comment #7)
> Hi Dean,
> 
> (In reply to comment #4)
> > (From update of attachment 208136 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208136&action=review
> > 
> > >> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> > >> +        return to;
> > > 
> > > Do we really just want to animate lists of same length? I wonder if it would make sense to 0 the other values.
> > 
> > I think it would make more sense to repeat values e.g.
> > 5 10 -> 10 20 15 25 would be the same as 5 10 5 10 -> 10 20 15 25
> > and 5 10 15 20 -> 10 20 would animate as 5 10 15 20 -> 10 20 10 20
> 
> Right, the current FireFox implementation seems to do that.
> 
> > However, giving that makes the intermediate getComputedStyle() values a bit confusing, I think it's ok to also say that we don't animate lists of different lengths.
> 
> I am not in favour of either solution, but for completeness (and because FF supports it) I just uploaded a patch that does the repeating. Worst case we can just revert to my initial patch and fix Dirks's remaining points(variable rename and more tests). Let me know :)

I don't feel strongly about any solution, we can skip if we do not have the same pattern for now. But please add more tests that cover the cases. Compatibility with FF doesn't seem too bad though.

[Rob Buis]

Created attachment 208192 [details]
Patch

[Dirk Schulze]

Comment on attachment 208192 [details]
Patch

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

r=me with the changes.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> +    size_t fromLen = from.size();
> +    size_t toLen = to.size();

fromLength and toLength

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:241
> +    size_t len = fromLen;

resultLength

[Mark Lam]

This change broke the Windows build:
http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/50704/steps/compile-webkit/logs/stdio

     1>..\page\animation\CSSPropertyAnimation.cpp(243): error C3861: 'remainder': identifier not found

Please fix.

[Mark Lam]

(In reply to comment #12)
> This change broke the Windows build:
> http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/50704/steps/compile-webkit/logs/stdio
> 
>      1>..\page\animation\CSSPropertyAnimation.cpp(243): error C3861: 'remainder': identifier not found
> 
> Please fix.

FYI, I'm referring to http://trac.webkit.org/changeset/153754 which references this bug.

[Rob Buis]

Committed r153757: <http://trac.webkit.org/changeset/153757>

[Mark Lam]

(In reply to comment #14)
> Committed r153757: <http://trac.webkit.org/changeset/153757>

FYI, the Windows build failure was fixed in <http://trac.webkit.org/changeset/153764>.

[Rob Buis]

(In reply to comment #15)
> (In reply to comment #14)
> > Committed r153757: <http://trac.webkit.org/changeset/153757>
> 
> FYI, the Windows build failure was fixed in <http://trac.webkit.org/changeset/153764>.

I saw, thanks!