REGRESSION (r263506): scale transform transitions won't overshoot

• Component: Animations
• Status: RESOLVED FIXED

[Tim Guan-tin Chien [:timdream]]

Created attachment 407234 [details]
scale-test.html

STR:

See test case, a <div> comes with `transition: 1s transform cubic-bezier(0, 2, 1, 2)` set.

Expected:

The bezier curve should get the div to scale up a bit and does down to the final size during the CSS transition.

Actual:

The transition looks linear and disregard the curve set?

Note:

Firefox and Chrome goes with the expected behavior.

Version:

Release 110 (Safari 14.0, WebKit 15610.1.21.0.2)
macOS 10.15.5 (19F101)

[Radar WebKit Bug Importer]

<rdar://problem/67759310>

[Antoine Quint]

This regressed with r263506, the fix for bug 213495.

[Antoine Quint]

Strange, we are setting the correct animation-wide timing function in GraphicsLayerCA::setupAnimation() and then returning the correct, default linear timing function for the keyframe in GraphicsLayerCA::timingFunctionForAnimationValue().

[Antoine Quint]

Applying the timing function to the keyframe instead of the CA animation fixes the issue. This could be a Core Animation bug or an unexpected behavior of the API.

[Antoine Quint]

I confirmed this is an issue with Core Animation: setting the `timingFunction` property on a CAKeyframeAnimation clips the animation when the timing function is outside of the 0-1 range. However, applying the same timing function to the `timingFunctions` (plural) property addresses the issue.

This means that for CSS Animations this is not an issue since CSS Animations never apply an animation-wide timing function.

For CSS Transitions, we can work around this issue since there is a single interval.

However, for JS-originated animations which can have both multiple intervals (more than 2 keyframes) and an animation-wide timing function, we will require a fix to Core Animation.

Applying a workaround to fix CSS Transitions seems well worth it since:

1. this is a regression
2. the reported bug is about those specifically
3. they are more common than JS-originated animations at this stage (which never worked for this behavior)

[Antoine Quint]

My plan is to add a new member to PlatformCAAnimation to return the number of values for an animation, which we'll use to determine whether we have an animation with 2 values or more to determine whether we should use the `timingFunctions` (plural) property rather than the `timingFunction` (singular) property.

[Antoine Quint]

Actually, it'll be even easier to use Animation::property() so that GraphicsLayerCA can tell we're dealing with a CSS Transition.

[Antoine Quint]

Created attachment 407461 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 407461 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1680
> +        // If we are dealing with a CSS Transition and using a cubic bezier timing function, we set up
> +        // the Animation object to reflect this which will allow the GraphicsLayerCA code to set the
> +        // timing function on the single keyframe interval rather than on the animation at large,
> +        // working around a Core Animation issue where setting a cubic bezier on a CAKeyframeAnimation's
> +        // timingFunction property can lead to clipped animations should a value be above 1.

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3341
> +        // A CSS Transition is the only scenario where Animation::property() will have
> +        // its mode set to SingleProperty. In this case, we don't set the animation-wide
> +        // timing function, instead opting to set it using the timingFunctions (plural)
> +        // property to work around a Core Animation issue where setting a cubic bezier
> +        // on a CAKeyframeAnimation's timingFunction property (singular) can lead to
> +        // clipped animations should a value be above 1.
> +        // FIXME: https://bugs.webkit.org/show_bug.cgi?id=215918

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3357
> +            // A CSS Transition is the only scenario where Animation::property() will have
> +            // its mode set to SingleProperty. In this case, we chose not to set set the
> +            // animation-wide timing function, so we set it on the single keyframe interval
> +            // to work around a Core Animation issue where setting a cubic bezier on a
> +            // CAKeyframeAnimation's timingFunction property (singular) can lead to clipped
> +            // animations should a value be above 1.
> +            // FIXME: https://bugs.webkit.org/show_bug.cgi?id=215918

I think this comment could be reduced to:
// Do blah to work around a limitation of Core Animation: webkit.org/b/215918

> LayoutTests/webanimations/accelerated-css-transition-with-easing-y-axis-above-1.html:40
> +    if (!UIHelper.isWebKit2())
> +        await new Promise(requestAnimationFrame);

A bit weird. Can you just await UIHelper.renderingUpdate()?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 407461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407461&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:1680
> > +        // If we are dealing with a CSS Transition and using a cubic bezier timing function, we set up
> > +        // the Animation object to reflect this which will allow the GraphicsLayerCA code to set the
> > +        // timing function on the single keyframe interval rather than on the animation at large,
> > +        // working around a Core Animation issue where setting a cubic bezier on a CAKeyframeAnimation's
> > +        // timingFunction property can lead to clipped animations should a value be above 1.
> 
> I think this comment could be reduced to:
> // Do blah to work around a limitation of Core Animation: webkit.org/b/215918

I'll try to tone those down to only capture the bits relevant to the immediate code.

> > LayoutTests/webanimations/accelerated-css-transition-with-easing-y-axis-above-1.html:40
> > +    if (!UIHelper.isWebKit2())
> > +        await new Promise(requestAnimationFrame);
> 
> A bit weird. Can you just await UIHelper.renderingUpdate()?

Will do.

[Antoine Quint]

Committed r266280: <https://trac.webkit.org/changeset/266280>

[Antoine Quint]

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