Bug 215807 – REGRESSION (r263506): timing of CSS Animation on https://animate.style is incorrect

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

If you go to animate.style you'll notice the animations are playing faster than they do in other browsers (Chrome and Firefox for instance).

[Antoine Quint]

<rdar://problem/66770136>

[Antoine Quint]

Created attachment 407189 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 407189 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        In r263506, we added a way for accelerated animations to adhere to both a timing function set on the
> +        animation, affecting the timing of the entire animation, as well as a timing function set on individual
> +        keyframes, affecting the timing of the animation in a given interval.

That's a weird way of saying that accelerated animation timing functions were broken.

> Source/WebCore/animation/KeyframeEffect.cpp:1662
>  Ref<const Animation> KeyframeEffect::backingAnimationForCompositedRenderer() const

It's weird that this has to exist, and weird that it returns a Ref<const Animation>.

> LayoutTests/webanimations/accelerated-css-animation-with-easing.html:54
> +    await new Promise(requestAnimationFrame);
> +    await new Promise(requestAnimationFrame);
> +    await new Promise(requestAnimationFrame);

I feel like this test will be flakey.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 407189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407189&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        In r263506, we added a way for accelerated animations to adhere to both a timing function set on the
> > +        animation, affecting the timing of the entire animation, as well as a timing function set on individual
> > +        keyframes, affecting the timing of the animation in a given interval.
> 
> That's a weird way of saying that accelerated animation timing functions
> were broken.

Not *all* of them, it was required to have more than 2 keyframes and use implicit timing functions on keyframes.

> > Source/WebCore/animation/KeyframeEffect.cpp:1662
> >  Ref<const Animation> KeyframeEffect::backingAnimationForCompositedRenderer() const
> 
> It's weird that this has to exist, and weird that it returns a Ref<const
> Animation>.

Agreed. When we remove the legacy animation code (real soon now) we can remove the use of Animation here and use WebAnimation objects instead.

> > LayoutTests/webanimations/accelerated-css-animation-with-easing.html:54
> > +    await new Promise(requestAnimationFrame);
> > +    await new Promise(requestAnimationFrame);
> > +    await new Promise(requestAnimationFrame);
> 
> I feel like this test will be flakey.

This was based on webanimations/accelerated-animation-with-easing.html which proved to be solid after some initial issues were ironed out.

[Antoine Quint]

Created attachment 407388 [details]
Patch for EWS

[Antoine Quint]

Created attachment 407393 [details]
Patch

[Antoine Quint]

Committed r266241: <https://trac.webkit.org/changeset/266241>

[Antoine Quint]

*** Bug 218224 has been marked as a duplicate of this bug. ***