Bug 60303

Summary: animation-timing-function property with a list uses first item for all animations
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, joybro201, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Marrin 2011-05-05 14:41:42 PDT
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 Chris Marrin 2011-05-05 14:42:23 PDT
Created attachment 92474 [details]
test case
Comment 2 Young Han Lee 2011-06-18 09:27:35 PDT
> -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 Young Han Lee 2011-06-21 10:19:46 PDT
Created attachment 98005 [details]
Patch
Comment 4 Young Han Lee 2011-06-21 10:26:03 PDT
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 Simon Fraser (smfr) 2011-06-21 10:32:02 PDT
Comment on attachment 98005 [details]
Patch

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 Young Han Lee 2011-06-22 07:28:40 PDT
Created attachment 98168 [details]
Patch
Comment 7 Young Han Lee 2011-06-22 07:30:36 PDT
(In reply to comment #5)
> (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?

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 Simon Fraser (smfr) 2011-06-22 08:39:08 PDT
(In reply to comment #7)
> (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?

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 Simon Fraser (smfr) 2011-06-22 08:40:06 PDT
Comment on attachment 98168 [details]
Patch

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 Young Han Lee 2011-06-22 09:23:56 PDT
Created attachment 98183 [details]
Patch
Comment 11 Young Han Lee 2011-06-22 09:28:05 PDT
(In reply to comment #9)
> (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

applied to the new patch.
Comment 12 Simon Fraser (smfr) 2011-06-22 09:31:39 PDT
Comment on attachment 98183 [details]
Patch

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 Young Han Lee 2011-06-22 09:44:44 PDT
Created attachment 98188 [details]
Patch
Comment 14 Young Han Lee 2011-06-22 09:47:29 PDT
(In reply to comment #12)
> (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.

Oops.. I missed it!
Comment 15 Young Han Lee 2011-06-22 10:16:13 PDT
(In reply to comment #8)
> (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.

This is filed as Bug 63152.

> 
> > Anyway, I changed the iteration count and enabled the pause API in the new patch.
> 
> Cool.
Comment 16 WebKit Review Bot 2011-06-22 12:57:26 PDT
Comment on attachment 98188 [details]
Patch

Clearing flags on attachment: 98188

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