diff --git a/packages/excalidraw/renderer/animation.ts b/packages/excalidraw/renderer/animation.ts index f6c7b04d68..27f85bb472 100644 --- a/packages/excalidraw/renderer/animation.ts +++ b/packages/excalidraw/renderer/animation.ts @@ -5,39 +5,60 @@ export type Animation = (params: { state?: R; }) => R | null | undefined; +type AnimationRecord = { + animation: Animation; + lastTime: number; + state: any; +}; + export class AnimationController { private static scheduledFrame: | { id: ReturnType; type: "raf" } | { id: ReturnType; type: "timeout" } | null = null; - private static animations = new Map< - string, - { - animation: Animation; - lastTime: number; - state: any; - } - >(); + private static animations = new Map(); static start(key: string, animation: Animation) { if (AnimationController.animations.has(key)) { return; } - const initialState = animation({ - deltaTime: 0, + const record: AnimationRecord = { + animation, + lastTime: 0, state: undefined, - }); + }; + AnimationController.animations.set(key, record); - if (initialState) { - AnimationController.animations.set(key, { - animation, - lastTime: 0, - state: initialState, + let initialState: R | null | undefined; + try { + initialState = animation({ + deltaTime: 0, + state: undefined, }); - - AnimationController.scheduleNextFrame(); + } catch (error) { + if (AnimationController.animations.get(key) === record) { + AnimationController.animations.delete(key); + AnimationController.cancelScheduledFrameIfIdle(); + } + throw error; } + + // The initial callback may synchronously cancel this animation or replace + // it with another animation under the same key. Never resurrect or + // overwrite it after control returns. + if (AnimationController.animations.get(key) !== record) { + return; + } + + if (!initialState) { + AnimationController.animations.delete(key); + AnimationController.cancelScheduledFrameIfIdle(); + return; + } + + record.state = initialState; + AnimationController.scheduleNextFrame(); } private static scheduleNextFrame() { @@ -85,7 +106,15 @@ export class AnimationController { AnimationController.scheduledFrame = null; if (AnimationController.animations.size > 0) { - for (const [key, animation] of AnimationController.animations) { + // A callback may synchronously add, cancel, or replace animations. Work + // from the frame's starting set so newly started animations begin on the + // next frame and every record runs at most once per tick. + const animations = [...AnimationController.animations]; + for (const [key, animation] of animations) { + if (AnimationController.animations.get(key) !== animation) { + continue; + } + const now = performance.now(); const deltaTime = animation.lastTime === 0 ? 0 : now - animation.lastTime; @@ -95,6 +124,12 @@ export class AnimationController { state: animation.state, }); + // The callback may have cancelled or replaced itself. Only the record + // that was invoked is allowed to update or remove its registration. + if (AnimationController.animations.get(key) !== animation) { + continue; + } + if (!state) { AnimationController.animations.delete(key); diff --git a/packages/excalidraw/tests/animation.test.ts b/packages/excalidraw/tests/animation.test.ts index 08df4473a9..2843b9d680 100644 --- a/packages/excalidraw/tests/animation.test.ts +++ b/packages/excalidraw/tests/animation.test.ts @@ -70,4 +70,98 @@ describe("AnimationController", () => { expect(secondFrames).toBe(1); expect(vi.getTimerCount()).toBe(0); }); + + it("does not resurrect an animation cancelled during its initial callback", () => { + let frames = 0; + + AnimationController.start(FIRST_KEY, () => { + frames++; + AnimationController.cancel(FIRST_KEY); + return { keep: true }; + }); + + expect(frames).toBe(1); + expect(AnimationController.running(FIRST_KEY)).toBe(false); + expect(vi.getTimerCount()).toBe(0); + }); + + it("cleans up the registration when the initial callback throws", () => { + expect(() => + AnimationController.start(FIRST_KEY, () => { + throw new Error("initial frame failed"); + }), + ).toThrow("initial frame failed"); + + expect(AnimationController.running(FIRST_KEY)).toBe(false); + expect(vi.getTimerCount()).toBe(0); + }); + + it("preserves a same-key replacement started during the initial callback", async () => { + let originalFrames = 0; + let replacementFrames = 0; + + const replacement = ({ state }: { state?: { keep: true } }) => { + replacementFrames++; + return state ? null : { keep: true as const }; + }; + + AnimationController.start(FIRST_KEY, () => { + originalFrames++; + AnimationController.cancel(FIRST_KEY); + AnimationController.start(FIRST_KEY, replacement); + return { keep: true }; + }); + + expect(originalFrames).toBe(1); + expect(replacementFrames).toBe(1); + expect(AnimationController.running(FIRST_KEY)).toBe(true); + + await vi.runOnlyPendingTimersAsync(); + + expect(originalFrames).toBe(1); + expect(replacementFrames).toBe(2); + expect(AnimationController.running(FIRST_KEY)).toBe(false); + }); + + it("does not let a completed callback delete its same-key replacement", async () => { + // tests for this unwanted case: + // + // 1. The original animation’s scheduled callback begins. + // 2. Before it returns, application code cancels it and starts + // a replacement using the same key. + // 3. The original callback returns null. + // 4. Previously, tick() then called delete(key), accidentally deleting + // the replacement. + + let originalFrames = 0; + let replacementFrames = 0; + + const replacement = ({ state }: { state?: { keep: true } }) => { + replacementFrames++; + return state ? null : { keep: true as const }; + }; + + AnimationController.start(FIRST_KEY, ({ state }) => { + if (!state) { + return { keep: true }; + } + + originalFrames++; + AnimationController.cancel(FIRST_KEY); + AnimationController.start(FIRST_KEY, replacement); + return null; + }); + + await vi.runOnlyPendingTimersAsync(); + + expect(originalFrames).toBe(1); + expect(replacementFrames).toBe(1); + expect(AnimationController.running(FIRST_KEY)).toBe(true); + + await vi.runOnlyPendingTimersAsync(); + + expect(originalFrames).toBe(1); + expect(replacementFrames).toBe(2); + expect(AnimationController.running(FIRST_KEY)).toBe(false); + }); });