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.
Created attachment 208136 [details] Patch
<rdar://problem/14610309>
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.
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.
Created attachment 208139 [details] Patch
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)
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 :)
(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.
(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.
Created attachment 208192 [details] Patch
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
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.
(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.
Committed r153757: <http://trac.webkit.org/changeset/153757>
(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>.
(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!