fix(studio): static-position drag no longer freezes an element beside an animated rotation (#2016)

## What

Brief description of the change.

## Why

Why is this change needed?

## How

How was this implemented? Any notable design decisions?

## Test plan

How was this tested?

- [ ] Unit tests added/updated
- [ ] Manual testing performed
- [ ] Documentation updated (if applicable)
This commit is contained in:
Miguel Ángel
2026-07-07 03:45:17 -04:00
committed by GitHub
parent 306a291dea
commit 5d59835446
8 changed files with 280 additions and 112 deletions
@@ -5,6 +5,7 @@ import { commitGsapPositionFromDrag } from "./gsapDragPositionCommit";
import {
commitStaticGsapPosition,
commitStaticGsapRotation,
findExistingPositionWrite,
parkPlayheadOnKeyframe,
type GsapDragCommitCallbacks,
} from "./gsapDragCommit";
@@ -75,6 +76,7 @@ describe("commitGsapPositionFromDrag — flat tween", () => {
});
it("extends the existing tween (never spawns a parallel one) when dragged OUTSIDE its range", async () => {
// fallow-ignore-next-line code-duplication
usePlayerStore.setState({ currentTime: 6 }); // outside [1.2, 3.4]
const { types, callbacks } = recordingCallbacks();
@@ -94,7 +96,9 @@ describe("commitGsapPositionFromDrag — flat tween", () => {
});
it("adds a keyframe at the playhead when dragged INSIDE its range", async () => {
// fallow-ignore-next-line code-duplication
usePlayerStore.setState({ currentTime: 2 }); // inside [1.2, 3.4]
// fallow-ignore-next-line code-duplication
const { types, callbacks } = recordingCallbacks();
await commitGsapPositionFromDrag(
@@ -115,6 +119,7 @@ describe("commitGsapPositionFromDrag — flat tween", () => {
// User clicked the 100% diamond (activeKeyframePct=100), playhead drifted past
// the end. Expect: convert + add-keyframe AT 100% — not replace-with-keyframes.
usePlayerStore.setState({ currentTime: 6, activeKeyframePct: 100 }); // outside [1.2, 3.4]
// fallow-ignore-next-line code-duplication
const { types, mutations, callbacks } = recordingCallbacks();
await commitGsapPositionFromDrag(
@@ -338,6 +343,7 @@ describe("commitStaticGsapPosition — instantPatch (value-only set)", () => {
await commitStaticGsapPosition(
selection(),
{ x: -50, y: 30 },
// fallow-ignore-next-line code-duplication
{ x: 0, y: 0 },
"#puck-a",
null, // no existing set → `add` a new base gsap.set
@@ -352,6 +358,55 @@ describe("commitStaticGsapPosition — instantPatch (value-only set)", () => {
});
});
// A degenerate `tl.to("#el",{keyframes:{...},duration:0})` — what a pre-fix drag
// left behind when it routed a STATIC position hold (sitting beside a keyframed
// rotation) into the keyframe branch. A duration-0 keyframed tween renders its
// final keyframe at every playhead, so the element froze and "couldn't move".
const keyframedZeroDurationHold = (): GsapAnimation =>
({
id: "#puck-a-frozen",
targetSelector: "#puck-a",
method: "to",
propertyGroup: "position",
duration: 0,
keyframes: {
keyframes: [
{ percentage: 0, properties: { x: 100, y: 50 } },
{ percentage: 100, properties: { x: -260, y: -70 } },
],
},
properties: {},
}) as unknown as GsapAnimation;
describe("static position hold recognition + heal (frozen duration-0 keyframed tween)", () => {
beforeEach(() => usePlayerStore.setState({ currentTime: 0, activeKeyframePct: null }));
it("findExistingPositionWrite recognizes a keyframed zero-duration position hold", () => {
const found = findExistingPositionWrite([keyframedZeroDurationHold()], "#puck-a");
expect(found?.id).toBe("#puck-a-frozen");
});
it("commitStaticGsapPosition heals a keyframed hold by delete + clean add-set (never update-property)", async () => {
const { commits, callbacks } = optionRecordingCallbacks();
await commitStaticGsapPosition(
selection(),
{ x: -50, y: 30 },
{ x: 0, y: 0 },
"#puck-a",
keyframedZeroDurationHold(),
callbacks,
);
const types = commits.map((c) => c.mutation.type);
// Can't update-property into keyframes — must delete the frozen tween and
// write a clean static set, so the element becomes freely movable.
expect(types).toEqual(["delete", "add"]);
expect(types).not.toContain("update-property");
expect((commits[1].mutation as { method?: string }).method).toBe("set");
});
});
describe("commitStaticGsapRotation — instantPatch (value-only set)", () => {
beforeEach(() => usePlayerStore.setState({ currentTime: 0, activeKeyframePct: null }));
@@ -377,6 +432,7 @@ describe("commitStaticGsapRotation — instantPatch (value-only set)", () => {
it("ADDS a global gsap.set with a global-set instantPatch (off-timeline, no flash)", async () => {
const { commits, callbacks } = optionRecordingCallbacks();
// fallow-ignore-next-line code-duplication
await commitStaticGsapRotation(selection(), 42, "#puck-a", null, callbacks);
expect(commits).toHaveLength(1);
+41 -109
View File
@@ -14,7 +14,16 @@ import { resolveTweenStart, resolveTweenDuration } from "../utils/globalTimeComp
import { roundTo3 } from "../utils/rounding";
import { computeElementPercentage } from "./gsapShared";
import { computeDraggedGsapPosition } from "./draggedGsapPosition";
import type { RuntimeTweenChange, SetPatchProps } from "./gsapRuntimePatch";
import type { RuntimeTweenChange } from "./gsapRuntimePatch";
import {
setPatchFromUpdateProperties,
setPatchFromUpdateProperty,
} from "./gsapDragStaticSetHelpers";
export {
findExistingPositionWrite,
findRotationSetAnimation,
findSizeSetAnimation,
} from "./gsapDragStaticSetHelpers";
export interface GsapDragCommitCallbacks {
commitMutation: (
selection: DomEditSelection,
@@ -103,86 +112,6 @@ export async function materializeIfDynamic(
// ── Drag → GSAP position math ──────────────────────────────────────────────
/** The shape of an `update-property` mutation a static-set nudge POSTs. */
interface UpdatePropertyMutation {
type: "update-property";
animationId: string;
property: string;
value: number;
}
/**
* Build the `instantPatch` for a value-only `tl.set` from the SAME
* `update-property` mutation(s) that are POSTed — so the patch can never carry a
* value the source write didn't (one source of truth). Each mutation contributes
* its `{property: value}` channel to the patch's props.
*/
function setPatchFromUpdateProperties(
selector: string,
mutations: UpdatePropertyMutation[],
global = false,
): { selector: string; change: RuntimeTweenChange } {
const props: SetPatchProps = {};
for (const m of mutations) props[m.property as keyof SetPatchProps] = m.value;
// An off-timeline `gsap.set` has no runtime tween to patch — apply it to the
// element directly. An on-timeline `tl.set` mutates its tween (so a re-seek keeps it).
return { selector, change: { kind: global ? "global-set" : "set", props } };
}
/** Single-mutation convenience over {@link setPatchFromUpdateProperties}. */
function setPatchFromUpdateProperty(
selector: string,
mutation: UpdatePropertyMutation,
global = false,
): { selector: string; change: RuntimeTweenChange } {
return setPatchFromUpdateProperties(selector, [mutation], global);
}
/**
* Find the studio position-hold `set` for a selector — a `tl.set("#el",{x,y})`
* with no duration. This is what a static-element nudge writes/updates.
*/
function findPositionSetAnimation(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
return (
animations.find(
(a) =>
a.method === "set" &&
a.targetSelector === selector &&
("x" in a.properties || "y" in a.properties),
) ?? null
);
}
/**
* Find the EXISTING static position HOLD to update for a static-hold drag. Not
* just a `set`: a degenerate `tl.to("#el",{duration:0,x,y})` (what
* remove-all-keyframes leaves behind) is a held position too, and the next drag
* must UPDATE it in place rather than append a second `gsap.set` that fights it
* (the duplicate-position-write bug). Only zero-duration holds qualify — a
* live-duration `to`/`from` is NOT a static hold (and in the static path it's a
* stale/phantom parse: re-committing it would resurrect a just-deleted tween).
* Prefers a `set` (the canonical static channel) when both forms exist.
*/
function findExistingPositionWrite(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
const set = findPositionSetAnimation(animations, selector);
if (set) return set;
return (
animations.find(
(a) =>
a.targetSelector === selector &&
a.propertyGroup === "position" &&
!a.keyframes &&
(a.duration ?? 0) === 0,
) ?? null
);
}
/**
* Commit a STATIC element drag as a `tl.set("#el",{x,y})` — the single-source
* position channel for elements with no position animation. Idempotent: a
@@ -201,6 +130,34 @@ export async function commitStaticGsapPosition(
): Promise<void> {
const { newX, newY } = computeDraggedGsapPosition(selection.element, studioOffset, gsapPos);
if (existingSet) {
if (existingSet.keyframes) {
// Keyframed zero-duration hold (drag-path corruption): can't update-property
// into keyframes — delete it and write a clean static set instead.
const coalesceKey = `gsap:heal-static:${existingSet.id}`;
await callbacks.commitMutation(
selection,
{ type: "delete", animationId: existingSet.id },
{ label: "Move layer", skipReload: true, coalesceKey },
);
await callbacks.commitMutation(
selection,
{
type: "add",
targetSelector: selector,
method: "set",
position: 0,
properties: { x: newX, y: newY },
global: true,
},
{
label: "Move layer",
softReload: true,
coalesceKey,
instantPatch: { selector, change: { kind: "global-set", props: { x: newX, y: newY } } },
},
);
return;
}
// Update in place — two single-property mutations (the API updates one prop
// per call). Coalesce them and reload only after the second lands.
const coalesceKey = `gsap:set-nudge:${existingSet.id}`;
@@ -262,19 +219,6 @@ export async function commitStaticGsapPosition(
);
}
export { findExistingPositionWrite };
function findRotationSetAnimation(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
return (
animations.find(
(a) => a.method === "set" && a.targetSelector === selector && "rotation" in a.properties,
) ?? null
);
}
/**
* Commit a STATIC element rotation as a `tl.set("#el",{rotation})` — the single-
* source rotation channel for elements with no rotation animation (mirrors
@@ -328,19 +272,6 @@ export async function commitStaticGsapRotation(
);
}
export { findRotationSetAnimation };
function findSizeSetAnimation(animations: GsapAnimation[], selector: string): GsapAnimation | null {
return (
animations.find(
(a) =>
a.method === "set" &&
a.targetSelector === selector &&
("width" in a.properties || "height" in a.properties),
) ?? null
);
}
/**
* Commit a STATIC element resize as a `tl.set("#el",{width,height})` — the
* single-source size channel for elements with no size animation (mirrors
@@ -489,8 +420,6 @@ export async function commitKeyframedSizeFromResize(
return true;
}
export { findSizeSetAnimation };
// ── Whole-path offset (plain drag on animated element) ──────────────────
/**
@@ -498,6 +427,7 @@ export { findSizeSetAnimation };
* shifts together so the animation shape is preserved and the element can't
* dart off-screen. For flat tweens (no keyframes), convert first then shift.
*/
// fallow-ignore-next-line code-duplication
// fallow-ignore-next-line complexity
export async function commitWholePathOffset(
selection: DomEditSelection,
@@ -515,6 +445,7 @@ export async function commitWholePathOffset(
gsapPos,
);
const deltaX = newX - baseGsapX;
// fallow-ignore-next-line code-duplication
const deltaY = newY - baseGsapY;
const origX = Number.parseFloat(el.getAttribute("data-hf-drag-initial-offset-x") ?? "") || 0;
const origY = Number.parseFloat(el.getAttribute("data-hf-drag-initial-offset-y") ?? "") || 0;
@@ -525,6 +456,7 @@ export async function commitWholePathOffset(
el.removeAttribute("data-hf-drag-initial-offset-y");
};
// fallow-ignore-next-line code-duplication
let effectiveAnim = anim;
if (anim.keyframes) {
const newId = await materializeIfDynamic(anim, iframe, callbacks.commitMutation, selection);
@@ -202,6 +202,7 @@ async function commitFlatViaKeyframes(
}
const coalesceKey = `gsap:convert-drag:${anim.id}`;
// fallow-ignore-next-line code-duplication
await callbacks.commitMutation(
selection,
{
@@ -229,6 +230,7 @@ async function commitFlatViaKeyframes(
if (editedSelected) parkPlayheadOnKeyframe(anim, pct);
}
// fallow-ignore-next-line code-duplication
// fallow-ignore-next-line complexity
export async function commitGsapPositionFromDrag(
selection: DomEditSelection,
@@ -240,6 +242,7 @@ export async function commitGsapPositionFromDrag(
callbacks: GsapDragCommitCallbacks,
): Promise<void> {
const el = selection.element;
// fallow-ignore-next-line code-duplication
const { newX, newY, baseGsapX, baseGsapY } = computeDraggedGsapPosition(
el,
studioOffset,
@@ -0,0 +1,109 @@
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import { resolveTweenDuration } from "../utils/globalTimeCompiler";
import type { RuntimeTweenChange, SetPatchProps } from "./gsapRuntimePatch";
/** The shape of an `update-property` mutation a static-set nudge POSTs. */
interface UpdatePropertyMutation {
type: "update-property";
animationId: string;
property: string;
value: number;
}
/**
* Build the `instantPatch` for a value-only `tl.set` from the SAME
* `update-property` mutation(s) that are POSTed — so the patch can never carry a
* value the source write didn't (one source of truth). Each mutation contributes
* its `{property: value}` channel to the patch's props.
*/
export function setPatchFromUpdateProperties(
selector: string,
mutations: UpdatePropertyMutation[],
global = false,
): { selector: string; change: RuntimeTweenChange } {
const props: SetPatchProps = {};
for (const m of mutations) props[m.property as keyof SetPatchProps] = m.value;
// An off-timeline `gsap.set` has no runtime tween to patch — apply it to the
// element directly. An on-timeline `tl.set` mutates its tween (so a re-seek keeps it).
return { selector, change: { kind: global ? "global-set" : "set", props } };
}
/** Single-mutation convenience over {@link setPatchFromUpdateProperties}. */
export function setPatchFromUpdateProperty(
selector: string,
mutation: UpdatePropertyMutation,
global = false,
): { selector: string; change: RuntimeTweenChange } {
return setPatchFromUpdateProperties(selector, [mutation], global);
}
/**
* Find the studio position-hold `set` for a selector — a `tl.set("#el",{x,y})`
* with no duration. This is what a static-element nudge writes/updates.
*/
function findPositionSetAnimation(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
return (
animations.find(
(a) =>
a.method === "set" &&
a.targetSelector === selector &&
("x" in a.properties || "y" in a.properties),
) ?? null
);
}
/**
* Find the EXISTING static position HOLD to update for a static-hold drag. Not
* just a `set`: a degenerate `tl.to("#el",{duration:0,x,y})` (what
* remove-all-keyframes leaves behind) is a held position too, and the next drag
* must UPDATE it in place rather than append a second `gsap.set` that fights it
* (the duplicate-position-write bug). Only zero-duration holds qualify — a
* live-duration `to`/`from` is NOT a static hold (and in the static path it's a
* stale/phantom parse: re-committing it would resurrect a just-deleted tween).
* A keyframed zero-duration `to` is ALSO a static hold (a drag-path corruption
* artifact) and must be recognized so the static commit normalizes it.
* Prefers a `set` (the canonical static channel) when both forms exist.
*/
export function findExistingPositionWrite(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
const set = findPositionSetAnimation(animations, selector);
if (set) return set;
return (
animations.find(
(a) =>
a.targetSelector === selector &&
a.propertyGroup === "position" &&
resolveTweenDuration(a) === 0,
) ?? null
);
}
export function findRotationSetAnimation(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
return (
animations.find(
(a) => a.method === "set" && a.targetSelector === selector && "rotation" in a.properties,
) ?? null
);
}
export function findSizeSetAnimation(
animations: GsapAnimation[],
selector: string,
): GsapAnimation | null {
return (
animations.find(
(a) =>
a.method === "set" &&
a.targetSelector === selector &&
("width" in a.properties || "height" in a.properties),
) ?? null
);
}
+23 -2
View File
@@ -38,6 +38,23 @@ import {
import { hasNonHoldTweenForElement } from "./gsapRuntimeKeyframes";
import { roundTo3 } from "../utils/rounding";
// Position channels — used to scope the "has a live position tween?" check so a
// sibling rotation/scale animation never forces a static position hold into the
// keyframe branch (which corrupts it into a frozen duration-0 keyframed tween).
const POSITION_CHANNELS = [
"x",
"y",
"xPercent",
"yPercent",
"left",
"top",
// GSAP normalizes translateX/Y to x/y at play time, but readTween reads the
// AUTHORED shape — include them so a hand-authored translateX/Y position tween
// still counts as a live position tween.
"translateX",
"translateY",
];
// ── Property-group tween resolution ───────────────────────────────────────
/**
@@ -185,7 +202,7 @@ export async function tryGsapDragIntercept(
// `tl.set("#el",{x,y})`, not a keyframe conversion: re-nudge an existing set in
// place (idempotent), else add a new one. This also covers the stale-cache
// phantom — committing a set is correct because the element genuinely has no live motion.
const hasNonHold = hasNonHoldTweenForElement(iframe, selector);
const hasNonHold = hasNonHoldTweenForElement(iframe, selector, undefined, POSITION_CHANNELS);
// A KEYFRAMED position tween — even one that's currently a flat constant ("hold",
// e.g. 0% and 100% identical) — is still an animation the user is building, so a
// drag must add/update a keyframe, NOT fall back to a static `set`. Without this,
@@ -193,7 +210,9 @@ export async function tryGsapDragIntercept(
// fights the tween (the "drag didn't create a keyframe / didn't persist" bug). The
// static path is only for elements with NO keyframed position tween (truly static,
// or just a leftover position-hold `set`).
const hasKeyframedPosTween = !!posAnim?.keyframes;
// A zero-duration keyframed tween is a static HOLD, not a live animation —
// treat it as static so the drag heals it instead of feeding it more keyframes.
const hasKeyframedPosTween = !!posAnim?.keyframes && resolveTweenDuration(posAnim) > 0;
if (!hasNonHold && !hasKeyframedPosTween) {
const existingSet =
posAnim && posAnim.method === "set" && posAnim.targetSelector === selector
@@ -362,6 +381,7 @@ export async function tryGsapResizeIntercept(
const outsideRange = ts !== null && td > 0 && (ct < ts - 0.01 || ct > ts + td + 0.01); // Convert flat tweens to keyframes only for in-range resizes.
// Outside-range uses the extend path which handles everything atomically.
if (!outsideRange) {
// fallow-ignore-next-line code-duplication
if (anim.hasUnresolvedKeyframes || anim.hasUnresolvedSelector) {
const newId = await materializeIfDynamic(anim, iframe, commitMutation, selection);
if (newId) anim = { ...anim, id: newId };
@@ -530,6 +550,7 @@ export async function tryGsapRotationIntercept(
return true;
}
// fallow-ignore-next-line code-duplication
if (anim.hasUnresolvedKeyframes || anim.hasUnresolvedSelector) {
const newId = await materializeIfDynamic(anim, iframe, commitMutation, selection);
if (newId) anim = { ...anim, id: newId };
@@ -176,6 +176,40 @@ describe("hasNonHoldTweenForElement — strict live-tween existence (drag stale-
});
});
describe("hasNonHoldTweenForElement — channel-scoped (position vs sibling rotation)", () => {
const el = { id: "puck-c" };
const rotationTween = {
targets: () => [el],
vars: { keyframes: { "0%": { rotation: 0 }, "100%": { rotation: 90 } }, duration: 1 },
duration: () => 1,
startTime: () => 0,
};
const positionTween = {
targets: () => [el],
vars: { keyframes: { "0%": { x: 0, y: 0 }, "100%": { x: 100, y: 40 } }, duration: 1 },
duration: () => 1,
startTime: () => 0,
};
it("unfiltered: a rotation-only keyframed tween counts as non-hold", () => {
expect(hasNonHoldTweenForElement(fakeIframe(el, [rotationTween]), "#puck-c")).toBe(true);
});
it("position-scoped: a sibling rotation-only tween is NOT a live position tween", () => {
// The primary fix: a keyframed rotation must not route a static position hold
// into the keyframe branch. Scoping to position channels returns false here.
expect(
hasNonHoldTweenForElement(fakeIframe(el, [rotationTween]), "#puck-c", undefined, ["x", "y"]),
).toBe(false);
});
it("position-scoped: a real keyframed position tween still counts", () => {
expect(
hasNonHoldTweenForElement(fakeIframe(el, [positionTween]), "#puck-c", undefined, ["x", "y"]),
).toBe(true);
});
});
describe("arcPathFromMotionPathValue", () => {
it("builds arc config from object form { path, curviness }", () => {
const arc = arcPathFromMotionPathValue({
@@ -237,6 +237,7 @@ export function resolveRuntimeTween(
kind: "keyframe" | "set",
compositionId?: string,
channels?: string[],
// fallow-ignore-next-line code-duplication
): ResolvedRuntimeTween | null {
const timelines = timelinesOf(iframe);
if (!timelines) return null;
@@ -261,6 +262,7 @@ export function resolveRuntimeTween(
const wantChannels = channels && channels.length > 0 ? channels : null;
let first: ResolvedRuntimeTween | null = null;
// fallow-ignore-next-line code-duplication
let channelMatch: ResolvedRuntimeTween | null = null;
for (const tlId of tlIds) {
const timeline = timelines[tlId];
@@ -316,6 +318,7 @@ export function readRuntimeKeyframes(
selector: string,
compositionId?: string,
requireChannels?: string[],
// fallow-ignore-next-line code-duplication
): ReadTween | null {
const timelines = timelinesOf(iframe);
if (!timelines) return null;
@@ -344,11 +347,13 @@ export function readRuntimeKeyframes(
// (e.g. two non-overlapping gesture recordings → two separate `to()`s). The
// overlay must draw the segment under the PLAYHEAD, not blindly the first one
// — otherwise recording a second gesture leaves the path stuck on the first.
// fallow-ignore-next-line code-duplication
let firstRead: ReadTween | null = null;
for (const tlId of tlIds) {
const timeline = timelines[tlId];
if (!timeline?.getChildren) continue;
const now = typeof timeline.time === "function" ? timeline.time() : null;
// fallow-ignore-next-line code-duplication
for (const tween of timeline.getChildren(true)) {
if (!tween.vars || !matchesElement(tween, targetEl)) continue;
const dur = typeof tween.duration === "function" ? tween.duration() : 0;
@@ -377,12 +382,17 @@ export function readRuntimeKeyframes(
* The drag's stale-parse guard needs this exact distinction after a delete-all
* only a hold may remain, and resurrecting the deleted tween from the stale parse
* must be avoided.
* When `channels` is provided, only tweens carrying one of those keyframe
* properties count as non-hold motion (e.g. position channels), so a sibling
* rotation/scale tween doesn't make a static position hold enter the keyframe
* branch.
*/
// fallow-ignore-next-line complexity
export function hasNonHoldTweenForElement(
iframe: HTMLIFrameElement | null,
selector: string,
compositionId?: string,
channels?: string[],
): boolean {
const timelines = timelinesOf(iframe);
if (!timelines) return false;
@@ -401,11 +411,13 @@ export function hasNonHoldTweenForElement(
}
if (!targetEl) return false;
// fallow-ignore-next-line code-duplication
for (const tween of timeline.getChildren(true)) {
if (!tween.vars || !matchesElement(tween, targetEl)) continue;
const dur = typeof tween.duration === "function" ? tween.duration() : 0;
if (isZeroDurationSet(dur)) continue; // skip hold/set tweens (see isZeroDurationSet)
if (readTween(tween.vars)) return true;
const read = readTween(tween.vars);
if (read && (!channels || readCarriesChannel(read, channels))) return true;
}
return false;
}
@@ -29,6 +29,7 @@ export async function commitWholePropertyOffset(
callbacks: GsapDragCommitCallbacks,
label: string,
): Promise<void> {
// fallow-ignore-next-line code-duplication
let effectiveAnim = anim;
if (anim.keyframes) {
const newId = await materializeIfDynamic(anim, iframe, callbacks.commitMutation, selection);