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

[xaviergonz]

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]

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]

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]

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

https://github.com/motiondivision/motionone/blob/d6306f38611747ab6ad98739f58b4db017be5a5b/packages/animation/src/Animation.ts#L169

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]

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]

@mattgperry I can confirm the fix above works

[xaviergonz]

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]

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

[xaviergonz]

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]

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]

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]

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]

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

[xaviergonz]

@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]

Thanks for letting me know!

[xaviergonz]

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]

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