diff --git a/packages/studio/src/components/editor/MotionPathOverlay.tsx b/packages/studio/src/components/editor/MotionPathOverlay.tsx index 5f06ec6c99..797d4c3351 100644 --- a/packages/studio/src/components/editor/MotionPathOverlay.tsx +++ b/packages/studio/src/components/editor/MotionPathOverlay.tsx @@ -12,6 +12,7 @@ import { KeyframeDiamondContextMenu, type KeyframeDiamondContextMenuState, } from "../../player/components/KeyframeDiamondContextMenu"; +import type { TimelineKeyframeTarget } from "../../player/components/timelineKeyframeIdentity"; import { commitAddKeyframe, commitAddWaypoint, @@ -96,9 +97,13 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ const [draft, setDraft] = useState(null); const [ghost, setGhost] = useState<{ x: number; y: number; segIndex: number } | null>(null); const [hoverNode, setHoverNode] = useState(null); - // Right-click context menu on a keyframe node — same delete actions as the - // timeline keyframe diamond. - const [kfMenu, setKfMenu] = useState(null); + // Right-click context menu on a path node — same delete actions as the + // timeline keyframe diamond. The node it was opened on rides along, because + // which entries apply depends on whether it is a keyframe or a waypoint. + const [kfMenu, setKfMenu] = useState<{ + state: KeyframeDiamondContextMenuState; + ref: MotionNodeRef; + } | null>(null); // The keyframe % selected by clicking its node — highlighted, and the next drag // modifies it rather than adding a keyframe. const activeKeyframePct = usePlayerStore((s) => s.activeKeyframePct); @@ -436,21 +441,47 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ }; const elementId = selection?.id ?? null; - // Right-click a keyframe node → the timeline's keyframe context menu (delete - // this keyframe / delete all), so motion-path keyframes are removable in place. + // Right-click any path node → the timeline's keyframe context menu (delete this + // one / delete all), so path nodes are removable in place. Waypoints open it + // too: returning early for them let the browser's own context menu open over + // the editor overlay, which is never what a right-click on a node should do. const onNodeContextMenu = (e: React.MouseEvent, ref: MotionNodeRef) => { - if (ref.type !== "keyframe" || !animId || !elementId || !timelineElement) return; + if (!animId || !elementId || !timelineElement) return; e.preventDefault(); e.stopPropagation(); setKfMenu({ - x: e.clientX, - y: e.clientY, - element: timelineElement, - elementId, - percentage: ref.pct, - tweenPercentage: ref.pct, + ref, + state: { + x: e.clientX, + y: e.clientY, + element: timelineElement, + elementId, + // A waypoint carries no percentage of its own: one tween-level ease owns + // every segment, and its index is the identity the delete acts on. The + // menu never reads this for a waypoint, because the only entry that + // would (Move to Playhead) is hidden below. + percentage: ref.type === "keyframe" ? ref.pct : 0, + tweenPercentage: ref.type === "keyframe" ? ref.pct : 0, + }, }); }; + const menuRef = kfMenu?.ref; + // Deleting one node: by tween-% for a keyframe, by path index for a waypoint. + // A waypoint delete is offered on the same condition as the hover x badge, + // because removeMotionPathPointInScript refuses to drop an arc below two + // anchors and the entry would silently do nothing. + const onMenuDelete = + menuRef === undefined || !animId + ? undefined + : menuRef.type === "keyframe" + ? (_elId: string, keyframe: TimelineKeyframeTarget) => { + handleGsapRemoveKeyframe(animId, keyframe.percentage); + } + : removable + ? () => { + void commitRemoveWaypoint(animId, menuRef.index, commitMutation); + } + : undefined; return ( <> @@ -532,14 +563,17 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ {kfMenu && ( setKfMenu(null)} - onDelete={(_elId, keyframe) => - animId && handleGsapRemoveKeyframe(animId, keyframe.percentage) - } + onDelete={onMenuDelete} onDeleteAll={() => animId && handleGsapRemoveAllKeyframes(animId)} - onMoveToPlayhead={(_element, keyframe) => - animId && handleGsapMoveKeyframeToPlayhead(animId, keyframe.percentage) + // Retiming needs a percentage of this node's own, which a waypoint + // does not have. + onMoveToPlayhead={ + kfMenu.ref.type === "keyframe" + ? (_element, keyframe) => + animId && handleGsapMoveKeyframeToPlayhead(animId, keyframe.percentage) + : undefined } /> )} diff --git a/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx b/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx index f802da680e..6e457a0828 100644 --- a/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx +++ b/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx @@ -66,4 +66,25 @@ describe("KeyframeDiamondContextMenu", () => { expect(onDelete).toHaveBeenCalledWith("box", target); expect(onMoveToPlayhead).toHaveBeenCalledWith(element, target); }); + + // An arc waypoint on a two-anchor path cannot be dropped on its own, so the + // caller withholds onDelete rather than offering an entry that does nothing. + it("hides the single-node delete when no handler is given", () => { + const host = document.createElement("div"); + document.body.appendChild(host); + const root = createRoot(host); + act(() => + root.render( + {}} onDeleteAll={vi.fn()} />, + ), + ); + + const labels = Array.from(document.body.querySelectorAll("button")).map( + (button) => button.textContent, + ); + expect(labels).toEqual(["Delete All Keyframes"]); + + act(() => root.unmount()); + host.remove(); + }); }); diff --git a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx index be500d494e..bd47e3ee85 100644 --- a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx +++ b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx @@ -19,7 +19,10 @@ export interface KeyframeDiamondContextMenuState { interface KeyframeDiamondContextMenuProps { state: KeyframeDiamondContextMenuState; onClose: () => void; - onDelete: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + /** Omitted where this node cannot be deleted on its own (see the arc-waypoint + * floor in removeMotionPathPointInScript): an entry that silently no-ops is + * worse than no entry. */ + onDelete?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; onDeleteAll: (element: TimelineElement) => void; /** Retime the keyframe to the current playhead, preserving its value + ease. */ onMoveToPlayhead?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void; @@ -43,7 +46,9 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe }; const menuWidth = 200; - const menuHeight = onMoveToPlayhead ? 100 : 70; + // Measured off the rendered rows, so the flip-up test below stays right as + // optional entries drop out. + const menuHeight = 10 + (1 + (onMoveToPlayhead ? 1 : 0) + (onDelete ? 1 : 0)) * 30; const overflowY = state.y + menuHeight - window.innerHeight; const adjustedX = state.x + menuWidth > window.innerWidth ? state.x - menuWidth : state.x; const adjustedY = overflowY > 0 ? state.y - overflowY - 8 : state.y; @@ -71,16 +76,18 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe )} {/* Delete */} - + {onDelete && ( + + )}