diff --git a/packages/studio/src/components/editor/keyframeRetime.test.ts b/packages/studio/src/components/editor/keyframeRetime.test.ts index 4047d5c77..81264f0ab 100644 --- a/packages/studio/src/components/editor/keyframeRetime.test.ts +++ b/packages/studio/src/components/editor/keyframeRetime.test.ts @@ -180,3 +180,45 @@ describe("resolveKeyframeRetime — guards", () => { expect(r.pctRemap).toEqual([]); }); }); + +describe("resolveKeyframeRetime — move percentages are rounded like the resize path", () => { + // The move branch used to return the raw quotient, so `74.81203007518799%` + // landed in the user's source and churned the diff on every drag. + const decimals = (n: number): number => String(n).split(".")[1]?.length ?? 0; + + it("rounds a repeating quotient to 3dp", () => { + const r = resolveKeyframeRetime({ + tweenStart: 2, + tweenDuration: 3, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, // (3-2)/3 = 33.333333333333336% + }); + expect(r.kind).toBe("move"); + expect(r.toTweenPct).toBe(33.333); + }); + + it("never emits more than 3 decimal places", () => { + for (const tweenDuration of [3, 7, 9, 11, 133]) { + const r = resolveKeyframeRetime({ + tweenStart: 2, + tweenDuration, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, + }); + expect(r.kind).toBe("move"); + expect(decimals(r.toTweenPct ?? 0)).toBeLessThanOrEqual(3); + } + }); + + it("leaves an already-short percentage untouched", () => { + const r = resolveKeyframeRetime({ + ...WINDOW, + keyframes: KEYFRAMES, + draggedTweenPct: 0, + dropAbsTime: 3, // (3-2)/4 = exactly 25% + }); + expect(r.toTweenPct).toBe(25); + }); +}); diff --git a/packages/studio/src/components/editor/keyframeRetime.ts b/packages/studio/src/components/editor/keyframeRetime.ts index f7fa1d134..c722e6abb 100644 --- a/packages/studio/src/components/editor/keyframeRetime.ts +++ b/packages/studio/src/components/editor/keyframeRetime.ts @@ -121,7 +121,10 @@ export function resolveKeyframeRetime(opts: { // Within the tween window → plain move (re-key the tween-%). if (dropAbsTime >= tweenStart - EPSILON_TIME && dropAbsTime <= tweenEnd + EPSILON_TIME) { - const toTweenPct = clamp(((dropAbsTime - tweenStart) / tweenDuration) * 100, 0, 100); + // Round here, not at the return: the no-op test below and the value written + // to source must be the same number. The resize branch already rounds, so + // this keeps both write paths at the authored 3dp precision. + const toTweenPct = round3(clamp(((dropAbsTime - tweenStart) / tweenDuration) * 100, 0, 100)); if (Math.abs(toTweenPct - draggedTweenPct) < NOOP_EPSILON_PCT) return { kind: "noop" }; return { kind: "move", toTweenPct }; } diff --git a/packages/studio/src/player/components/useTimelineRangeSelection.ts b/packages/studio/src/player/components/useTimelineRangeSelection.ts index 1a98e251c..c4cd93046 100644 --- a/packages/studio/src/player/components/useTimelineRangeSelection.ts +++ b/packages/studio/src/player/components/useTimelineRangeSelection.ts @@ -268,6 +268,11 @@ export function useTimelineRangeSelection({ if (!point || !scrollRect || isTimelineRulerPress(e.clientY, scrollRect.top)) { isDragging.current = true; setIsScrubbing(true); + // Seed the pending coordinate so a press with no pointermove still + // replays THIS x on pointerup. `updateScrubDrag` is the only other + // writer, so without this a plain click settles on the ref's initial + // 0 and clamps the playhead back to t=0. + pendingClientXRef.current = e.clientX; seekFromX(e.clientX); return; } diff --git a/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx b/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx new file mode 100644 index 000000000..61c93c6a4 --- /dev/null +++ b/packages/studio/src/player/components/useTimelineRangeSelectionScrub.test.tsx @@ -0,0 +1,127 @@ +// @vitest-environment happy-dom + +// Regression guard for the ruler-click seek. `handlePointerUp` replays +// `pendingClientXRef`, which only `updateScrubDrag` (pointermove) used to write, +// so a press with no move settled on the ref's initial 0 and clamped the +// playhead to t=0 — silently discarding the correct pointerdown seek. +import React, { act } from "react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { mountReactHarness } from "../../hooks/domSelectionTestHarness"; +import { useTimelineRangeSelection } from "./useTimelineRangeSelection"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +/** Scroll container top; a press within RULER_H of this counts as a ruler press. */ +const SCROLL_TOP = 100; +/** Comfortably inside the ruler band (RULER_H is 28 at time of writing). */ +const RULER_Y = SCROLL_TOP + 4; + +type Handlers = ReturnType; + +function makeScrollEl(): HTMLDivElement { + const el = document.createElement("div"); + el.getBoundingClientRect = () => + ({ top: SCROLL_TOP, left: 0, width: 1000, height: 400 }) as DOMRect; + return el; +} + +function makePointerEvent(clientX: number, clientY: number): React.PointerEvent { + const currentTarget = document.createElement("div"); + currentTarget.setPointerCapture = vi.fn(); + return { + button: 0, + shiftKey: false, + metaKey: false, + ctrlKey: false, + pointerId: 1, + clientX, + clientY, + target: document.createElement("div"), + currentTarget, + } as unknown as React.PointerEvent; +} + +const roots: Array<{ unmount: () => void }> = []; + +afterEach(() => { + for (const root of roots.splice(0)) act(() => root.unmount()); +}); + +function setup(): { handlers: () => Handlers; seekFromX: ReturnType } { + const seekFromX = vi.fn(); + const scrollEl = makeScrollEl(); + let latest: Handlers | null = null; + + // Hoisted so they survive re-renders. The hook mutates `isDragging.current` + // across the press, and a fresh object per render would reset it to false. + const scrollRef = { current: scrollEl }; + const ppsRef = { current: 25 }; + const dragScrollRaf = { current: 0 }; + const isDragging = { current: false }; + const elementsRef = { current: [] }; + const trackOrderRef = { current: [] }; + const rowHeightsRef = { current: [] }; + + function Probe(): null { + latest = useTimelineRangeSelection({ + scrollRef, + ppsRef, + effectiveDuration: 30, + pps: 25, + seekFromX, + autoScrollDuringDrag: vi.fn(), + dragScrollRaf, + isDragging, + setShowPopover: vi.fn(), + elementsRef, + trackOrderRef, + rowHeightsRef, + contentOrigin: 0, + }); + return null; + } + + roots.push(mountReactHarness()); + return { + handlers: () => { + if (!latest) throw new Error("hook did not render"); + return latest; + }, + seekFromX, + }; +} + +describe("useTimelineRangeSelection — ruler press seek", () => { + it("replays the pressed x on pointerup when the pointer never moved", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(1000, RULER_Y))); + act(() => handlers().handlePointerUp()); + + // Both the press and the settle must land on the SAME x. The bug settled on + // 0, so the LAST call is the one that decides where the playhead ends up. + expect(seekFromX).toHaveBeenCalledWith(1000); + expect(seekFromX).not.toHaveBeenCalledWith(0); + expect(seekFromX.mock.calls.at(-1)).toEqual([1000]); + }); + + it("does not fall back to x=0 for a press nearer the left edge either", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(400, RULER_Y))); + act(() => handlers().handlePointerUp()); + + expect(seekFromX.mock.calls.at(-1)).toEqual([400]); + }); + + it("settles on the final pointermove x when the pointer did move", () => { + const { handlers, seekFromX } = setup(); + + act(() => handlers().handlePointerDown(makePointerEvent(700, RULER_Y))); + act(() => handlers().handlePointerMove(makePointerEvent(760, RULER_Y))); + act(() => handlers().handlePointerUp()); + + // The move must win over the seeded press coordinate. + expect(seekFromX.mock.calls.at(-1)).toEqual([760]); + }); +});