diff --git a/packages/studio/src/components/editor/KeyframeNavigation.tsx b/packages/studio/src/components/editor/KeyframeNavigation.tsx index b8016f1a6..219e08d92 100644 --- a/packages/studio/src/components/editor/KeyframeNavigation.tsx +++ b/packages/studio/src/components/editor/KeyframeNavigation.tsx @@ -11,12 +11,15 @@ interface KeyframeNavigationProps { tweenPercentage?: number; properties: Record; ease?: string; + /** The tween that authored this keyframe, when the cache knows it. */ + animationId?: string; }> | null; /** Current playhead percentage within the element's lifetime (0-100) */ currentPercentage: number; onSeek: (percentage: number) => void; onAddKeyframe: (percentage: number) => void; - onRemoveKeyframe: (percentage: number) => void; + /** `animationId` is the clicked keyframe's OWN tween; see handleDiamondClick. */ + onRemoveKeyframe: (percentage: number, animationId?: string) => void; onConvertToKeyframes: () => void; } @@ -152,7 +155,12 @@ export const KeyframeNavigation = memo(function KeyframeNavigation({ if (diamondState === "ghost") { onConvertToKeyframes(); } else if (diamondState === "active" && atCurrent) { - onRemoveKeyframe(atCurrent.tweenPercentage ?? atCurrent.percentage); + // Report the keyframe's OWN tween. A merged gutter row shows the keyframes + // of every tween in the property group, so the caller's "the group's + // animation" guess names the wrong tween whenever the clicked keyframe + // belongs to a sibling — and the writer then finds no keyframe at that + // percentage and silently does nothing. + onRemoveKeyframe(atCurrent.tweenPercentage ?? atCurrent.percentage, atCurrent.animationId); } else { onAddKeyframe(clipToTweenPercentage(propertyKeyframes, currentPercentage)); } diff --git a/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx b/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx new file mode 100644 index 000000000..3cf0ddcd1 --- /dev/null +++ b/packages/studio/src/components/editor/KeyframeNavigationDiamond.test.tsx @@ -0,0 +1,59 @@ +// @vitest-environment happy-dom + +import { act } from "react"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { installReactActEnvironment, mountReactHarness } from "../../hooks/domSelectionTestHarness"; +import { KeyframeNavigation } from "./KeyframeNavigation"; + +beforeAll(installReactActEnvironment); + +/** + * Regression: a merged gutter row shows the keyframes of EVERY tween in the + * property group. The panel guesses "the group's animation" for the write, so a + * click on a keyframe authored by a sibling tween named the wrong animation and + * the writer silently found nothing to remove. The diamond now reports the + * clicked keyframe's own animationId. + */ +const MERGED_ROW = [ + { percentage: 0, tweenPercentage: 0, properties: { x: 0 }, animationId: "delta-to-500-position" }, + { + percentage: 50, + tweenPercentage: 25, + properties: { x: 120 }, + animationId: "delta-to-4000-position", + }, +]; + +function clickDiamond(currentPercentage: number, onRemoveKeyframe: (...args: never[]) => void) { + const root = mountReactHarness( + , + ); + const diamond = document.querySelector('[title="Remove x keyframe"]'); + expect(diamond).not.toBeNull(); + act(() => { + diamond?.click(); + }); + act(() => root.unmount()); +} + +describe("KeyframeNavigation diamond", () => { + it("reports the clicked keyframe's own animation on a merged row", () => { + const onRemoveKeyframe = vi.fn(); + clickDiamond(50, onRemoveKeyframe); + expect(onRemoveKeyframe).toHaveBeenCalledWith(25, "delta-to-4000-position"); + }); + + it("still reports the first tween's keyframe as its own", () => { + const onRemoveKeyframe = vi.fn(); + clickDiamond(0, onRemoveKeyframe); + expect(onRemoveKeyframe).toHaveBeenCalledWith(0, "delta-to-500-position"); + }); +}); diff --git a/packages/studio/src/components/editor/PropertyPanel.tsx b/packages/studio/src/components/editor/PropertyPanel.tsx index 237d84269..0c9905e80 100644 --- a/packages/studio/src/components/editor/PropertyPanel.tsx +++ b/packages/studio/src/components/editor/PropertyPanel.tsx @@ -408,7 +408,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "x", displayX) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("x"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("x"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("x"))} /> )} @@ -433,7 +435,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "y", displayY) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("y"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("y"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("y"))} /> )} @@ -458,7 +462,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "width", displayW) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("width"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("width"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("width"))} /> )} @@ -483,7 +489,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "height", displayH) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("height"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("height"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("height"))} /> )} @@ -507,7 +515,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro onCommitAnimatedProperty && void onCommitAnimatedProperty(element, "rotation", displayR) } - onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("rotation"), pct)} + onRemoveKeyframe={(pct, animationId) => + onRemoveKeyframe?.(animationId ?? animIdForProp("rotation"), pct) + } onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("rotation"))} /> )} diff --git a/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx b/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx index da217814f..a9c256a00 100644 --- a/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx +++ b/packages/studio/src/components/editor/propertyPanelFlatLayoutSection.tsx @@ -83,10 +83,10 @@ function KeyframeGutter({ track("button", `Add ${property} keyframe`); void onCommitAnimatedProperty(element, property, displayValue); }} - onRemoveKeyframe={(pct) => { + onRemoveKeyframe={(pct, animationId) => { if (!onRemoveKeyframe) return; track("button", `Remove ${property} keyframe`); - onRemoveKeyframe(animIdForProp(property), pct); + onRemoveKeyframe(animationId ?? animIdForProp(property), pct); }} onConvertToKeyframes={() => { if (!onConvertToKeyframes) return; diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index 1589f7061..cbb35e90b 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -184,7 +184,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { view.unmount(); }); - it("settles false for a boundary drag while the tween is still flat", async () => { + it("retimes a flat tween's boundary through update-meta, not the keyframe writer", async () => { const view = renderCallbacks(); await expect( @@ -198,9 +198,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { }, 25, ), - ).resolves.toBe(false); + ).resolves.toBe(true); + // The start boundary moved to 0.25s; the end stays put, so the window is 0.75s. + expect(mocks.actions.handleGsapUpdateMeta).toHaveBeenCalledWith( + flatAnimation.id, + { position: 0.25, duration: 0.75 }, + mocks.selection, + ); expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled(); + // resize-keyframed-tween would convert the flat tween to keyframes form. expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled(); view.unmount(); }); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index 050b37339..7ca47e0bf 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -256,10 +256,6 @@ export function useTimelineEditCallbacks({ const anim = animations.find((a) => a.id === target.animId); const tweenStart = anim ? resolveTweenStart(anim) : null; if (!anim || tweenStart === null) return Promise.resolve(false); - // Synthesized flat endpoints are clip boundaries, not authored keyframes. - // Boundary-to-clip resize wiring is intentionally deferred; ignore the - // drag rather than dispatching a free keyframe move that cannot be written. - if (!anim.keyframes) return Promise.resolve(false); const sourceFile = sel.sourceFile || activeCompPath || "index.html"; const { elements, domClipChildren } = usePlayerStore.getState(); const { elStart, elDuration } = resolveClipTimingBasis( @@ -285,6 +281,19 @@ export function useTimelineEditCallbacks({ decision.position != null && decision.duration != null ) { + // An empty remap means a FLAT tween's synthesized boundary: there is no + // keyframe node to re-key, only the window to move. Sending it through + // the keyframed-resize writer would rewrite the authored flat tween into + // keyframes form as a side effect of a pure position/duration change, so + // dispatch update-meta and leave the tween as the author wrote it. + if (decision.pctRemap.length === 0) { + handleGsapUpdateMeta( + target.animId, + { position: decision.position, duration: decision.duration }, + sel, + ); + return true; + } return handleGsapResizeKeyframedTween( target.animId, decision.position,