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.
This commit is contained in:
Miguel Angel Simon Sierra
2026-07-28 20:37:52 +02:00
parent fd5555be75
commit f04cdb79c5
4 changed files with 147 additions and 10 deletions
@@ -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,
+31 -6
View File
@@ -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<GsapAnimation, "targetSelector" | "hasUnresolvedSelector">,
selection: DomEditSelection,
): string | null {
if (animation.targetSelector && !animation.hasUnresolvedSelector) {
return animation.targetSelector;
}
return selectorFromSelection(selection);
}
// ── Percentage computation ────────────────────────────────────────────────────
/**
@@ -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 <section>, stop at the
// data-hf-id row, and NOT keep climbing to #outer.
document.body.innerHTML = `
<div id="outer">
<header></header>
<section>
<div data-hf-id="mid">
<div class="cell"></div>
<div class="cell"></div>
<div class="cell"></div>
</div>
</section>
</div>
`;
const el = document.querySelectorAll<HTMLElement>(".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 = `<div id="scene"><div></div><div></div></div>`;
const el = document.querySelectorAll<HTMLElement>("#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 = `<div id="scene"><div class="group"></div><div class="group"></div></div>`;
const el = document.querySelectorAll<HTMLElement>(".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();
@@ -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<CommitMutationOptions>,
): Promise<void> {
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<void> {
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<void> {
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);