diff --git a/packages/studio/src/hooks/gsapKeyframeCommit.ts b/packages/studio/src/hooks/gsapKeyframeCommit.ts index eb674b7d6..7260d7c38 100644 --- a/packages/studio/src/hooks/gsapKeyframeCommit.ts +++ b/packages/studio/src/hooks/gsapKeyframeCommit.ts @@ -68,6 +68,10 @@ export async function commitKeyframeAtTimeImpl( selection, { type: "add-with-keyframes" as const, + // Null here means the live DOM could not prove any one-element form. + // This branch has no graceful no-op to fall to, so it takes the + // author's own selector, group collapse and all, over dropping the + // keyframe the user just asked for. targetSelector: writeTargetSelector(selection) ?? selector, position: absoluteTime, duration: defaultDuration, diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index bba2713b4..a7ce0ee4b 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -221,7 +221,7 @@ function structuralSelector(element: Element): string | null { } const parent = node.parentElement; if (!parent) break; - const index = Array.prototype.indexOf.call(parent.children, node) + 1; + const index = [...parent.children].indexOf(node) + 1; if (index < 1) return null; parts.unshift(`${node.tagName.toLowerCase()}:nth-child(${index})`); } @@ -242,9 +242,15 @@ function structuralSelector(element: Element): string | null { * {@link resolveSelectorElementIds} reads back as all five, collapsing their * timeline rows into one. Every rung below resolves to exactly one element. * - * ponytail: the last rung returns the bare selector unchanged rather than null. - * Refusing to write would turn a mis-targeted add into a silently dead button; - * it is only reachable with no live DOM to disambiguate against. + * Null means "no string here addresses one element". With a live DOM to check + * against, a failed structural walk (detached between select and commit, a + * shadow-root boundary, a chain that no longer re-resolves) IS that evidence, + * so returning the bare selector anyway would hand back the exact input this + * function exists to replace. Callers decide what to do with the null: the + * add-keyframe path has a graceful no-animation fallback, and the paths that + * cannot afford to drop a user edit opt back in with `?? selectorFromSelection` + * where the trade is visible. The bare selector comes back only with no DOM to + * disambiguate against, where refusing would be guessing rather than knowing. */ export function writeTargetSelector(selection: DomEditSelection): string | null { if (selection.id) return idSelector(selection.id); @@ -255,12 +261,31 @@ export function writeTargetSelector(selection: DomEditSelection): string | null if (selection.selector && matchesExactlyOne(doc, selection.selector, element)) { return selection.selector; } - const structural = structuralSelector(element); - if (structural) return structural; + return structuralSelector(element); } return selection.selector ?? null; } +/** + * The selector a `replace-with-keyframes` mutation must re-author an EXISTING + * tween against. The server deletes the tween and adds it back, so this string + * REWRITES its target: deriving it from the selection instead discards whatever + * the author aimed at, and silently widens a tween {@link writeTargetSelector} + * had already narrowed to one element back onto every class sibling. + * + * The selection is the fallback only for a target the parser could not resolve + * statically, where there is no authored string to preserve. + */ +export function existingTweenTargetSelector( + animation: Pick, + selection: DomEditSelection, +): string | null { + if (animation.targetSelector && !animation.hasUnresolvedSelector) { + return animation.targetSelector; + } + return selectorFromSelection(selection); +} + // ── Percentage computation ──────────────────────────────────────────────────── /** diff --git a/packages/studio/src/hooks/gsapShared.writeTarget.test.ts b/packages/studio/src/hooks/gsapShared.writeTarget.test.ts index 627007cb0..c09c8b878 100644 --- a/packages/studio/src/hooks/gsapShared.writeTarget.test.ts +++ b/packages/studio/src/hooks/gsapShared.writeTarget.test.ts @@ -6,6 +6,7 @@ import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { buildStableSelector, getSelectorIndex } from "../components/editor/domEditingDom"; import { resolveSelectorElementIds, writeTargetSelector } from "./gsapShared"; import { commitKeyframeAtTimeImpl } from "./gsapKeyframeCommit"; +import { promoteSetToKeyframes } from "./useEnableKeyframes"; afterEach(() => { document.body.innerHTML = ""; @@ -102,6 +103,62 @@ describe("writeTargetSelector", () => { expect(writeTargetSelector(selectionFor(el))).toBe(".header"); }); + + it("hands identity back to a data-hf-id ancestor part way up a deep chain", () => { + // Mixed identity: the walk must step past the id-less
, stop at the + // data-hf-id row, and NOT keep climbing to #outer. + document.body.innerHTML = ` +
+
+
+
+
+
+
+
+
+
+ `; + const el = document.querySelectorAll(".cell")[2]!; + + const written = writeTargetSelector(selectionFor(el)); + + expect(written).toBe('[data-hf-id="mid"] > div:nth-child(3)'); + expect(document.querySelectorAll(written!)).toHaveLength(1); + expect(document.querySelector(written!)).toBe(el); + }); + + it("falls through to the structural walk when the selection's selector is not valid CSS", () => { + document.body.innerHTML = `
`; + const el = document.querySelectorAll("#scene > div")[1]!; + // querySelectorAll throws on this, so the "does it match exactly one?" rung + // must read as a miss rather than crashing the add. + const selection = { ...selectionFor(el), selector: "div[unclosed" } as DomEditSelection; + + expect(writeTargetSelector(selection)).toBe("#scene > div:nth-child(2)"); + }); + + it("returns null when a live DOM is present and no rung addresses one element", () => { + document.body.innerHTML = `
`; + const el = document.querySelectorAll(".group")[1]!; + const selection = selectionFor(el); + expect(selection.selector).toBe(".group"); + // Detached between selecting and committing: ownerDocument still resolves, + // but there is no parent to count a :nth-child position in. Returning + // ".group" here would re-author the very selector this function replaces. + el.remove(); + + expect(writeTargetSelector(selection)).toBeNull(); + }); + + it("still returns the selection's own selector when there is no DOM to check against", () => { + const selection = { + selector: ".group", + sourceFile: "index.html", + } as unknown as DomEditSelection; + + expect(writeTargetSelector(selection)).toBe(".group"); + }); }); describe("writeTargetSelector — write/read round trip", () => { @@ -131,6 +188,48 @@ describe("writeTargetSelector — write/read round trip", () => { }); }); +describe("existingTweenTargetSelector", () => { + it("keeps the narrowed target when a replace re-authors the tween", async () => { + const groups = mountGroupSiblings(); + const selection = selectionFor(groups[2]); + const commitMutation = vi.fn(async () => undefined); + // A tween a previous add already narrowed to one sibling. Re-deriving the + // target from the selection would widen it back to all five. + const setAnim = { + id: "a1", + targetSelector: "#scene > div:nth-child(3)", + method: "set", + position: 0, + properties: { x: 10 }, + }; + + await promoteSetToKeyframes({ commitMutation } as never, selection, setAnim as never, 0, null); + + const mutation = commitMutation.mock.calls[0]?.[0] as { targetSelector: string }; + expect(mutation.targetSelector).toBe("#scene > div:nth-child(3)"); + expect(document.querySelectorAll(mutation.targetSelector)).toHaveLength(1); + }); + + it("falls back to the selection when the tween's own target did not resolve", async () => { + const groups = mountGroupSiblings(); + const selection = selectionFor(groups[2]); + const commitMutation = vi.fn(async () => undefined); + const setAnim = { + id: "a1", + targetSelector: "targets[i]", + hasUnresolvedSelector: true, + method: "set", + position: 0, + properties: { x: 10 }, + }; + + await promoteSetToKeyframes({ commitMutation } as never, selection, setAnim as never, 0, null); + + const mutation = commitMutation.mock.calls[0]?.[0] as { targetSelector: string }; + expect(mutation.targetSelector).toBe(".group"); + }); +}); + describe("commitKeyframeAtTimeImpl — new-tween target", () => { it("authors a one-element selector when no tween exists at the playhead", async () => { const groups = mountGroupSiblings(); diff --git a/packages/studio/src/hooks/useEnableKeyframes.ts b/packages/studio/src/hooks/useEnableKeyframes.ts index c74869efa..c886ca58b 100644 --- a/packages/studio/src/hooks/useEnableKeyframes.ts +++ b/packages/studio/src/hooks/useEnableKeyframes.ts @@ -13,7 +13,7 @@ import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { usePlayerStore } from "../player/store/playerStore"; import { fetchParsedAnimations, getAnimationsForElement } from "./useGsapTweenCache"; import { - selectorFromSelection, + existingTweenTargetSelector, computeElementPercentage, KEYFRAME_PCT_MATCH, isInstantHold, @@ -251,7 +251,10 @@ async function extendKeyframedTweenToPlayhead( iframe: HTMLIFrameElement | null, commitOverrides?: Partial, ): Promise { - const selector = selectorFromSelection(sel); + // Re-authoring an EXISTING tween: keep the target it already writes rather + // than re-deriving one from the selection, which would widen a tween already + // narrowed to one element back onto its class siblings (existingTweenTargetSelector). + const selector = existingTweenTargetSelector(anim, sel); const position = readElementPosition(iframe, sel, anim); if (!selector || Object.keys(position).length === 0 || !session.commitMutation) return; const extended = buildExtendedKeyframes(anim, currentTime, position, duration); @@ -336,7 +339,10 @@ export async function promoteSetToKeyframes( t: number, iframe: HTMLIFrameElement | null, ): Promise { - const selector = selectorFromSelection(sel); + // Re-authoring an EXISTING tween: keep the target it already writes rather + // than re-deriving one from the selection, which would widen a tween already + // narrowed to one element back onto its class siblings (existingTweenTargetSelector). + const selector = existingTweenTargetSelector(setAnim, sel); const setStart = resolveTweenStart(setAnim) ?? 0; if (!selector || !session.commitMutation) return; // Playhead at or before the set → there's no forward range to promote into. @@ -390,7 +396,10 @@ export async function applyArcKeyframeAtPlayhead( iframe: HTMLIFrameElement | null, ): Promise { if (!session.commitMutation) return; - const targetSelector = selectorFromSelection(sel); + // Re-authoring an EXISTING tween: keep the target it already writes rather + // than re-deriving one from the selection, which would widen a tween already + // narrowed to one element back onto its class siblings (existingTweenTargetSelector). + const targetSelector = existingTweenTargetSelector(arcAnim, sel); if (!targetSelector) return; const start = resolveTweenStart(arcAnim) ?? 0; const duration = resolveEditableTweenDuration(arcAnim, sel);