Skip to content

fix(parsers): preserve authored keyframe intent #2582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
miguel-heygen wants to merge 1 commit into
base: codex/pr2387-ease-mode-switch-final
Choose a base branch
from
Draft

fix(parsers): preserve authored keyframe intent #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions packages/parsers/src/gsapParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
serializeValue as valueToCode,
safeJsKey as safeKey,
resolveConversionProps,
mergePercentageKeyframes,
} from "./gsapSerialize";

export type {
Expand Down Expand Up @@ -1961,7 +1962,7 @@ function buildKeyframeObjectCode(
}>,
options?: { easeEach?: string },
): string {
const entries = keyframes.map((kf) => {
const entries = mergePercentageKeyframes(keyframes).map((kf) => {
const props = keyframePropsToCode(kf);
if (kf.ease) props.push(`ease: ${JSON.stringify(kf.ease)}`);
if (kf.auto) props.push(`_auto: 1`);
Expand Down Expand Up @@ -2355,10 +2356,9 @@ export function removeKeyframeFromScript(
/**
* Retime a keyframe: move the keyframe at `fromPercentage` to `toPercentage`,
* PRESERVING its properties and per-keyframe ease (the Studio "Move to Playhead"
* gesture). Re-sorts keyframes by percentage. If a keyframe already exists at
* `toPercentage`, it is overwritten by the moved one (no duplicate). No-op when
* the animation/keyframe isn't found, the tween has no object-form keyframes, or
* the move resolves onto the same keyframe. Acorn twin: moveKeyframeInScript.
* gesture). Re-sorts keyframes by percentage. No-op when the animation/keyframe
* isn't found, the tween has no object-form keyframes, the move resolves onto the
* same keyframe, or the destination is occupied. Acorn twin: moveKeyframeInScript.
*/
export function moveKeyframeInScript(
script: string,
Expand All @@ -2382,19 +2382,19 @@ export function moveKeyframeInScript(
// retime, because findKeyframePropByPct resolves the destination back onto the
// from-keyframe — so a deliberate 1% drag committed nothing. Acorn twin too.
if (Math.abs(fromPercentage - toPercentage) < MOVE_NOOP_EPSILON_PCT) return script;
// A destination keyframe is only a real collision (overwrite) when it's a
// DIFFERENT keyframe; resolving back onto the from-keyframe is not.
// Never overwrite another authored keyframe. Resolving the destination back
// onto the source keyframe is not a collision (the tolerance is intentionally
// wider than MOVE_NOOP_EPSILON_PCT).
const dest = findKeyframePropByPct(kfNode, toPercentage);
const collision = dest && dest.prop !== match.prop ? dest : null;
if (dest && dest.prop !== match.prop) return script;

// Reuse each keyframe's value node verbatim (preserves properties +
// per-keyframe ease + _auto). Drop the moved keyframe (and any destination
// keyframe it overwrites), re-key the moved value to toPercentage, then re-sort.
// per-keyframe ease + _auto). Drop the moved keyframe, re-key its value to
// toPercentage, then re-sort.
const movedValue = match.prop.value;
const entries: Array<{ pct: number; value: AstNode }> = [];
for (const prop of filterPercentageProps(kfNode)) {
if (prop === match.prop) continue;
if (collision && prop === collision.prop) continue;
const pct = percentageFromKey(propKeyName(prop) ?? "");
if (Number.isNaN(pct)) continue;
entries.push({ pct, value: prop.value });
Expand Down Expand Up @@ -2446,7 +2446,11 @@ export function resizeKeyframedTweenInScript(
match.prop.key = parseExpr(`{ ${JSON.stringify(`${to}%`)}: 0 }`).properties[0].key;
}

applyUpdatesToCall(loc.target.call, { position: newPosition, duration: newDuration });
const durationAuthored = findPropertyNode(loc.target.call.varsArg, "duration") !== undefined;
applyUpdatesToCall(loc.target.call, {
position: newPosition,
...(durationAuthored ? { duration: newDuration } : {}),
});
return recast.print(loc.parsed.ast).code;
}

Expand Down
29 changes: 29 additions & 0 deletions packages/parsers/src/gsapSerialize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,35 @@ export interface GsapPercentageKeyframe {
ease?: string;
}

export interface WritableGsapPercentageKeyframe extends GsapPercentageKeyframe {
auto?: boolean;
}

/**
* Collapse duplicate percentage entries before serializing an object literal.
* Matches addKeyframeToScript's merge contract: later properties/ease win while
* unrelated authored properties, an earlier ease, and endpoint auto markers survive.
*/
export function mergePercentageKeyframes(
keyframes: readonly WritableGsapPercentageKeyframe[],
): WritableGsapPercentageKeyframe[] {
const byPercentage = new Map<number, WritableGsapPercentageKeyframe>();
for (const keyframe of keyframes) {
const existing = byPercentage.get(keyframe.percentage);
if (!existing) {
byPercentage.set(keyframe.percentage, {
...keyframe,
properties: { ...keyframe.properties },
});
continue;
}
existing.properties = { ...existing.properties, ...keyframe.properties };
if (keyframe.ease !== undefined) existing.ease = keyframe.ease;
if (keyframe.auto) existing.auto = true;
}
return [...byPercentage.values()];
}

export type GsapKeyframeFormat = "percentage" | "object-array" | "simple-array";

export interface GsapKeyframesData {
Expand Down
75 changes: 63 additions & 12 deletions packages/parsers/src/gsapWriter.parity.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,8 +1124,9 @@ describe("parity: updateKeyframeInScript (recast vs acorn)", () => {
// ── moveKeyframeInScript (retime: preserve value + ease) ─────────────────────
// "Move to Playhead" retimes a keyframe in time, keeping its properties and
// per-keyframe ease. The moved keyframe must vanish from the source percentage
// and reappear (with identical value + ease) at the destination; a destination
// collision is overwritten, not duplicated. recast and acorn must agree.
// and reappear (with identical value + ease) at the destination. An occupied
// destination rejects the retime without mutating authored data. Both writers
// must agree.
const MOVE_KF_SCRIPT = `
const tl = gsap.timeline({ paused: true });
tl.to("#box", { keyframes: { "0%": { x: 0 }, "50%": { x: 100, opacity: 0.5, ease: "power2.in" }, "100%": { x: 200 } }, duration: 1 }, 0.2);
Expand All @@ -1145,16 +1146,10 @@ describe("moveKeyframeInScript: retime preserves value + ease (acorn) ", () => {
expect(pcts).not.toContain(50);
});

it("overwrites the destination keyframe on collision (no duplicate)", () => {
it("rejects an occupied destination without mutating either writer", () => {
const id = acornId(MOVE_KF_SCRIPT);
const out = moveKeyframeAcorn(MOVE_KF_SCRIPT, id, 50, 100);
const kfs = shapeOf(out).keyframes?.keyframes ?? [];
const pcts = kfs.map((k) => k.percentage);
expect(pcts).toEqual([0, 100]);
const dest = kfs.find((k) => k.percentage === 100)!;
// The moved keyframe's value + ease replaced the old 100% { x: 200 }.
expect(dest.properties).toEqual({ x: 100, opacity: 0.5 });
expect(dest.ease).toBe("power2.in");
expect(moveKeyframeAcorn(MOVE_KF_SCRIPT, id, 50, 100)).toBe(MOVE_KF_SCRIPT);
expect(moveKeyframeRecast(MOVE_KF_SCRIPT, id, 50, 100)).toBe(MOVE_KF_SCRIPT);
});

it("no-ops only for a negligible move (below the drag epsilon)", () => {
Expand Down Expand Up @@ -1203,7 +1198,7 @@ describe("parity: moveKeyframeInScript (recast vs acorn)", () => {
expectParity(MOVE_KF_SCRIPT, 50, 10);
});

it("retime onto an existing percentage (collision overwrite)", () => {
it("reject an occupied destination", () => {
expectParity(MOVE_KF_SCRIPT, 50, 100);
});

Expand Down Expand Up @@ -1242,6 +1237,12 @@ describe("moveKeyframeInScript: array-form keyframes (recast + acorn parity)", (
modelOf(moveKeyframeRecast(KF_ADD_ARRAY_SCRIPT, id, 50, 75)),
);
});

it("leaves array-form source untouched when the destination is occupied", () => {
const id = acornId(KF_ADD_ARRAY_SCRIPT);
expect(moveKeyframeAcorn(KF_ADD_ARRAY_SCRIPT, id, 50, 100)).toBe(KF_ADD_ARRAY_SCRIPT);
expect(moveKeyframeRecast(KF_ADD_ARRAY_SCRIPT, id, 50, 100)).toBe(KF_ADD_ARRAY_SCRIPT);
});
});

// ── resizeKeyframedTweenInScript (boundary drag: re-key + grow window) ────────
Expand All @@ -1253,6 +1254,10 @@ const RESIZE_KF_SCRIPT = `
const tl = gsap.timeline({ paused: true });
tl.to("#box", { keyframes: { "0%": { opacity: 0, _auto: 1 }, "50%": { opacity: 0.5, ease: "power2.in" }, "100%": { opacity: 1, _auto: 1 }, easeEach: "power1.inOut" }, duration: 1, ease: "power3.out" }, 0.2);
`;
const RESIZE_KF_NO_DURATION_SCRIPT = `
const tl = gsap.timeline({ paused: true });
tl.to("#box", { keyframes: { "0%": { opacity: 0 }, "50%": { opacity: 0.5, ease: "power2.in" }, "100%": { opacity: 1 } }, ease: "power3.out" }, 0.2);
`;
// Window [0.2, 1.2]; drag the last keyframe (abs 1.2) out to abs 2.2 → [0.2, 2.2].
// abs 0.2/0.7/2.2 over the new 2.0s window → 0 / 25 / 100.
const RESIZE_REMAP = [
Expand Down Expand Up @@ -1290,6 +1295,18 @@ describe("resizeKeyframedTweenInScript: preserves author intent (acorn + recast)
it(`${label}: no-ops on unknown id`, () => {
expect(resize(RESIZE_KF_SCRIPT, "bad-id", 0.2, 2, RESIZE_REMAP)).toBe(RESIZE_KF_SCRIPT);
});

it(`${label}: preserves an unauthored duration`, () => {
const id = acornId(RESIZE_KF_NO_DURATION_SCRIPT);
const out = resize(RESIZE_KF_NO_DURATION_SCRIPT, id, 0.4, 2, RESIZE_REMAP);
expect(out).not.toMatch(/\bduration\s*:/);
const shape = shapeOf(out);
expect(shape.duration).toBeUndefined();
expect(parseGsapScript(out).animations[0]!.position).toBeCloseTo(0.4, 5);
expect(shape.keyframes?.keyframes.map((keyframe) => keyframe.percentage)).toEqual([
0, 25, 100,
]);
});
}

it("parity: both writers reparse to the same model", () => {
Expand All @@ -1298,6 +1315,15 @@ describe("resizeKeyframedTweenInScript: preserves author intent (acorn + recast)
modelOf(resizeKeyframedTweenRecast(RESIZE_KF_SCRIPT, id, 0.2, 2, RESIZE_REMAP)),
);
});

it("parity: both writers preserve duration absence", () => {
const id = acornId(RESIZE_KF_NO_DURATION_SCRIPT);
expect(
modelOf(resizeKeyframedTweenAcorn(RESIZE_KF_NO_DURATION_SCRIPT, id, 0.4, 2, RESIZE_REMAP)),
).toEqual(
modelOf(resizeKeyframedTweenRecast(RESIZE_KF_NO_DURATION_SCRIPT, id, 0.4, 2, RESIZE_REMAP)),
);
});
});

// Regression: same array-form gap as moveKeyframeInScript above — boundary
Expand Down Expand Up @@ -1375,6 +1401,31 @@ function lastModelOf(script: string) {
return arr[arr.length - 1];
}

describe("keyframe object builders: duplicate percentage merge", () => {
const duplicateKeyframes = [
{ percentage: 0, properties: { x: 0 } },
{ percentage: 50, properties: { x: 100 }, ease: "power1.in" },
{ percentage: 50, properties: { x: 120, opacity: 0.5 }, auto: true },
{ percentage: 100, properties: { x: 200 } },
];

for (const [label, add] of [
["acorn", addWithKfAcorn],
["recast", addWithKfRecast],
] as const) {
it(`${label}: emits one merged key and preserves authored properties and ease`, () => {
const out = add(ADD_WITH_KF_BASE, "#hero", 0, 1, duplicateKeyframes).script;
expect(out.match(/["']50%["']\s*:/g)).toHaveLength(1);
const atFifty = lastModelOf(out)?.keyframes?.keyframes.filter(
(keyframe) => keyframe.percentage === 50,
);
expect(atFifty).toHaveLength(1);
expect(atFifty?.[0]?.properties).toEqual({ x: 120, opacity: 0.5, _auto: 1 });
expect(atFifty?.[0]?.ease).toBe("power1.in");
});
}
});

// NOTE (WS-3.F): recast is retired, so `recast` here is an alias of the acorn
// writer and the historical `toEqual(lastModelOf(recast))` comparisons are
// tautologies. The WS-3.C ops below instead pin the acorn output as golden
Expand Down
28 changes: 15 additions & 13 deletions packages/parsers/src/gsapWriterAcorn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
resolveConversionProps,
extractArcWaypoints,
buildMotionPathObjectCode,
mergePercentageKeyframes,
} from "./gsapSerialize.js";
import {
parseGsapScriptAcornForWrite,
Expand Down Expand Up @@ -1271,10 +1272,9 @@ export function removeKeyframeFromScript(
/**
* Retime a keyframe: move the keyframe at `fromPercentage` to `toPercentage`,
* PRESERVING its properties and per-keyframe ease (the Studio "Move to Playhead"
* gesture). Re-sorts keyframes by percentage. If a keyframe already exists at
* `toPercentage`, it is overwritten by the moved one (no duplicate). No-op when
* the animation/keyframe isn't found, the tween has no object-form keyframes, or
* the move resolves onto the same keyframe.
* gesture). Re-sorts keyframes by percentage. No-op when the animation/keyframe
* isn't found, the tween has no object-form keyframes, the move resolves onto the
* same keyframe, or the destination is occupied.
*/
export function moveKeyframeInScript(
script: string,
Expand All @@ -1296,18 +1296,18 @@ export function moveKeyframeInScript(
// retime, because findKfPropByPct resolves the destination back onto the
// from-keyframe — so a deliberate 1% drag committed nothing.
if (Math.abs(fromPercentage - toPercentage) < MOVE_NOOP_EPSILON_PCT) return src;
// A destination keyframe is only a real collision (overwrite) when it's a
// DIFFERENT keyframe; resolving back onto the from-keyframe is not.
// Never overwrite another authored keyframe. Resolving the destination back
// onto the source keyframe is not a collision (the tolerance is intentionally
// wider than MOVE_NOOP_EPSILON_PCT).
const dest = findKfPropByPct(kfNode, toPercentage);
const collision = dest && dest.prop !== match.prop ? dest : null;
if (dest && dest.prop !== match.prop) return script;

// Rebuild the keyframes object: drop the moved keyframe (and any keyframe at
// the destination it overwrites), re-key the moved record to toPercentage,
// then re-sort. recordToCode round-trips properties + per-keyframe ease + _auto.
// Rebuild the keyframes object: drop the moved keyframe, re-key the moved
// record to toPercentage, then re-sort. recordToCode round-trips properties +
// per-keyframe ease + _auto.
const entries: Array<{ pct: number; record: Record<string, number | string> }> = [];
for (const prop of percentagePropsOf(kfNode)) {
if (prop === match.prop) continue;
if (collision && prop === collision.prop) continue;
const pct = percentageFromKey(propKeyName(prop) ?? "");
if (Number.isNaN(pct)) continue;
entries.push({ pct, record: valueNodeToRecord(prop.value, src) });
Expand Down Expand Up @@ -1365,7 +1365,9 @@ export function resizeKeyframedTweenInScript(
ms.overwrite(keyNode.start, keyNode.end, JSON.stringify(`${to}%`));
}
overwritePosition(ms, target.call, newPosition);
upsertProp(ms, target.call.varsArg, "duration", newDuration);
if (findPropertyNode(target.call.varsArg, "duration")) {
upsertProp(ms, target.call.varsArg, "duration", newDuration);
}
return ms.toString();
}

Expand Down Expand Up @@ -1555,7 +1557,7 @@ function buildKeyframeObjectCode(
}>,
easeEach?: string,
): string {
const entries = keyframes.map((kf) => {
const entries = mergePercentageKeyframes(keyframes).map((kf) => {
const props = Object.entries(kf.properties).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`);
if (kf.ease) props.push(`ease: ${JSON.stringify(kf.ease)}`);
if (kf.auto) props.push(`_auto: 1`);
Expand Down
Loading