fix(studio): harden keyframe editing semantics

This commit is contained in:
Miguel Angel Simon Sierra
2026-07-28 00:13:46 +02:00
parent 9fc0011703
commit d57039882f
41 changed files with 1294 additions and 435 deletions
@@ -1,10 +1,12 @@
import { memo } from "react";
import { createPortal } from "react-dom";
import { useContextMenuDismiss } from "../../hooks/useContextMenuDismiss";
import type { TimelineElement } from "../store/playerStore";
export interface KeyframeDiamondContextMenuState {
x: number;
y: number;
element: TimelineElement;
elementId: string;
percentage: number;
tweenPercentage?: number;
@@ -23,12 +25,12 @@ interface KeyframeDiamondContextMenuProps {
tweenPercentage?: number,
animationId?: string,
) => void;
onDeleteAll: (elementId: string) => void;
onDeleteAll: (element: TimelineElement) => void;
onChangeEase?: (elementId: string, percentage: number, ease: string) => void;
onCopyProperties?: (elementId: string, percentage: number) => void;
/** Retime the keyframe to the current playhead, preserving its value + ease. */
onMoveToPlayhead?: (
elementId: string,
element: TimelineElement,
fromPercentage: number,
propertyGroup?: string,
tweenPercentage?: number,
@@ -66,7 +68,7 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe
// and returns the tween-% for the mutation. Passing tween-% here would
// miss the lookup on any tween whose window is shorter than the clip.
onMoveToPlayhead(
state.elementId,
state.element,
state.percentage,
state.propertyGroup,
state.tweenPercentage,
@@ -101,7 +103,7 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe
type="button"
className="w-full flex items-center gap-2 px-3 py-1.5 text-xs text-red-400 hover:bg-neutral-800 cursor-pointer text-left"
onClick={() => {
onDeleteAll(state.elementId);
onDeleteAll(state.element);
onClose();
}}
>
@@ -435,6 +435,9 @@ export const Timeline = memo(function Timeline({
onDragLeave={() => clearDropPreview()}
onDrop={handleAssetDrop}
onPointerDown={(e) => {
// Let interactive controls (keyframe nav/toggle, caret, inputs) handle
// their own clicks — scrubbing here would preventDefault and eat them.
if (e.target instanceof Element && e.target.closest("button, input, select, a")) return;
if (activeTool === "razor" && e.shiftKey && e.button === 0 && scrollRef.current) {
const rect = scrollRef.current.getBoundingClientRect();
const x =
@@ -512,15 +515,15 @@ export const Timeline = memo(function Timeline({
onMoveKeyframe={onMoveKeyframe}
onContextMenuKeyframe={(e, elId, pct) => {
const el = expandedElements.find((x) => (x.key ?? x.id) === elId);
if (el) {
setSelectedElementId(elId);
onSelectElement?.(el);
}
if (!el) return;
setSelectedElementId(elId);
onSelectElement?.(el);
const kfData = keyframeCache.get(elId);
const kf = kfData?.keyframes.find((k) => Math.abs(k.percentage - pct) < 0.2);
setKfContextMenu({
x: e.clientX + 4,
y: e.clientY + 2,
element: el,
elementId: elId,
percentage: pct,
tweenPercentage: kf?.tweenPercentage,
@@ -45,6 +45,42 @@ function renderDiamonds(onClickKeyframe = vi.fn()) {
}
describe("TimelineClipDiamonds", () => {
it("keeps dense keyframe hit regions and visuals from overlapping", () => {
const host = document.createElement("div");
document.body.append(host);
const root = createRoot(host);
act(() => {
root.render(
<TimelineDiamondLane
keyframesData={{
format: "percentage",
keyframes: [30, 60, 90].map((percentage) => ({
percentage,
propertyGroup: "position",
properties: { x: percentage },
})),
}}
clipWidthPx={36}
clipHeightPx={48}
accentColor="#4ba3d2"
isSelected
currentPercentage={0}
elementId="clip-1"
selectedKeyframes={new Set()}
groupAware
/>,
);
});
const diamonds = Array.from(host.querySelectorAll<HTMLButtonElement>("button[title]"));
expect(diamonds).toHaveLength(3);
for (const diamond of diamonds) {
expect(Number.parseFloat(diamond.style.width)).toBeCloseTo(10.8);
expect(Number(diamond.querySelector("svg")?.getAttribute("width"))).toBeCloseTo(8.8);
}
act(() => root.unmount());
});
it("treats primary pointerup without drag as a keyframe click", () => {
const { host, root, onClickKeyframe } = renderDiamonds();
const diamond = host.querySelector<HTMLButtonElement>('button[title="50%"]');
@@ -516,6 +552,18 @@ describe("TimelineClipDiamonds", () => {
act(() => root.unmount());
});
it("keeps the ease button above nearby diamonds without blocking the segment", () => {
const { host, root } = renderSegmentLane(false);
const segment = host.querySelector<HTMLElement>("[data-keyframe-ease-segment]");
const ease = segment?.querySelector<HTMLButtonElement>("[data-keyframe-ease-button]");
const diamond = host.querySelector<HTMLButtonElement>('button[title="50%"]');
expect(segment?.style.pointerEvents).toBe("none");
expect(ease?.style.pointerEvents).toBe("auto");
expect(Number(segment?.style.zIndex)).toBeGreaterThan(Number(diamond?.style.zIndex));
act(() => root.unmount());
});
it("hides the inline ease button on a segment with no source animation id", () => {
// A runtime-scanned keyframe has no animationId, so there is no tween to
// target; the segment ending on it must not render a (dead) ease button.
@@ -183,10 +183,6 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
cancelPreviewFrame();
};
}, []);
// Index of the segment whose mid-point ease button is revealed on hover, like
// Figma. Null = no segment hovered → no button shown (resting state is just
// the connector line + diamonds).
const [hoveredSegment, setHoveredSegment] = useState<number | null>(null);
// The button element can re-render (reposition/unmount) synchronously from
// the state updates onClickKeyframe/onMoveKeyframe trigger, before the
// browser gets to auto-synthesize the "click" event that normally follows
@@ -213,7 +209,6 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
const diamondSize = beatsActive
? Math.round(clipHeightPx * 0.45)
: Math.round(LANE_H * DIAMOND_RATIO);
const half = diamondSize / 2;
const centerY = beatsActive ? BEAT_BAND_H + (clipHeightPx - BEAT_BAND_H) / 2 : clipHeightPx / 2;
const sorted = keyframesData.keyframes
.filter((kf) => kf.percentage >= KF_MIN_PCT && kf.percentage <= KF_MAX_PCT)
@@ -221,6 +216,20 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
// Clip-%s of the sorted keyframes — the neighbour clamp (preview + drop) needs
// the whole row to bound the dragged diamond between its immediate siblings.
const sortedClipPcts = sorted.map((k) => k.percentage);
const sortedCenterXs = sorted.map((keyframe) =>
Math.max(0, Math.min(clipWidthPx, (keyframe.percentage / 100) * clipWidthPx)),
);
const markerMetrics = sortedCenterXs.map((centerX, index) => {
const previousGap = index > 0 ? centerX - sortedCenterXs[index - 1]! : Infinity;
const nextGap =
index < sortedCenterXs.length - 1 ? sortedCenterXs[index + 1]! - centerX : Infinity;
const nearestGap = Math.max(1, Math.min(previousGap, nextGap));
const hitWidth = Math.min(diamondSize, nearestGap);
return {
hitWidth,
visualSize: hitWidth === diamondSize ? diamondSize : Math.max(2, hitWidth - 2),
};
});
const baseColor = isSelected ? accentColor : "#a3a3a3";
const baseOpacity = isSelected ? 0.4 : 0.25;
const canDrag = isSelected && !!onMoveKeyframe;
@@ -240,11 +249,14 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
}}
>
{sorted.map((kf, i) => {
const prev = sorted[i - 1];
if (!prev) return null;
const x1 = Math.max(0, Math.min(clipWidthPx, (prev.percentage / 100) * clipWidthPx));
const x2 = Math.max(0, Math.min(clipWidthPx, (kf.percentage / 100) * clipWidthPx));
if (i === 0) return null;
const prev = sorted[i - 1]!;
const x1 = sortedCenterXs[i - 1]!;
const x2 = sortedCenterXs[i]!;
if (x2 - x1 < 1) return null;
const connectorLeft = x1 + markerMetrics[i - 1]!.visualSize / 2;
const connectorWidth =
x2 - x1 - markerMetrics[i - 1]!.visualSize / 2 - markerMetrics[i]!.visualSize / 2;
// Group-aware target for the ease button: the segment ease is
// per-keyframe (each keyframe carries its own animationId/tweenPercentage).
// On a merged inline row the button is hidden where the segment is
@@ -259,9 +271,9 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
className="absolute"
data-keyframe-connector={groupAware ? "" : undefined}
style={{
left: x1,
left: connectorLeft,
top: centerY,
width: x2 - x1,
width: Math.max(0, connectorWidth),
height: 2,
transform: "translateY(-1px)",
background: baseColor,
@@ -271,7 +283,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
/>
{onSelectSegment && !kf.easeAmbiguous && kf.animationId !== undefined && (
<div
className="absolute"
className="group absolute"
data-keyframe-ease-segment=""
style={{
left: x1,
@@ -279,40 +291,43 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
width: x2 - x1,
height: 18,
transform: "translateY(-50%)",
pointerEvents: "auto",
// Own a stacking context above the diamond buttons. At fit
// zoom the 16px ease control can overlap its neighbouring
// diamond; without a z-index here the later diamond wins the
// hit test even though the child button has z-index 3.
zIndex: 3,
// Only the centered control is interactive. The transparent
// segment wrapper must not swallow connector/clip gestures.
pointerEvents: "none",
}}
onMouseEnter={() => setHoveredSegment(i)}
onMouseLeave={() => setHoveredSegment((h) => (h === i ? null : h))}
>
{hoveredSegment === i && (
<button
type="button"
data-keyframe-ease-button=""
aria-label={`Edit ${ease} easing`}
title={`Edit ${ease} easing`}
className="absolute flex items-center justify-center rounded"
style={{
left: "50%",
top: "50%",
width: 16,
height: 16,
transform: "translate(-50%, -50%)",
zIndex: 3,
pointerEvents: "auto",
padding: 0,
border: "1px solid rgba(255, 255, 255, 0.14)",
background: "#171717",
cursor: "pointer",
}}
onPointerDown={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation();
onSelectSegment(target);
}}
>
<MiniCurveSvg ease={ease} active size={12} />
</button>
)}
<button
type="button"
data-keyframe-ease-button=""
aria-label={`Edit ${ease} easing`}
title={`Edit ${ease} easing`}
className="absolute flex items-center justify-center rounded opacity-0 transition-opacity group-hover:opacity-100 focus-visible:opacity-100"
style={{
left: "50%",
top: "50%",
width: 16,
height: 16,
transform: "translate(-50%, -50%)",
zIndex: 3,
pointerEvents: "auto",
padding: 0,
border: "1px solid rgba(255, 255, 255, 0.14)",
background: "#171717",
cursor: "pointer",
}}
onPointerDown={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation();
onSelectSegment(target);
}}
>
<MiniCurveSvg ease={ease} active size={12} />
</button>
</div>
)}
</Fragment>
@@ -324,12 +339,13 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
const kfKey = timelineKeyframeSelectionKey(elementId, target);
// While dragging this diamond, render it at the live preview clip-%.
const renderPct = preview?.kfKey === kfKey ? preview.clipPct : kf.percentage;
// Center the diamond ON its keyframe %: left = (% · width) half, so the
// diamond's midpoint sits exactly on the playhead/ruler x for that time.
// Center the marker's non-overlapping hit region ON its keyframe %, so
// the diamond's midpoint sits exactly on the playhead/ruler x for that time.
// The 0% diamond's left half lands in the reserved left gutter (the
// content origin is inset past the label column, Figma-style) so it stays
// fully visible instead of being clipped by the sticky label column.
const leftPx = (renderPct / 100) * clipWidthPx - half;
const marker = markerMetrics[i]!;
const leftPx = (renderPct / 100) * clipWidthPx - marker.hitWidth / 2;
const isKfSelected = selectedKeyframes.has(kfKey);
const atPlayhead = isSelected && Math.abs(kf.percentage - currentPercentage) < 0.5;
const isHighlighted = isKfSelected || atPlayhead;
@@ -480,7 +496,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
left: leftPx,
top: centerY,
transform: "translateY(-50%)",
width: diamondSize,
width: marker.hitWidth,
height: diamondSize,
zIndex: isHighlighted ? 2 : 1,
pointerEvents: "auto",
@@ -489,6 +505,10 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
cursor: canDrag ? "ew-resize" : "pointer",
padding: 0,
touchAction: "none",
display: "flex",
alignItems: "center",
justifyContent: "center",
overflow: "visible",
}}
onPointerDown={onPointerDown}
onPointerMove={onPointerMove}
@@ -510,7 +530,12 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({
}}
title={`${kf.percentage}%`}
>
<svg width={diamondSize} height={diamondSize} viewBox="0 0 10 10">
<svg
width={marker.visualSize}
height={marker.visualSize}
viewBox="0 0 10 10"
style={{ flexShrink: 0, pointerEvents: "none" }}
>
{isKfSelected && (
<path
d="M5 0L10 5L5 10L0 5Z"
@@ -112,13 +112,7 @@ function laneEaseSegments(host: HTMLElement, group: string): HTMLElement[] {
);
}
// The mid-segment ease button is revealed on hover (Figma parity), so tests must
// hover the segment strip before its button exists. React derives onMouseEnter
// from a bubbling mouseover, so dispatching that is what arms the hover.
function revealEaseButton(segment: HTMLElement): HTMLButtonElement | null {
act(() => {
segment.dispatchEvent(new MouseEvent("mouseover", { bubbles: true }));
});
return segment.querySelector<HTMLButtonElement>("button[data-keyframe-ease-button]");
}
@@ -281,7 +275,7 @@ describe("TimelinePropertyLanes", () => {
act(() => root.unmount());
});
it("reveals one midpoint ease button per segment on hover, regardless of selection", () => {
it("keeps one accessible midpoint ease button per segment, regardless of selection", () => {
const animations = [
animation("position-tween", "position", [
{ percentage: 0, properties: { x: 0 } },
@@ -295,12 +289,15 @@ describe("TimelinePropertyLanes", () => {
expect(segments).toHaveLength(2);
expect(segments.map((segment) => segment.style.left)).toEqual(["0px", "100px"]);
expect(laneDiamonds(host, "position")).toHaveLength(3);
// Resting state: no button until a segment is hovered.
expect(laneEaseButtons(host, "position")).toHaveLength(0);
// Hovering reveals exactly one button — the hovered segment's.
expect(revealEaseButton(segments[0]!)).not.toBeNull();
expect(laneEaseButtons(host, "position")).toHaveLength(1);
const buttons = laneEaseButtons(host, "position");
expect(buttons).toHaveLength(2);
expect(buttons.every((button) => button.classList.contains("opacity-0"))).toBe(true);
expect(buttons.every((button) => button.classList.contains("group-hover:opacity-100"))).toBe(
true,
);
expect(buttons.every((button) => button.classList.contains("focus-visible:opacity-100"))).toBe(
true,
);
// The ease button is available on hover even when the element is NOT selected
// (a lane shows for the track's active/primary clip, not only the selected one).
@@ -73,9 +73,9 @@ export interface TimelineEditCallbacks {
onRazorSplit?: (element: TimelineElement, splitTime: number) => Promise<void> | void;
onRazorSplitAll?: (splitTime: number) => Promise<void> | void;
onDeleteKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
onDeleteAllKeyframes?: (elementId: string) => void;
onDeleteAllKeyframes?: (element: TimelineElement) => void;
onChangeKeyframeEase?: (elementId: string, percentage: number, ease: string) => void;
onMoveKeyframeToPlayhead?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
onMoveKeyframeToPlayhead?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void;
/** Drag-to-retime: `keyframe` identifies the dragged keyframe (its percentage
* is clip-relative), `toClipPercentage` is the neighbour-clamped drop. */
onMoveKeyframe?: (
@@ -0,0 +1,42 @@
import { describe, expect, it } from "vitest";
import {
timelineKeyframeSelectionKey,
timelineKeyframeTargetFromSelectionKey,
} from "./timelineKeyframeIdentity";
describe("timeline keyframe selection identity", () => {
it("round-trips an expanded lane with colon-bearing identities", () => {
const key = timelineKeyframeSelectionKey("comp#a:child", {
percentage: 75,
tweenPercentage: 40,
propertyGroup: "position",
animationId: "child:position",
});
expect(timelineKeyframeTargetFromSelectionKey("comp#a:child", key)).toEqual({
percentage: 75,
tweenPercentage: 40,
propertyGroup: "position",
animationId: "child:position",
});
});
it("does not confuse an expanded lane whose element id extends the active id", () => {
const key = timelineKeyframeSelectionKey("comp#a:child", {
percentage: 75,
tweenPercentage: 40,
propertyGroup: "position",
animationId: "child-position",
});
expect(timelineKeyframeTargetFromSelectionKey("comp#a", key)).toBeNull();
});
it("retains the collapsed key fallback and rejects malformed percentages", () => {
expect(timelineKeyframeTargetFromSelectionKey("comp#a", "comp#a:30")).toEqual({
percentage: 30,
});
expect(timelineKeyframeTargetFromSelectionKey("comp#a", "comp#a:NaN")).toBeNull();
expect(timelineKeyframeTargetFromSelectionKey("comp#a", "comp#b:30")).toBeNull();
});
});
@@ -10,8 +10,50 @@ export function timelineKeyframeSelectionKey(
target: TimelineKeyframeTarget,
): string {
if (!target.propertyGroup) return `${elementId}:${target.percentage}`;
const groupKey = target.animationId
? `${target.propertyGroup}:${target.animationId}`
: target.propertyGroup;
return `${elementId}:${groupKey}:${target.percentage}`;
return JSON.stringify([
elementId,
target.propertyGroup,
target.animationId ?? "",
target.percentage,
target.tweenPercentage ?? target.percentage,
]);
}
export function timelineKeyframeTargetFromSelectionKey(
elementId: string,
key: string,
): TimelineKeyframeTarget | null {
if (key.startsWith("[")) {
let decoded: unknown;
try {
decoded = JSON.parse(key);
} catch {
return null;
}
if (!Array.isArray(decoded) || decoded.length !== 5) return null;
const [selectedElementId, propertyGroup, animationId, percentage, tweenPercentage] = decoded;
if (
selectedElementId !== elementId ||
typeof propertyGroup !== "string" ||
propertyGroup.length === 0 ||
typeof animationId !== "string" ||
typeof percentage !== "number" ||
!Number.isFinite(percentage) ||
typeof tweenPercentage !== "number" ||
!Number.isFinite(tweenPercentage)
) {
return null;
}
return {
propertyGroup,
animationId: animationId || undefined,
percentage,
tweenPercentage,
};
}
const separator = key.lastIndexOf(":");
if (separator < 0 || key.slice(0, separator) !== elementId) return null;
const percentage = Number(key.slice(separator + 1));
return Number.isFinite(percentage) ? { percentage } : null;
}
@@ -77,10 +77,9 @@ export function useTimelineKeyframeHandlers({
const onContextMenuKeyframe = useCallback(
(e: ReactMouseEvent, elId: string, target: TimelineKeyframeTarget) => {
const el = expandedElements.find((item) => (item.key ?? item.id) === elId);
if (el) {
setSelectedElementId(elId);
onSelectElement?.(el);
}
if (!el) return;
setSelectedElementId(elId);
onSelectElement?.(el);
const kfData = keyframeCache.get(elId);
const kf = kfData?.keyframes.find(
(item) => Math.abs(item.percentage - target.percentage) < 0.2,
@@ -93,6 +92,7 @@ export function useTimelineKeyframeHandlers({
tweenPercentage: target.tweenPercentage ?? kf?.tweenPercentage,
propertyGroup: target.propertyGroup,
animationId: target.animationId,
element: el,
currentEase: kf?.ease ?? kfData?.ease,
});
},