Bug 210526

Summary: WebAnimations API doesn't properly apply keyframe easings to transforms
Product: WebKit Reporter: Alan Orozco <alanorozco>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, graouts, graouts, koivisto, simon.fraser, travis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=213495
https://bugs.webkit.org/show_bug.cgi?id=215853
Attachments:
Description Flags
Safari (incorrect): Left box is animated with `top`. Right box is animated using `transform`. Both have the same keyframe easings.
none
(ignore this attachment, it was added accidentally)
none
Chrome (correct): Left box is animated with `top`. Right box is animated using `transform`. Both have the same keyframe easings.
none
Test showing CSS and Web Animations for top and transform
none
Patch koivisto: review+

Description Alan Orozco 2020-04-14 17:45:28 PDT
Created attachment 396484 [details]
Safari (incorrect): Left box is animated with `top`. Right box is animated using `transform`. Both have the same keyframe easings.

Overview:

Animation effect definitions may include easing properties in each keyframe, like so:

  element.animate(
    [
      {
        top: "0px",
        easing: "step-start"
      },
      {
        top: "100px",
        easing: "step-start"
      },
      {
        top: "0px",
      }
    ],
    1000
  );

These should define the timing of interpolations between frames, but the
property is ignored when the keyframes include transforms.

Steps to reproduce:

Define an animation effect using a) transforms and b) easings per-keyframe.
An example is hosted here: https://webkit-animation-easing.glitch.me/easing.html

  element.animate(
    [
      {
        transform: "translateY(0px)",
        easing: "step-start"
      },
      {
        transform: "translateY(100px)",
        easing: "step-start"
      },
      {
        transform: "translateY(0px)",
      }
    ],
    1000
  );

Actual Results:

See in example URL that keyframe easings are not respected for the element
whose `transform` is animated.

Expected results:

Animation effect should respect easings, like it does for the element whose
`top` is animated.

Platform:

Safari Version 13.1 (15609.1.20.111.8)
macOS 10.15.4 (19E266)

Additional Information:

Attached video showing actual result.
Comment 1 Alan Orozco 2020-04-14 17:46:04 PDT
Created attachment 396485 [details]
(ignore this attachment, it was added accidentally)
Comment 2 Radar WebKit Bug Importer 2020-04-14 18:10:50 PDT
<rdar://problem/61800424>
Comment 3 Alan Orozco 2020-04-14 18:57:30 PDT
Created attachment 396488 [details]
Chrome (correct): Left box is animated with `top`. Right box is animated using `transform`. Both have the same keyframe easings.
Comment 4 Antoine Quint 2020-04-15 08:01:24 PDT
We don't support the "steps" family of timing functions for accelerated animations. We should opt out of the accelerated path in that case.
Comment 5 Antoine Quint 2020-04-15 08:20:50 PDT
Created attachment 396538 [details]
Test showing CSS and Web Animations for top and transform

Attaching a new test case that shows how CSS Animations behave correctly but not a JS-originated animation.
Comment 6 Antoine Quint 2020-04-15 08:23:48 PDT
I think we're not setting the per-keyframe timing functions in KeyframeEffect::updateBlendingKeyframes().
Comment 7 Antoine Quint 2020-04-20 04:16:39 PDT
Created attachment 396965 [details]
Patch
Comment 8 Antoine Quint 2020-04-20 04:57:03 PDT
Committed r260360: <https://trac.webkit.org/changeset/260360>
Comment 9 Antti Koivisto 2020-04-20 04:58:03 PDT
Comment on attachment 396965 [details]
Patch

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

> LayoutTests/webanimations/transform-animation-with-steps-timing-function-not-accelerated.html:23
> +    animation.ready.then(() => {
> +        // We wait for two frames to ensure an accelerated animation would have been committed.
> +        requestAnimationFrame(() => {
> +            requestAnimationFrame(() => {
> +                assert_equals(internals.acceleratedAnimationsForElement(target).length, 0, "The animation's target has no accelerated animation.");
> +                t.done();
> +            });
> +        });
> +    });

This would read better with await
Comment 10 Antoine Quint 2020-06-05 01:31:11 PDT
*** Bug 212686 has been marked as a duplicate of this bug. ***
Comment 11 Antoine Quint 2020-06-23 11:45:25 PDT
This caused bug 213495.