Bug 60303 – animation-timing-function property with a list uses first item for all animations

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

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.

[Chris Marrin]

Created attachment 92474 [details]
test case

[Young Han Lee]

> -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.

[Young Han Lee]

Created attachment 98005 [details]
Patch

[Young Han Lee]

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.

[Simon Fraser (smfr)]

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...)
    ...

[Young Han Lee]

Created attachment 98168 [details]
Patch

[Young Han Lee]

(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 :)

[Simon Fraser (smfr)]

(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.

[Simon Fraser (smfr)]

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

[Young Han Lee]

Created attachment 98183 [details]
Patch

[Young Han Lee]

(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.

[Simon Fraser (smfr)]

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.

[Young Han Lee]

Created attachment 98188 [details]
Patch

[Young Han Lee]

(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!

[Young Han Lee]

(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.

[WebKit Review Bot]

Comment on attachment 98188 [details]
Patch

Clearing flags on attachment: 98188

Committed r89462: <http://trac.webkit.org/changeset/89462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.