diff --git a/packages/studio/src/components/editor/MotionPathOverlay.tsx b/packages/studio/src/components/editor/MotionPathOverlay.tsx index 664140429..a34bf7e53 100644 --- a/packages/studio/src/components/editor/MotionPathOverlay.tsx +++ b/packages/studio/src/components/editor/MotionPathOverlay.tsx @@ -132,7 +132,10 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ const createMode = geometryResolved && !geometry && Boolean(selection?.element) && !isPlaying; const createSelector = createMode ? selectorFor(selection) : null; const compW = compositionSize?.width ?? null; - const canCreate = createMode && hasMotionPathPlugin(iframeRef.current); + // No one-element selector means the path could only be authored onto the + // element's class siblings, so the toolbar toggle stays hidden instead of + // arming a press that the effect below would silently drop. + const canCreate = createMode && !!createSelector && hasMotionPathPlugin(iframeRef.current); // Publish whether the selected element can take a path so the preview toolbar // shows its "set destination" toggle. Drops to false when this overlay unmounts diff --git a/packages/studio/src/components/editor/motionPathSelection.test.ts b/packages/studio/src/components/editor/motionPathSelection.test.ts new file mode 100644 index 000000000..35927e389 --- /dev/null +++ b/packages/studio/src/components/editor/motionPathSelection.test.ts @@ -0,0 +1,65 @@ +// @vitest-environment jsdom +import { afterEach, describe, expect, it } from "vitest"; +import type { DomEditSelection } from "./domEditingTypes"; +import { buildStableSelector, getSelectorIndex } from "./domEditingDom"; +import { selectorFor } from "./motionPathSelection"; + +afterEach(() => { + document.body.innerHTML = ""; +}); + +function selectionFor(el: HTMLElement): DomEditSelection { + const selector = buildStableSelector(el); + return { + element: el, + id: el.id || undefined, + hfId: el.getAttribute("data-hf-id") || undefined, + selector, + selectorIndex: getSelectorIndex(document, el, selector, "index.html", null), + sourceFile: "index.html", + dataAttributes: { start: "0", duration: "2" }, + } as unknown as DomEditSelection; +} + +function mountGroupSiblings(): HTMLElement[] { + document.body.innerHTML = ` +
+
+
+
+
+ `; + return Array.from(document.querySelectorAll(".group")); +} + +describe("selectorFor", () => { + it("addresses one element for a class-only sibling", () => { + const groups = mountGroupSiblings(); + const selector = selectorFor(selectionFor(groups[2]!)); + + // The bare ".group" both measured home off the FIRST sibling and wrote the + // new motion path onto all three. + expect(selector).not.toBe(".group"); + expect(document.querySelectorAll(selector!)).toHaveLength(1); + expect(document.querySelector(selector!)).toBe(groups[2]); + }); + + it("keeps a unique id target", () => { + document.body.innerHTML = `
`; + const el = document.querySelector("#hero")!; + + expect(selectorFor(selectionFor(el))).toBe("#hero"); + }); + + it("returns null with no selection", () => { + expect(selectorFor(null)).toBeNull(); + }); + + it("returns null when no rung addresses one element", () => { + const groups = mountGroupSiblings(); + const selection = selectionFor(groups[1]!); + groups[1]!.remove(); + + expect(selectorFor(selection)).toBeNull(); + }); +}); diff --git a/packages/studio/src/components/editor/motionPathSelection.ts b/packages/studio/src/components/editor/motionPathSelection.ts index 10d193d50..c869ace83 100644 --- a/packages/studio/src/components/editor/motionPathSelection.ts +++ b/packages/studio/src/components/editor/motionPathSelection.ts @@ -5,11 +5,21 @@ */ import type { GsapAnimation } from "@hyperframes/parsers/gsap-parser"; import type { DomEditSelection } from "./domEditing"; +import { writeTargetSelector } from "../../hooks/gsapShared"; +/** + * The selector the overlay both MEASURES the element by and authors a new + * motion path against. + * + * Both halves need exactly one element. The selection's own selector is a bare + * class for an id-less element, so a `.group` sibling read its home position off + * the FIRST sibling (skewing the destination the click computes) and then wrote + * `add-motion-path` onto all five. `writeTargetSelector` is the same one-element + * narrowing every other new-tween writer goes through; null means no such form + * exists, and the overlay hides "set destination" rather than write a wrong one. + */ export function selectorFor(sel: DomEditSelection | null): string | null { - if (!sel) return null; - if (sel.id) return `#${CSS.escape(sel.id)}`; - return sel.selector ?? null; + return sel ? writeTargetSelector(sel) : null; } /** The animation whose path is editable on-canvas: literal, statically resolved, diff --git a/packages/studio/src/hooks/gsapDragCommit.ts b/packages/studio/src/hooks/gsapDragCommit.ts index aab4eebcb..62fd190e7 100644 --- a/packages/studio/src/hooks/gsapDragCommit.ts +++ b/packages/studio/src/hooks/gsapDragCommit.ts @@ -51,9 +51,14 @@ export interface GsapDragCommitCallbacks { * write over every sibling sharing the class. Retargets of an EXISTING tween * must NOT come through here (they keep `anim.targetSelector`, so a tween the * author aimed at a group stays aimed at it). + * + * Null means no one-element form exists, and every caller drops the commit + * rather than falling back to `selector` (see writeTargetSelector): the drag + * reverts on the next reload, which is recoverable, where a `.group` write is + * not. */ -function newTweenTarget(selection: DomEditSelection, selector: string): string { - return writeTargetSelector(selection) ?? selector; +function newTweenTarget(selection: DomEditSelection): string | null { + return writeTargetSelector(selection); } // Re-export for backward compatibility with existing imports. @@ -77,17 +82,18 @@ export function parkPlayheadOnKeyframe(anim: GsapAnimation, pct: number): void { async function replaceKeyframedPositionHold( selection: DomEditSelection, - selector: string, existingSet: GsapAnimation, properties: { x: number; y: number }, commitMutation: GsapDragCommitCallbacks["commitMutation"], ): Promise { + const target = newTweenTarget(selection); + if (!target) return; const persist = async (commit: GsapDragCommitCallbacks["commitMutation"]) => { await commit( selection, { type: "add", - targetSelector: newTweenTarget(selection, selector), + targetSelector: target, method: "set", position: 0, properties, @@ -186,7 +192,6 @@ export async function commitStaticGsapPosition( // least one hold on disk, then delete the corrupt tween in one transaction. await replaceKeyframedPositionHold( selection, - selector, existingSet, { x: newX, y: newY }, callbacks.commitMutation, @@ -213,7 +218,8 @@ export async function commitStaticGsapPosition( // an instant patch so the first nudge shows immediately (no soft-reload flash). // The patch reuses the WRITTEN target so the runtime moves exactly the element // the source write names. - const target = newTweenTarget(selection, selector); + const target = newTweenTarget(selection); + if (!target) return; await callbacks.commitMutation( selection, { @@ -270,7 +276,8 @@ export async function commitStaticGsapRotation( return; } // New static hold → off-timeline `gsap.set` (no 0% keyframe marker) + instant patch. - const target = newTweenTarget(selection, selector); + const target = newTweenTarget(selection); + if (!target) return; await callbacks.commitMutation( selection, { @@ -323,11 +330,13 @@ export async function commitStaticGsapSize( ); return; } + const target = newTweenTarget(selection); + if (!target) return; await callbacks.commitMutation( selection, { type: "add", - targetSelector: newTweenTarget(selection, selector), + targetSelector: target, method: "set", position: 0, properties: { width, height }, @@ -411,11 +420,13 @@ export async function commitKeyframedSizeFromResize( // transport applies both in one ordered batch; a plain commit fallback keeps the // same recoverable ordering. Only the transaction's result triggers the reload. const addLabel = `Resize (size keyframe ${pct.toFixed(0)}%)`; + const target = newTweenTarget(selection); + if (!target) return false; await callbacks.commitMutation( selection, { type: "add-with-keyframes", - targetSelector: newTweenTarget(selection, selector), + targetSelector: target, position: roundTo3(ts), duration: roundTo3(td), keyframes, diff --git a/packages/studio/src/hooks/gsapKeyframeCommit.ts b/packages/studio/src/hooks/gsapKeyframeCommit.ts index 7260d7c38..ed3ecc498 100644 --- a/packages/studio/src/hooks/gsapKeyframeCommit.ts +++ b/packages/studio/src/hooks/gsapKeyframeCommit.ts @@ -63,16 +63,18 @@ export async function commitKeyframeAtTimeImpl( }, ); } else { + // Null means the live DOM could not prove any one-element form. Falling + // back to the author's own selector would write the group-collapsing + // target this narrowing exists to prevent, so the keyframe is dropped + // instead (see writeTargetSelector). + const target = writeTargetSelector(selection); + if (!target) return; const defaultDuration = 0.5; await commitMutation( 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, + targetSelector: target, position: absoluteTime, duration: defaultDuration, keyframes: [ diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index fbf44f786..a0ce6317c 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -251,11 +251,12 @@ function structuralSelector(element: Element): string | null { * 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. + * function exists to replace. Every caller treats the null as "do not author + * this tween": falling back to the selection's own selector would write the + * group-collapsing target this function exists to prevent, and a gesture that + * does not persist reverts visibly on the next reload, where a tween silently + * aimed at five elements does not. 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); @@ -298,8 +299,14 @@ export function existingTweenTargetSelector( * String equality against `selectorFromSelection` alone is not enough once new * tweens are authored with a narrowed one-element selector: the next edit would * miss the write it just made and append a second, conflicting one. Falling back - * to the live DOM keeps the pair consistent, and still matches a deliberate group - * tween (`.group` matches each of its siblings) so merges into it keep working. + * to the live DOM keeps the pair consistent. + * + * That fallback is `matchesExactlyOne`, not a bare `element.matches`. A target + * the element merely shares with its siblings is a GROUP tween, and the callers + * here MUTATE what they find: an individual nudge on one of five `.group` + * siblings would rewrite the group's own tween and move all five. Only the + * selection that IS the group (string equality above, where the author selected + * `.group` itself) may edit it; every other element authors its own write. */ export function tweenTargetsElement( targetSelector: string, @@ -307,13 +314,9 @@ export function tweenTargetsElement( element: Element | null | undefined, ): boolean { if (targetSelector === selector) return true; - if (!element) return false; - try { - return element.matches(targetSelector); - } catch { - // Not a selector `matches()` understands, so it never addressed this element. - return false; - } + const doc = element?.ownerDocument; + if (!element || !doc) return false; + return matchesExactlyOne(doc, targetSelector, element); } // ── Percentage computation ──────────────────────────────────────────────────── diff --git a/packages/studio/src/hooks/gsapShared.writeTarget.test.ts b/packages/studio/src/hooks/gsapShared.writeTarget.test.ts index c09c8b878..1be27acf5 100644 --- a/packages/studio/src/hooks/gsapShared.writeTarget.test.ts +++ b/packages/studio/src/hooks/gsapShared.writeTarget.test.ts @@ -4,7 +4,7 @@ import { parseGsapScript } from "@hyperframes/core/gsap-parser"; import { addAnimationWithKeyframesToScript } from "@hyperframes/parsers/gsap-writer-acorn"; import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { buildStableSelector, getSelectorIndex } from "../components/editor/domEditingDom"; -import { resolveSelectorElementIds, writeTargetSelector } from "./gsapShared"; +import { resolveSelectorElementIds, tweenTargetsElement, writeTargetSelector } from "./gsapShared"; import { commitKeyframeAtTimeImpl } from "./gsapKeyframeCommit"; import { promoteSetToKeyframes } from "./useEnableKeyframes"; @@ -244,3 +244,46 @@ describe("commitKeyframeAtTimeImpl — new-tween target", () => { expect(document.querySelector(mutation.targetSelector)).toBe(groups[3]); }); }); + +describe("tweenTargetsElement", () => { + it("matches a tween narrowed to this element by a selector the selection does not use", () => { + const groups = mountGroupSiblings(); + groups[2]!.id = "narrowed"; + + // The write half authored "#narrowed"; the read half still has ".group". + expect(tweenTargetsElement("#narrowed", ".group", groups[2])).toBe(true); + }); + + it("does not hand an individual element the group tween it merely inherits", () => { + const groups = mountGroupSiblings(); + + // Selecting one sibling by its own address must not let an edit mutate the + // ".group" tween: that write moves all five, not the one being nudged. + expect(tweenTargetsElement(".group", "#scene > div:nth-child(3)", groups[2])).toBe(false); + }); + + it("still edits a group tween when the group itself is the selection", () => { + const groups = mountGroupSiblings(); + + expect(tweenTargetsElement(".group", ".group", groups[0])).toBe(true); + }); + + it("does not match a target that is not a selector matches() understands", () => { + const groups = mountGroupSiblings(); + + expect(tweenTargetsElement("div[unclosed", ".group", groups[0])).toBe(false); + }); +}); + +describe("commitKeyframeAtTimeImpl — no one-element target", () => { + it("drops the keyframe rather than authoring the bare class", async () => { + const groups = mountGroupSiblings(); + const selection = selectionFor(groups[3]!); + groups[3]!.remove(); + const commitMutation = vi.fn(async () => undefined); + + await commitKeyframeAtTimeImpl(selection, 1, [], { x: 12 }, commitMutation); + + expect(commitMutation).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts index 346f60a78..640f59328 100644 --- a/packages/studio/src/hooks/useAnimatedPropertyCommit.ts +++ b/packages/studio/src/hooks/useAnimatedPropertyCommit.ts @@ -214,7 +214,7 @@ async function commitStaticSet( } // Fresh adds don't reshape existing sets, so their ids can't go stale. for (const batch of newSetBatches) { - await addGlobalStaticSet(selection, batch, selector, commit); + await addGlobalStaticSet(selection, batch, commit); } } @@ -244,16 +244,17 @@ function findGroupOwningStaticWrite( async function addGlobalStaticSet( selection: DomEditSelection, batch: [string, number | string][], - selector: string, commit: Commit, ): Promise { const numericProps: SetPatchProps = {}; for (const [k, v] of batch) { if (typeof v === "number") numericProps[k as keyof SetPatchProps] = v; } - // A brand-new write, so it must address ONE element: `selector` is the bare - // class an id-less selection yields, which would hold every sibling. - const target = writeTargetSelector(selection) ?? selector; + // A brand-new write, so it must address ONE element: the selection's own + // selector is the bare class an id-less element yields, which would hold every + // sibling. No one-element form means no write at all (see writeTargetSelector). + const target = writeTargetSelector(selection); + if (!target) return; await commit( selection, { @@ -493,7 +494,11 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { // contaminating a foreign-group tween. Mirror an existing keyframed tween's // time range so the new group animates over the same span. The 0% baseline is // an `_auto` endpoint so it tracks the nearest keyframe as you add more. - if (selector) { + // A fresh tween, so its target must address ONE element; with no + // one-element form the edit is dropped rather than written onto every + // class sibling (see writeTargetSelector). + const newTweenTarget = writeTargetSelector(selection); + if (selector && newTweenTarget) { const template = selectedGsapAnimations.find((a) => !!a.keyframes); const tStart = template ? (resolveTweenStart(template) ?? 0) : 0; const tDur = template ? resolveTweenDuration(template) || 1 : 1; @@ -514,7 +519,7 @@ export function useAnimatedPropertyCommit(deps: CommitAnimatedPropertyDeps) { selection, { type: "add-with-keyframes", - targetSelector: writeTargetSelector(selection) ?? selector, + targetSelector: newTweenTarget, position: roundTo3(tStart), duration: roundTo3(tDur), keyframes, diff --git a/packages/studio/src/hooks/useGestureCommit.ts b/packages/studio/src/hooks/useGestureCommit.ts index 6115f2ff6..bc87caac3 100644 --- a/packages/studio/src/hooks/useGestureCommit.ts +++ b/packages/studio/src/hooks/useGestureCommit.ts @@ -178,7 +178,14 @@ export function useGestureCommit({ showToast("Cannot save — element has no selector", "error"); return; } - const writeSelector = writeTargetSelector(sel) ?? selector; + // A recorded gesture becomes a NEW tween, so its target must address one + // element; the selection's own selector would record the motion onto + // every sibling sharing its class (see writeTargetSelector). + const writeSelector = writeTargetSelector(sel); + if (!writeSelector) { + showToast("Cannot save: element has no unique selector", "error"); + return; + } if (liveSession.commitMutation) { const recStart = recordingStartTimeRef.current; const rawKeyframes = sortedPcts.map((pct) => ({