mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-03 12:54:29 +00:00
fix(studio): retime flat tween boundaries and target the clicked keyframe's own tween
Two more regressions the QA triage attributed to this stack. Dragging a flat tween's boundary diamond did nothing: the handler bailed on `!anim.keyframes` even though resolveKeyframeRetime already resolves that case to a position/duration resize. The empty remap now dispatches update-meta, which moves the window without rewriting the authored flat tween into keyframes form (what the keyframed-resize writer would do as a side effect). The property panel's keyframe gutter guessed one animation per property group, so clicking a keyframe authored by a sibling tween on a merged row named the wrong tween and the writer silently removed nothing. The diamond now reports the clicked keyframe's own animationId, with the group guess kept as the fallback for cache rows that carry no identity.
This commit is contained in:
@@ -11,12 +11,15 @@ interface KeyframeNavigationProps {
|
|||||||
tweenPercentage?: number;
|
tweenPercentage?: number;
|
||||||
properties: Record<string, number | string>;
|
properties: Record<string, number | string>;
|
||||||
ease?: string;
|
ease?: string;
|
||||||
|
/** The tween that authored this keyframe, when the cache knows it. */
|
||||||
|
animationId?: string;
|
||||||
}> | null;
|
}> | null;
|
||||||
/** Current playhead percentage within the element's lifetime (0-100) */
|
/** Current playhead percentage within the element's lifetime (0-100) */
|
||||||
currentPercentage: number;
|
currentPercentage: number;
|
||||||
onSeek: (percentage: number) => void;
|
onSeek: (percentage: number) => void;
|
||||||
onAddKeyframe: (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;
|
onConvertToKeyframes: () => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -152,7 +155,12 @@ export const KeyframeNavigation = memo(function KeyframeNavigation({
|
|||||||
if (diamondState === "ghost") {
|
if (diamondState === "ghost") {
|
||||||
onConvertToKeyframes();
|
onConvertToKeyframes();
|
||||||
} else if (diamondState === "active" && atCurrent) {
|
} 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 {
|
} else {
|
||||||
onAddKeyframe(clipToTweenPercentage(propertyKeyframes, currentPercentage));
|
onAddKeyframe(clipToTweenPercentage(propertyKeyframes, currentPercentage));
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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(
|
||||||
|
<KeyframeNavigation
|
||||||
|
property="x"
|
||||||
|
keyframes={MERGED_ROW}
|
||||||
|
currentPercentage={currentPercentage}
|
||||||
|
onSeek={vi.fn()}
|
||||||
|
onAddKeyframe={vi.fn()}
|
||||||
|
onRemoveKeyframe={onRemoveKeyframe as never}
|
||||||
|
onConvertToKeyframes={vi.fn()}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
const diamond = document.querySelector<HTMLElement>('[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");
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -408,7 +408,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro
|
|||||||
onCommitAnimatedProperty &&
|
onCommitAnimatedProperty &&
|
||||||
void onCommitAnimatedProperty(element, "x", displayX)
|
void onCommitAnimatedProperty(element, "x", displayX)
|
||||||
}
|
}
|
||||||
onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("x"), pct)}
|
onRemoveKeyframe={(pct, animationId) =>
|
||||||
|
onRemoveKeyframe?.(animationId ?? animIdForProp("x"), pct)
|
||||||
|
}
|
||||||
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("x"))}
|
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("x"))}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
@@ -433,7 +435,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro
|
|||||||
onCommitAnimatedProperty &&
|
onCommitAnimatedProperty &&
|
||||||
void onCommitAnimatedProperty(element, "y", displayY)
|
void onCommitAnimatedProperty(element, "y", displayY)
|
||||||
}
|
}
|
||||||
onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("y"), pct)}
|
onRemoveKeyframe={(pct, animationId) =>
|
||||||
|
onRemoveKeyframe?.(animationId ?? animIdForProp("y"), pct)
|
||||||
|
}
|
||||||
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("y"))}
|
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("y"))}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
@@ -458,7 +462,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro
|
|||||||
onCommitAnimatedProperty &&
|
onCommitAnimatedProperty &&
|
||||||
void onCommitAnimatedProperty(element, "width", displayW)
|
void onCommitAnimatedProperty(element, "width", displayW)
|
||||||
}
|
}
|
||||||
onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("width"), pct)}
|
onRemoveKeyframe={(pct, animationId) =>
|
||||||
|
onRemoveKeyframe?.(animationId ?? animIdForProp("width"), pct)
|
||||||
|
}
|
||||||
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("width"))}
|
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("width"))}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
@@ -483,7 +489,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro
|
|||||||
onCommitAnimatedProperty &&
|
onCommitAnimatedProperty &&
|
||||||
void onCommitAnimatedProperty(element, "height", displayH)
|
void onCommitAnimatedProperty(element, "height", displayH)
|
||||||
}
|
}
|
||||||
onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("height"), pct)}
|
onRemoveKeyframe={(pct, animationId) =>
|
||||||
|
onRemoveKeyframe?.(animationId ?? animIdForProp("height"), pct)
|
||||||
|
}
|
||||||
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("height"))}
|
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("height"))}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
@@ -507,7 +515,9 @@ export const PropertyPanel = memo(function PropertyPanel(props: PropertyPanelPro
|
|||||||
onCommitAnimatedProperty &&
|
onCommitAnimatedProperty &&
|
||||||
void onCommitAnimatedProperty(element, "rotation", displayR)
|
void onCommitAnimatedProperty(element, "rotation", displayR)
|
||||||
}
|
}
|
||||||
onRemoveKeyframe={(pct) => onRemoveKeyframe?.(animIdForProp("rotation"), pct)}
|
onRemoveKeyframe={(pct, animationId) =>
|
||||||
|
onRemoveKeyframe?.(animationId ?? animIdForProp("rotation"), pct)
|
||||||
|
}
|
||||||
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("rotation"))}
|
onConvertToKeyframes={() => onConvertToKeyframes?.(animIdForProp("rotation"))}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
|
|||||||
@@ -83,10 +83,10 @@ function KeyframeGutter({
|
|||||||
track("button", `Add ${property} keyframe`);
|
track("button", `Add ${property} keyframe`);
|
||||||
void onCommitAnimatedProperty(element, property, displayValue);
|
void onCommitAnimatedProperty(element, property, displayValue);
|
||||||
}}
|
}}
|
||||||
onRemoveKeyframe={(pct) => {
|
onRemoveKeyframe={(pct, animationId) => {
|
||||||
if (!onRemoveKeyframe) return;
|
if (!onRemoveKeyframe) return;
|
||||||
track("button", `Remove ${property} keyframe`);
|
track("button", `Remove ${property} keyframe`);
|
||||||
onRemoveKeyframe(animIdForProp(property), pct);
|
onRemoveKeyframe(animationId ?? animIdForProp(property), pct);
|
||||||
}}
|
}}
|
||||||
onConvertToKeyframes={() => {
|
onConvertToKeyframes={() => {
|
||||||
if (!onConvertToKeyframes) return;
|
if (!onConvertToKeyframes) return;
|
||||||
|
|||||||
@@ -184,7 +184,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
|
|||||||
view.unmount();
|
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();
|
const view = renderCallbacks();
|
||||||
|
|
||||||
await expect(
|
await expect(
|
||||||
@@ -198,9 +198,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
|
|||||||
},
|
},
|
||||||
25,
|
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();
|
expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled();
|
||||||
|
// resize-keyframed-tween would convert the flat tween to keyframes form.
|
||||||
expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled();
|
expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled();
|
||||||
view.unmount();
|
view.unmount();
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -256,10 +256,6 @@ export function useTimelineEditCallbacks({
|
|||||||
const anim = animations.find((a) => a.id === target.animId);
|
const anim = animations.find((a) => a.id === target.animId);
|
||||||
const tweenStart = anim ? resolveTweenStart(anim) : null;
|
const tweenStart = anim ? resolveTweenStart(anim) : null;
|
||||||
if (!anim || tweenStart === null) return Promise.resolve(false);
|
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 sourceFile = sel.sourceFile || activeCompPath || "index.html";
|
||||||
const { elements, domClipChildren } = usePlayerStore.getState();
|
const { elements, domClipChildren } = usePlayerStore.getState();
|
||||||
const { elStart, elDuration } = resolveClipTimingBasis(
|
const { elStart, elDuration } = resolveClipTimingBasis(
|
||||||
@@ -285,6 +281,19 @@ export function useTimelineEditCallbacks({
|
|||||||
decision.position != null &&
|
decision.position != null &&
|
||||||
decision.duration != 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(
|
return handleGsapResizeKeyframedTween(
|
||||||
target.animId,
|
target.animId,
|
||||||
decision.position,
|
decision.position,
|
||||||
|
|||||||
Reference in New Issue
Block a user