Bug 60303 - animation-timing-function property with a list uses first item for all animations
: animation-timing-function property with a list uses first item for all animat...
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-05-05 14:41 PST by
Modified: 2011-06-22 12:57 PST (History)


Attachments
test case (967 bytes, text/html)
2011-05-05 14:42 PST, Chris Marrin
no flags Details
Patch (6.47 KB, patch)
2011-06-21 10:19 PST, Young Han Lee
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.41 KB, patch)
2011-06-22 07:28 PST, Young Han Lee
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2011-06-22 09:23 PST, Young Han Lee
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.43 KB, patch)
2011-06-22 09:44 PST, Young Han Lee
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-05 14:41:42 PST
The attached test case shows that a animation-timing-function property with multiple values will use the first timing function for all animations in the list. Looks like we are selecting timing-function item 0 when expanding into separate animations.
------- Comment #1 From 2011-05-05 14:42:23 PST -------
Created an attachment (id=92474) [details]
test case
------- Comment #2 From 2011-06-18 09:27:35 PST -------
> -webkit-animation:
>   	bouncer 1s ease infinite alternate,
>   	mover 4s ease infinite alternate;

Hi, Chris

It seems that you made a mistake in the testcase. Shouldn't the two animations have different timing-functions?

Apart from the mistake, the bug you told is also reproduced on my environment.
------- Comment #3 From 2011-06-21 10:19:46 PST -------
Created an attachment (id=98005) [details]
Patch
------- Comment #4 From 2011-06-21 10:26:03 PST -------
KeyframeAnimation::fetchIntervalEndpointsForProperty() is always getting a timingFunction from the first animation, even for the calculation of the second animation.

This patch makes the function look for the correct animation by the keyframe's name.
------- Comment #5 From 2011-06-21 10:32:02 PST -------
(From update of attachment 98005 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=98005&action=review

> LayoutTests/animations/multiple-animations-on-an-element.html:35
> +        runAnimationTest(expectedValues, null, null, true);

Why does this have to disable the pause API?

> Source/WebCore/page/animation/KeyframeAnimation.cpp:153
> +    const Animation* matchedAnimation = getAnimationFromStyleByName(fromStyle, name());
> +    if (matchedAnimation)

We usually write this:
if (const Animation* matchedAnimation = getAnimationFrom...)
    ...
------- Comment #6 From 2011-06-22 07:28:40 PST -------
Created an attachment (id=98168) [details]
Patch
------- Comment #7 From 2011-06-22 07:30:36 PST -------
(In reply to comment #5)
> (From update of attachment 98005 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98005&action=review
> 
> > LayoutTests/animations/multiple-animations-on-an-element.html:35
> > +        runAnimationTest(expectedValues, null, null, true);
> 
> Why does this have to disable the pause API?

Because the test failed when the pause API is enabled, but I just found out that runAnimationTest always fails if the pause API is enabled and the test target animation has "infinite" iteration count.

Is this a bug, too?

Anyway, I changed the iteration count and enabled the pause API in the new patch.

> 
> > Source/WebCore/page/animation/KeyframeAnimation.cpp:153
> > +    const Animation* matchedAnimation = getAnimationFromStyleByName(fromStyle, name());
> > +    if (matchedAnimation)
> 
> We usually write this:
> if (const Animation* matchedAnimation = getAnimationFrom...)
>     ...

This is also changed.

Thanks :)
------- Comment #8 From 2011-06-22 08:39:08 PST -------
(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 98005 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=98005&action=review
> > 
> > > LayoutTests/animations/multiple-animations-on-an-element.html:35
> > > +        runAnimationTest(expectedValues, null, null, true);
> > 
> > Why does this have to disable the pause API?
> 
> Because the test failed when the pause API is enabled, but I just found out that runAnimationTest always fails if the pause API is enabled and the test target animation has "infinite" iteration count.
> 
> Is this a bug, too?

Probably. You should file a bug on it.

> Anyway, I changed the iteration count and enabled the pause API in the new patch.

Cool.
------- Comment #9 From 2011-06-22 08:40:06 PST -------
(From update of attachment 98168 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=98168&action=review

> LayoutTests/ChangeLog:16
> +        * animations/multiple-animations-on-an-element-expected.txt: Added.

You should rename the test to multiple-animations-timing-function.html
------- Comment #10 From 2011-06-22 09:23:56 PST -------
Created an attachment (id=98183) [details]
Patch
------- Comment #11 From 2011-06-22 09:28:05 PST -------
(In reply to comment #9)
> (From update of attachment 98168 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98168&action=review
> 
> > LayoutTests/ChangeLog:16
> > +        * animations/multiple-animations-on-an-element-expected.txt: Added.
> 
> You should rename the test to multiple-animations-timing-function.html

applied to the new patch.
------- Comment #12 From 2011-06-22 09:31:39 PST -------
(From update of attachment 98183 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=98183&action=review

> LayoutTests/ChangeLog:17
> +        * animations/multiple-animations-on-an-element-expected.txt: Added.
> +        * animations/multiple-animations-on-an-element.html: Added.

Still out of date.

> Source/WebCore/ChangeLog:16
> +        Test: animations/multiple-animations-on-an-element.html

Out of date.
------- Comment #13 From 2011-06-22 09:44:44 PST -------
Created an attachment (id=98188) [details]
Patch
------- Comment #14 From 2011-06-22 09:47:29 PST -------
(In reply to comment #12)
> (From update of attachment 98183 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98183&action=review
> 
> > LayoutTests/ChangeLog:17
> > +        * animations/multiple-animations-on-an-element-expected.txt: Added.
> > +        * animations/multiple-animations-on-an-element.html: Added.
> 
> Still out of date.
> 
> > Source/WebCore/ChangeLog:16
> > +        Test: animations/multiple-animations-on-an-element.html
> 
> Out of date.

Oops.. I missed it!
------- Comment #15 From 2011-06-22 10:16:13 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 98005 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=98005&action=review
> > > 
> > > > LayoutTests/animations/multiple-animations-on-an-element.html:35
> > > > +        runAnimationTest(expectedValues, null, null, true);
> > > 
> > > Why does this have to disable the pause API?
> > 
> > Because the test failed when the pause API is enabled, but I just found out that runAnimationTest always fails if the pause API is enabled and the test target animation has "infinite" iteration count.
> > 
> > Is this a bug, too?
> 
> Probably. You should file a bug on it.

This is filed as Bug 63152.

> 
> > Anyway, I changed the iteration count and enabled the pause API in the new patch.
> 
> Cool.
------- Comment #16 From 2011-06-22 12:57:26 PST -------
(From update of attachment 98188 [details])
Clearing flags on attachment: 98188

Committed r89462: <http://trac.webkit.org/changeset/89462>
------- Comment #17 From 2011-06-22 12:57:31 PST -------
All reviewed patches have been landed.  Closing bug.