[Bug] Calling play() then cancel() on an animation has different results in chrome and in safari #102

xaviergonz · 2022-06-28T16:14:31Z

1. Describe the bug

If an animation calls play() and then, at a later time, calls cancel() it has different results in Safari and Chrome:

In chrome it will reset the animation to the original frame
In safari it will reset the animation to the original frame for a single frame, then apply the last played frame styles.

Note that it doesn't matter where play() is called (might even be at the very beginning of the animation). However, if play() is not called then them both behave like in Chrome.

Also note that setting allowWebkitAcceleration to true seems to have no effect.

2. IMPORTANT: Provide a CodeSandbox reproduction of the bug

https://codesandbox.io/s/agitated-shtern-5ktj6v?file=/src/App2.tsx

3. Steps to reproduce

Open the sandbox with Chrome and Safari and see how in Chrome the box ends unrotated and in safari it ends rotated.

4. Expected behavior

I expect them both to behave in the same way.

6. Browser details

Latest Chrome and Safari in OSX 12.4

xaviergonz · 2022-06-28T19:14:08Z

I just checked using the web animation API in safari and in this case it works ok, so it must be a bug with the polyfill.
What I don't understand is how it still fails then when using allowWebkitAcceleration: true, I thought doing that the polyfill was not used?

mattgperry · 2022-06-28T19:19:11Z

The polyfill is only there to animate individual transforms outside of Chrome, so it’s never used in other cases. allowWebkitAccelerarion is for running WAAPI on the cpu vs the gpu in Safari for all other values

xaviergonz · 2022-06-28T19:21:02Z

A total wild guess, but maybe it is because this line

motionone/packages/animation/src/Animation.ts

Line 169 in d6306f3

requestAnimationFrame(this.tick)

should assign the returned value to this.frameRequestId?

or maybe it should even clear this.frameRequestId every time that it is not enqueued (at the beginning of this.tick), set it to undefined when it is cleared (in cancel) and not re-enqueue it if it is already enqueued (in play)

xaviergonz · 2022-06-28T19:23:56Z

Since I can't make PRs due to fork restrictions... (untested)

import type {
  AnimationControls,
  AnimationOptions,
  Easing,
} from "@motionone/types"
import {
  isEasingGenerator,
  isEasingList,
  defaults,
  noopReturn,
} from "@motionone/utils"
import { getEasingFunction } from "./utils/easing"
import { interpolate as createInterpolate } from "./utils/interpolate"

export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
  private resolve?: (value: any) => void

  private reject?: (value: any) => void

  startTime: number | null = null

  private pauseTime: number | undefined

  private rate = 1

  private tick: (t: number) => void

  private t = 0

  private cancelTimestamp: number | null = null

  private frameRequestId?: number

  playState: AnimationPlayState = "idle"

  constructor(
    output: (v: number) => void,
    keyframes: number[] = [0, 1],
    {
      easing = defaults.easing as Easing,
      duration = defaults.duration,
      delay = defaults.delay,
      endDelay = defaults.endDelay,
      repeat = defaults.repeat,
      offset,
      direction = "normal",
    }: AnimationOptions = {}
  ) {
    if (isEasingGenerator(easing)) {
      const custom = easing.createAnimation(keyframes, () => "0", true)
      easing = custom.easing
      if (custom.keyframes !== undefined) keyframes = custom.keyframes
      if (custom.duration !== undefined) duration = custom.duration
    }

    const animationEasing = isEasingList(easing)
      ? noopReturn
      : getEasingFunction(easing)

    const totalDuration = duration * (repeat + 1)

    const interpolate = createInterpolate(
      keyframes,
      offset,
      isEasingList(easing) ? easing.map(getEasingFunction) : noopReturn
    )

    this.tick = (timestamp: number) => {
      this.frameRequestId = undefined;

      // TODO: Temporary fix for OptionsResolver typing
      delay = delay as number

      if (this.pauseTime) timestamp = this.pauseTime

      let t = (timestamp - this.startTime!) * this.rate

      this.t = t

      // Convert to seconds
      t /= 1000

      // Rebase on delay
      t = Math.max(t - delay, 0)

      /**
       * If this animation has finished, set the current time
       * to the total duration.
       */
      if (this.playState === "finished") t = totalDuration

      /**
       * Get the current progress (0-1) of the animation. If t is >
       * than duration we'll get values like 2.5 (midway through the
       * third iteration)
       */
      const progress = t / duration

      // TODO progress += iterationStart

      /**
       * Get the current iteration (0 indexed). For instance the floor of
       * 2.5 is 2.
       */
      let currentIteration = Math.floor(progress)

      /**
       * Get the current progress of the iteration by taking the remainder
       * so 2.5 is 0.5 through iteration 2
       */
      let iterationProgress = progress % 1.0

      if (!iterationProgress && progress >= 1) {
        iterationProgress = 1
      }

      /**
       * If iteration progress is 1 we count that as the end
       * of the previous iteration.
       */
      iterationProgress === 1 && currentIteration--

      /**
       * Reverse progress if we're not running in "normal" direction
       */
      const iterationIsOdd = currentIteration % 2
      if (
        direction === "reverse" ||
        (direction === "alternate" && iterationIsOdd) ||
        (direction === "alternate-reverse" && !iterationIsOdd)
      ) {
        iterationProgress = 1 - iterationProgress
      }

      const p = t >= totalDuration ? 1 : Math.min(iterationProgress, 1)
      const latest = interpolate(animationEasing(p))

      output(latest)

      const isAnimationFinished =
        this.playState === "finished" || t >= totalDuration + endDelay

      if (isAnimationFinished) {
        this.playState = "finished"
        this.resolve?.(latest)
      } else if (this.playState !== "idle") {
        this.frameRequestId = requestAnimationFrame(this.tick)
      }
    }

    this.play()
  }

  finished = new Promise((resolve, reject) => {
    this.resolve = resolve
    this.reject = reject
  })

  play() {
    const now = performance.now()
    this.playState = "running"

    if (this.pauseTime) {
      this.startTime = now - (this.pauseTime - (this.startTime ?? 0))
    } else if (!this.startTime) {
      this.startTime = now
    }

    this.cancelTimestamp = this.startTime
    this.pauseTime = undefined
    if (this.frameRequestId === undefined) {
      this.frameRequestId = requestAnimationFrame(this.tick)
    }
  }

  pause() {
    this.playState = "paused"
    this.pauseTime = performance.now()
  }

  finish() {
    this.playState = "finished"
    this.tick(0)
  }

  stop() {
    this.playState = "idle"

    if (this.frameRequestId !== undefined) {
      cancelAnimationFrame(this.frameRequestId)
      this.frameRequestId = undefined;
    }

    this.reject?.(false)
  }

  cancel() {
    this.stop()
    this.tick(this.cancelTimestamp!)
  }

  reverse() {
    this.rate *= -1
  }

  commitStyles() {}

  get currentTime() {
    return this.t
  }

  set currentTime(t: number) {
    if (this.pauseTime || this.rate === 0) {
      this.pauseTime = t
    } else {
      this.startTime = performance.now() - t / this.rate
    }
  }

  get playbackRate() {
    return this.rate
  }

  set playbackRate(rate) {
    this.rate = rate
  }
}

xaviergonz · 2022-06-29T06:52:20Z

@mattgperry I can confirm the fix above works

xaviergonz · 2022-07-05T17:33:49Z

Basically what I did was to ensure that requestAnimationFrame is not called again when it is already scheduled to run so it doesn't double run.

mattgperry · 2022-07-09T19:21:15Z

Thanks for the fix! It'll go out in 10.12.0 hopefully in the next few days.

xaviergonz · 2022-07-10T11:17:29Z

I see that you applied the missing assignation, but not setting it to undefined when it is cleared and when the tick begins. is that on purpose or because you didn't notice?

mattgperry · 2022-07-10T12:08:44Z

Ah yeah this was on purpose, all the ids are unique so once it’s cleared it doesn’t really matter if we don’t delete it’s reference

xaviergonz · 2022-07-10T12:12:30Z

I think you could still queue two ticks if you call play() twice in a row if the requestAnimationFrame is not protected by a check through an if though (and that'd make the first tick un-cancelable since the id would be lost)

xaviergonz · 2022-07-10T12:13:59Z

Here's the code adapted to the latest version of the file

import type {
  AnimationControls,
  AnimationOptions,
  EasingFunction,
} from "@motionone/types";
import {
  isEasingGenerator,
  isEasingList,
  defaults,
  noopReturn,
  interpolate as createInterpolate,
} from "@motionone/utils";
import { getEasingFunction } from "./utils/easing";

export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
  private resolve?: (value: any) => void;

  private reject?: (value: any) => void;

  startTime: number | null = null;

  private pauseTime: number | undefined;

  private rate = 1;

  private tick: (t: number) => void;

  private t = 0;

  private cancelTimestamp: number | null = null;

  private frameRequestId?: number;

  private easing: EasingFunction = noopReturn;

  private duration: number = 0;

  private totalDuration: number = 0;

  private repeat: number = 0;

  playState: AnimationPlayState = "idle";

  constructor(
    output: (v: number) => void,
    keyframes: number[] = [0, 1],
    {
      easing,
      duration: initialDuration = defaults.duration,
      delay = defaults.delay,
      endDelay = defaults.endDelay,
      repeat = defaults.repeat,
      offset,
      direction = "normal",
    }: AnimationOptions = {}
  ) {
    easing = easing || defaults.easing;

    if (isEasingGenerator(easing)) {
      const custom = easing.createAnimation(keyframes, () => "0", true);
      easing = custom.easing;
      if (custom.keyframes !== undefined) keyframes = custom.keyframes;
      if (custom.duration !== undefined) initialDuration = custom.duration;
    }

    this.repeat = repeat;

    this.easing = isEasingList(easing) ? noopReturn : getEasingFunction(easing);

    this.updateDuration(initialDuration);

    const interpolate = createInterpolate(
      keyframes,
      offset,
      isEasingList(easing) ? easing.map(getEasingFunction) : noopReturn
    );

    this.tick = (timestamp: number) => {
      this.frameRequestId = undefined;

      // TODO: Temporary fix for OptionsResolver typing
      delay = delay as number;

      let t = 0;
      if (this.pauseTime !== undefined) {
        t = this.pauseTime;
      } else {
        t = (timestamp - this.startTime!) * this.rate;
      }

      this.t = t;

      // Convert to seconds
      t /= 1000;

      // Rebase on delay
      t = Math.max(t - delay, 0);

      /**
       * If this animation has finished, set the current time
       * to the total duration.
       */
      if (this.playState === "finished" && this.pauseTime === undefined) {
        t = this.totalDuration;
      }

      /**
       * Get the current progress (0-1) of the animation. If t is >
       * than duration we'll get values like 2.5 (midway through the
       * third iteration)
       */
      const progress = t / this.duration;

      // TODO progress += iterationStart

      /**
       * Get the current iteration (0 indexed). For instance the floor of
       * 2.5 is 2.
       */
      let currentIteration = Math.floor(progress);

      /**
       * Get the current progress of the iteration by taking the remainder
       * so 2.5 is 0.5 through iteration 2
       */
      let iterationProgress = progress % 1.0;

      if (!iterationProgress && progress >= 1) {
        iterationProgress = 1;
      }

      /**
       * If iteration progress is 1 we count that as the end
       * of the previous iteration.
       */
      iterationProgress === 1 && currentIteration--;

      /**
       * Reverse progress if we're not running in "normal" direction
       */
      const iterationIsOdd = currentIteration % 2;
      if (
        direction === "reverse" ||
        (direction === "alternate" && iterationIsOdd) ||
        (direction === "alternate-reverse" && !iterationIsOdd)
      ) {
        iterationProgress = 1 - iterationProgress;
      }

      const p = t >= this.totalDuration ? 1 : Math.min(iterationProgress, 1);
      const latest = interpolate(this.easing(p));

      output(latest);

      const isAnimationFinished =
        this.pauseTime === undefined &&
        (this.playState === "finished" || t >= this.totalDuration + endDelay);

      if (isAnimationFinished) {
        this.playState = "finished";
        this.resolve?.(latest);
      } else if (this.playState !== "idle") {
        this.requestAnimationFrame();
      }
    };

    this.play();
  }

  finished = new Promise((resolve, reject) => {
    this.resolve = resolve;
    this.reject = reject;
  });

  play() {
    const now = performance.now();
    this.playState = "running";

    if (this.pauseTime !== undefined) {
      this.startTime = now - this.pauseTime;
    } else if (!this.startTime) {
      this.startTime = now;
    }

    this.cancelTimestamp = this.startTime;
    this.pauseTime = undefined;
    this.requestAnimationFrame();
  }

  pause() {
    this.playState = "paused";
    this.pauseTime = this.t;
  }

  finish() {
    this.playState = "finished";
    this.tick(0);
  }

  stop() {
    this.playState = "idle";

    this.cancelAnimationFrame();

    this.reject?.(false);
  }

  cancel() {
    this.stop();
    this.tick(this.cancelTimestamp!);
  }

  reverse() {
    this.rate *= -1;
  }

  commitStyles() {}

  private updateDuration(duration: number) {
    this.duration = duration;
    this.totalDuration = duration * (this.repeat + 1);
  }

  get currentTime() {
    return this.t;
  }

  set currentTime(t: number) {
    if (this.pauseTime !== undefined || this.rate === 0) {
      this.pauseTime = t;
    } else {
      this.startTime = performance.now() - t / this.rate;
    }
  }

  get playbackRate() {
    return this.rate;
  }

  set playbackRate(rate) {
    this.rate = rate;
  }

  private requestAnimationFrame() {
    if (this.frameRequestId === undefined) {
      this.frameRequestId = requestAnimationFrame(this.tick);
    }
  }

  private cancelAnimationFrame() {
    if (this.frameRequestId !== undefined) {
      cancelAnimationFrame(this.frameRequestId);
      this.frameRequestId = undefined;
    }
  }
}

basically the class methods requestAnimationFrame and cancelAnimationFrame ensure you cannot double request an animation frame

xaviergonz · 2022-07-10T12:17:46Z

And here's the diff
https://www.diffchecker.com/6w8ylKos

xaviergonz · 2022-07-13T16:59:36Z

@mattgperry I just checked and the original sandbox still fails in safari with the latest 10.12.0 version :( , so I think the other fixes in the diff above are also needed

mattgperry · 2022-07-13T19:22:54Z

Thanks for letting me know!

xaviergonz · 2022-07-21T20:46:45Z

Here is the fix in patch format

From 0a0da4c23d6cf4e06e8666df327191aa481361e2 Mon Sep 17 00:00:00 2001
Date: Thu, 21 Jul 2022 22:45:59 +0200
Subject: [PATCH] fix animation polyfill

---
 packages/animation/src/Animation.ts           | 22 ++++++++++++++-----
 .../animation/src/__tests__/index.test.ts     |  8 +++----
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/packages/animation/src/Animation.ts b/packages/animation/src/Animation.ts
index 7c70dac..0428fe5 100644
--- a/packages/animation/src/Animation.ts
+++ b/packages/animation/src/Animation.ts
@@ -75,6 +75,8 @@ export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
     )
 
     this.tick = (timestamp: number) => {
+      this.frameRequestId = undefined
+
       // TODO: Temporary fix for OptionsResolver typing
       delay = delay as number
 
@@ -156,7 +158,7 @@ export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
         this.playState = "finished"
         this.resolve?.(latest)
       } else if (this.playState !== "idle") {
-        this.frameRequestId = requestAnimationFrame(this.tick)
+        this.requestAnimationFrame()
       }
     }
 
@@ -180,7 +182,7 @@ export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
 
     this.cancelTimestamp = this.startTime
     this.pauseTime = undefined
-    this.frameRequestId = requestAnimationFrame(this.tick)
+    this.requestAnimationFrame()
   }
 
   pause() {
@@ -196,9 +198,7 @@ export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
   stop() {
     this.playState = "idle"
 
-    if (this.frameRequestId !== undefined) {
-      cancelAnimationFrame(this.frameRequestId)
-    }
+    this.cancelAnimationFrame()
 
     this.reject?.(false)
   }
@@ -238,4 +238,16 @@ export class Animation implements Omit<AnimationControls, "stop" | "duration"> {
   set playbackRate(rate) {
     this.rate = rate
   }
+
+  private requestAnimationFrame() {
+    if (this.frameRequestId === undefined) {
+      this.frameRequestId = requestAnimationFrame(this.tick)
+    }
+  }
+  private cancelAnimationFrame() {
+    if (this.frameRequestId !== undefined) {
+      cancelAnimationFrame(this.frameRequestId)
+      this.frameRequestId = undefined
+    }
+  }
 }
diff --git a/packages/animation/src/__tests__/index.test.ts b/packages/animation/src/__tests__/index.test.ts
index d9aee09..3b57123 100644
--- a/packages/animation/src/__tests__/index.test.ts
+++ b/packages/animation/src/__tests__/index.test.ts
@@ -297,7 +297,7 @@ describe("animateNumber", () => {
     await animation.finished
     expect(output).toEqual([
       0.25, 0.5, 0.7499999999999999, 1, 0.25, 0.4999999999999998,
-      0.4999999999999998, 0.4999999999999998, 1,
+      0.4999999999999998, 0.4999999999999998, 0.7499999999999998, 1,
     ])
   })
 
@@ -331,9 +331,9 @@ describe("animateNumber", () => {
     )
     await animation.finished
     expect(output).toEqual([
-      0.125, 0.25, 0.37499999999999994, 0.37499999999999994, 0.25, 0.25, 0.25,
-      0.37499999999999994, 0.5, 0.625, 0.7499999999999999, 0.8749999999999999,
-      1,
+      0.125, 0.25, 0.37499999999999994, 0.37499999999999994, 0.25, 0.25, 0.125,
+      0.25, 0.37499999999999994, 0.5, 0.625, 0.7499999999999999,
+      0.8749999999999999, 1,
     ])
     expect(currentTime).toBe(150)
   })
-- 
2.37.0

tomByrer · 2022-08-20T21:50:11Z

Thanks for looking into this!
Is the Chrome result (reset to 1st frame) the desired behavior?

xaviergonz added the Bug Bug report label Jun 28, 2022

mattgperry mentioned this issue Jul 10, 2022

Resize #108

Merged

mattgperry closed this as completed in #108 Jul 10, 2022

mattgperry reopened this Jul 13, 2022

xaviergonz linked a pull request Aug 9, 2022 that will close this issue

fix calling play() then cancel() on an animation has different results in chrome and safari #125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug] Calling play() then cancel() on an animation has different results in chrome and in safari #102

[Bug] Calling play() then cancel() on an animation has different results in chrome and in safari #102

xaviergonz commented Jun 28, 2022 •

edited

Loading

xaviergonz commented Jun 28, 2022

mattgperry commented Jun 28, 2022

xaviergonz commented Jun 28, 2022 •

edited

Loading

xaviergonz commented Jun 28, 2022

xaviergonz commented Jun 29, 2022

xaviergonz commented Jul 5, 2022

mattgperry commented Jul 9, 2022

xaviergonz commented Jul 10, 2022

mattgperry commented Jul 10, 2022

xaviergonz commented Jul 10, 2022 •

edited

Loading

xaviergonz commented Jul 10, 2022 •

edited

Loading

xaviergonz commented Jul 10, 2022

xaviergonz commented Jul 13, 2022 •

edited

Loading

mattgperry commented Jul 13, 2022

xaviergonz commented Jul 21, 2022

tomByrer commented Aug 20, 2022

[Bug] Calling play() then cancel() on an animation has different results in chrome and in safari #102

[Bug] Calling play() then cancel() on an animation has different results in chrome and in safari #102

Comments

xaviergonz commented Jun 28, 2022 • edited Loading

xaviergonz commented Jun 28, 2022

mattgperry commented Jun 28, 2022

xaviergonz commented Jun 28, 2022 • edited Loading

xaviergonz commented Jun 28, 2022

xaviergonz commented Jun 29, 2022

xaviergonz commented Jul 5, 2022

mattgperry commented Jul 9, 2022

xaviergonz commented Jul 10, 2022

mattgperry commented Jul 10, 2022

xaviergonz commented Jul 10, 2022 • edited Loading

xaviergonz commented Jul 10, 2022 • edited Loading

xaviergonz commented Jul 10, 2022

xaviergonz commented Jul 13, 2022 • edited Loading

mattgperry commented Jul 13, 2022

xaviergonz commented Jul 21, 2022

tomByrer commented Aug 20, 2022

xaviergonz commented Jun 28, 2022 •

edited

Loading

xaviergonz commented Jun 28, 2022 •

edited

Loading

xaviergonz commented Jul 10, 2022 •

edited

Loading

xaviergonz commented Jul 10, 2022 •

edited

Loading

xaviergonz commented Jul 13, 2022 •

edited

Loading