Bug 213495 - REGRESSION (r260360): easing curves are broken on JS-originated animations
Summary: REGRESSION (r260360): easing curves are broken on JS-originated animations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 213504 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-06-22 16:40 PDT by Liam DeBeasi
Modified: 2020-08-27 07:58 PDT (History)
9 users (show)

See Also:


Attachments
Code Reproduction (509 bytes, text/html)
2020-06-22 16:40 PDT, Liam DeBeasi
no flags Details
The Actual Code Reproduction (620 bytes, text/html)
2020-06-22 16:43 PDT, Liam DeBeasi
no flags Details
Patch (12.66 KB, patch)
2020-06-24 06:34 PDT, Antoine Quint
darin: review+
Details | Formatted Diff | Diff
Comparison Safari, Technology Preview, Firefox (8.14 MB, video/quicktime)
2020-08-05 07:25 PDT, Niklas Merz
no flags Details
Slower Transitions on iOS 14 (934.50 KB, video/mp4)
2020-08-05 07:29 PDT, Niklas Merz
no flags Details
Working Transitions iOS 13 (513.60 KB, video/mp4)
2020-08-05 07:30 PDT, Niklas Merz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Liam DeBeasi 2020-06-22 16:40:20 PDT
Created attachment 402521 [details]
Code Reproduction

Easing curves are completely broken in WebKit on iOS 14 Beta 1 and Safari Tech Preview 108.

Steps to reproduce:

1. Open attached reproduction. This code animates a block with "ease" set for easing.
2. Notice that the animation runs with a linear curve.

I would expect that the square is animated with the easing curve that I specified.

I can only reproduce this with Web Animations. I cannot reproduce this with CSS Animations.
Comment 1 Liam DeBeasi 2020-06-22 16:43:35 PDT
Created attachment 402522 [details]
The Actual Code Reproduction

Sorry, I uploaded the wrong file. Please check "The Actual Code Reproduction" for a reproduction of the issue :-). The old file I uploaded shows the code working properly with CSS Animations.
Comment 2 Niklas Merz 2020-06-23 06:47:40 PDT
*** Bug 213504 has been marked as a duplicate of this bug. ***
Comment 3 Radar WebKit Bug Importer 2020-06-23 10:11:21 PDT
<rdar://problem/64649747>
Comment 4 Antoine Quint 2020-06-23 11:28:07 PDT
This is specific to accelerated animations, animating margin-left instead has the intended effect.
Comment 5 Antoine Quint 2020-06-23 11:40:46 PDT
This most likely regressed with https://trac.webkit.org/changeset/260360/.
Comment 6 Antoine Quint 2020-06-23 12:22:57 PDT
Interesting, this makes it look like setting timing functions on individual keyframes overrides the CAAnimation-wide timing function.
Comment 7 Antoine Quint 2020-06-23 12:34:25 PDT
It doesn't look like we ever set the CAAnimation timingFunction but rather set timing function on the keyframes directly, at least this is what GraphicsLayerCA::timingFunctionForAnimationValue() looks like it's doing. This isn't surprising since until Web Animations there was no notion of an animation-wide timing function in CSS Animations and CSS Transitions. We'll need to add pluming to pass the animation-wide timing function to the CAAnimation.
Comment 8 Simon Fraser (smfr) 2020-06-23 14:37:48 PDT
How did this not break tests? I know we have tests for easing functions on accelerated animations.
Comment 9 Antoine Quint 2020-06-23 15:15:18 PDT
Because we have relatively few tests for the few features, such as this one, that only exist in Web Animations in the accelerated case.
Comment 10 Antoine Quint 2020-06-24 06:34:13 PDT
Created attachment 402647 [details]
Patch
Comment 11 Darin Adler 2020-06-24 11:03:24 PDT
Comment on attachment 402647 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3082
> +static bool isKeyframe(const KeyframeValueList& valueList)
> +{
> +    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
> +        return valueList.size() > 1;
> +    return valueList.size() > 2;
> +}

In such a short function, I like the idea of an even shorter local variable name like "list".
Comment 12 Antoine Quint 2020-06-24 11:37:51 PDT
Committed r263466: <https://trac.webkit.org/changeset/263466>
Comment 13 Antoine Quint 2020-06-24 11:38:32 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 402647 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402647&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3082
> > +static bool isKeyframe(const KeyframeValueList& valueList)
> > +{
> > +    if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled())
> > +        return valueList.size() > 1;
> > +    return valueList.size() > 2;
> > +}
> 
> In such a short function, I like the idea of an even shorter local variable
> name like "list".

I renamed `valueList` as `list` in the commit.
Comment 14 Jason Lawrence 2020-06-24 16:04:50 PDT
Reverted r263466 for reason:

This commit caused 50+ crashes on multiple queues internally.

Committed r263481: <https://trac.webkit.org/changeset/263481>
Comment 15 Antoine Quint 2020-06-25 06:42:11 PDT
Committed r263506: <https://trac.webkit.org/changeset/263506>
Comment 16 Niklas Merz 2020-08-05 07:25:56 PDT
Created attachment 405997 [details]
Comparison Safari, Technology Preview, Firefox

Is this really fixed or not yet deployed? I still see a difference in Safari Technology Preview and Safari 13. Esspecially page transitions which are using these animations are not good in our app.

The video compares the attached example from Liam. I tried to get the animation somewhat in sync. Top right is Safari Technolog Preview and it appears slower. Top left is Firefox and bottom right current Safari.

It's is really visible and bad for the user experience if you look at the animations the Ionic framework built for page transitions. Sample app: https://ionic-repro.web.app
Comment 17 Niklas Merz 2020-08-05 07:29:26 PDT
Created attachment 405998 [details]
Slower Transitions on iOS 14
Comment 18 Niklas Merz 2020-08-05 07:30:01 PDT
Created attachment 405999 [details]
Working Transitions iOS 13
Comment 19 Simon Fraser (smfr) 2020-08-05 10:27:25 PDT
This change should be in STP 110.
Comment 20 Liam DeBeasi 2020-08-05 11:33:36 PDT
The issue is not resolved in iOS 14 beta 4, but I can confirm that it is resolved in Safari Tech Preview (tested in STP 111).