From f04cdb79c5f1475fa7d6048036a4e0af354a60a2 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 28 Jul 2026 20:32:08 +0200 Subject: [PATCH] fix(studio): never re-author a target the DOM proved is not unique writeTargetSelector returned the selection's bare selector whenever the structural walk failed, including when a live DOM was there to check against. An element detached between selecting and committing takes that path, so the add re-authored the exact `.group` string the function exists to replace. Return null instead: a failed walk against a live DOM is evidence, not absence of it. Callers that cannot drop a user edit opt back in with `?? selectorFromSelection` where the trade is visible. The replace-with-keyframes paths had the mirror defect. The server deletes and re-adds the tween, so their target string is a full rewrite, and they derived it from the selection: promoting a set on a tween already narrowed to `#scene > div:nth-child(3)` widened it back onto every class sibling, undoing the narrowing an earlier add had made. They now keep the tween's own authored target, matching what eight sibling commit modules already do. --- .../studio/src/hooks/gsapKeyframeCommit.ts | 4 + packages/studio/src/hooks/gsapShared.ts | 37 +++++-- .../src/hooks/gsapShared.writeTarget.test.ts | 99 +++++++++++++++++++ .../studio/src/hooks/useEnableKeyframes.ts | 17 +++- 4 files changed, 147 insertions(+), 10 deletions(-) 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);