diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts index 05e5c93da..2434f6c87 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts @@ -276,6 +276,69 @@ describe("updateKeyframeCacheFromParsed", () => { expect(usePlayerStore.getState().gsapAnimations.get("scene.html#box")).toEqual([animation]); }); + // `#stat3 .block` animates the BLOCK inside #stat3, not #stat3. An unanchored + // `^#([\w-]+)/` prefix match filed it under "stat3", which both stole the + // child's diamonds and collided with #stat3's own tween at the shared + // percentage (dropping #stat3's ease as ambiguous). + it("attributes a descendant selector to the child, not to its ancestor", () => { + const parent: GsapAnimation = { + id: "stat3-fromTo", + targetSelector: "#stat3", + method: "fromTo", + position: 8.88, + resolvedStart: 8.88, + duration: 0.25, + fromProperties: { y: 20 }, + properties: { y: 0 }, + ease: "power2.out", + propertyGroup: "position", + }; + const child: GsapAnimation = { + id: "block-from", + targetSelector: "#stat3 .block", + method: "from", + position: 9.13, + resolvedStart: 9.13, + duration: 0.3, + properties: { opacity: 0 }, + ease: "power2.in", + propertyGroup: "visual", + }; + usePlayerStore.setState({ + elements: [ + { id: "stat3-clip", domId: "stat3", tag: "div", start: 8.88, duration: 1, track: 0 }, + ], + }); + const doc = { + querySelectorAll: (selector: string) => + (selector === "#stat3 .block" + ? [{ id: "stat3-block" }] + : []) as unknown as NodeListOf, + } as unknown as Document; + + updateKeyframeCacheFromParsed([parent, child], "scene.html", "stat3", {}, doc); + + expect(usePlayerStore.getState().gsapAnimations.get("scene.html#stat3")).toEqual([parent]); + expect(usePlayerStore.getState().gsapAnimations.get("scene.html#stat3-block")).toEqual([child]); + // With the collision gone, #stat3's own curve survives instead of being + // deleted as an ambiguous same-percentage merge. + const parentKeyframes = cache().get("scene.html#stat3")?.keyframes ?? []; + expect(parentKeyframes.at(-1)?.ease).toBe("power2.out"); + expect(parentKeyframes.some((keyframe) => "easeAmbiguous" in keyframe)).toBe(false); + }); + + it("attributes a descendant selector to nothing when there is no document", () => { + const child: GsapAnimation = { + ...animWithKeyframes("block-from"), + targetSelector: "#stat3 .block", + }; + + updateKeyframeCacheFromParsed([child], "scene.html", "stat3", {}); + + expect(cache().has("scene.html#stat3")).toBe(false); + expect(usePlayerStore.getState().gsapAnimations.has("scene.html#stat3")).toBe(false); + }); + it("does not cache a flat tween without animatable numeric properties", () => { const animation: GsapAnimation = { id: "flat-box", diff --git a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts index 80fb02e01..a2ed338cb 100644 --- a/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts +++ b/packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts @@ -4,7 +4,7 @@ */ import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore"; -import { idFromSelector, resolveClipTimingBasis, toClipKeyframes } from "./gsapShared"; +import { resolveClipTimingBasis, resolveSelectorElementIds, toClipKeyframes } from "./gsapShared"; import { deduplicateKeyframes, synthesizeFlatTweenKeyframes } from "./gsapTweenSynth"; export function updateKeyframeCacheFromParsed( @@ -12,58 +12,65 @@ export function updateKeyframeCacheFromParsed( targetPath: string, selectionId: string | undefined, mutation: Record, + doc?: Document | null, ): void { const { setKeyframeCache, elements, domClipChildren } = usePlayerStore.getState(); const idsWithKeyframes = new Set(); const merged = new Map(); const sourceAnimations = new Map(); for (const anim of animations) { - const id = idFromSelector(anim.targetSelector); const kfSource = anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? []; - if (!id || kfSource.length === 0) continue; - idsWithKeyframes.add(id); - // Every tween that fed keyframeCache also lands in gsapAnimations, group or - // not: a mixed-group tween (`{ x, opacity }` classifies to undefined) used to - // cache diamonds with no source animation behind them, so the collapsed row - // drew keyframes the expanded lanes couldn't render. Lane consumers do the - // group filtering themselves (animationContributesLane). - sourceAnimations.set(id, [...(sourceAnimations.get(id) ?? []), anim]); + if (kfSource.length === 0) continue; + // Attribute the tween to every element it actually animates. A leading-id + // match filed `#stat3 .block` under `#stat3`: the child's diamonds landed on + // its ancestor AND collided with the ancestor's own tween at the shared + // percentage, which the same-% merge then resolved by dropping the ease. + for (const id of resolveSelectorElementIds(anim.targetSelector, doc)) { + idsWithKeyframes.add(id); + // Every tween that fed keyframeCache also lands in gsapAnimations, group or + // not: a mixed-group tween (`{ x, opacity }` classifies to undefined) used to + // cache diamonds with no source animation behind them, so the collapsed row + // drew keyframes the expanded lanes couldn't render. Lane consumers do the + // group filtering themselves (animationContributesLane). + sourceAnimations.set(id, [...(sourceAnimations.get(id) ?? []), anim]); - // Convert tween-relative percentages to clip-relative so diamonds - // render at the correct position within the timeline clip. The basis comes - // from the shared resolver, so this writer agrees with the AST load on both - // the sub-comp host fallback and the tween's own time frame. - const { elStart, elDuration } = resolveClipTimingBasis( - id, - targetPath, - elements, - domClipChildren, - ); - const clipKeyframes = toClipKeyframes(kfSource, anim, elStart, elDuration); + // Convert tween-relative percentages to clip-relative so diamonds + // render at the correct position within the timeline clip. The basis comes + // from the shared resolver, so this writer agrees with the AST load on both + // the sub-comp host fallback and the tween's own time frame. + const { elStart, elDuration } = resolveClipTimingBasis( + id, + targetPath, + elements, + domClipChildren, + ); + const clipKeyframes = toClipKeyframes(kfSource, anim, elStart, elDuration); - const existing = merged.get(id); - if (existing) { - // deduplicateKeyframes owns the same-% merge (including the easeAmbiguous - // flag downstream lanes read); a second copy of that rule here is how the - // two writers drift. - existing.keyframes = deduplicateKeyframes([...existing.keyframes, ...clipKeyframes]); - } else { - merged.set(id, { - ...anim.keyframes, - format: anim.keyframes?.format ?? "percentage", - keyframes: clipKeyframes, - }); + const existing = merged.get(id); + if (existing) { + // deduplicateKeyframes owns the same-% merge (including the easeAmbiguous + // flag downstream lanes read); a second copy of that rule here is how the + // two writers drift. + existing.keyframes = deduplicateKeyframes([...existing.keyframes, ...clipKeyframes]); + } else { + merged.set(id, { + ...anim.keyframes, + format: anim.keyframes?.format ?? "percentage", + keyframes: clipKeyframes, + }); + } } } for (const [id, entry] of merged) { for (const key of elementCacheKeys(targetPath, id)) setKeyframeCache(key, entry); writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id)); } - const targetId = - idFromSelector((mutation as { targetSelector?: string }).targetSelector) ?? selectionId; - if (targetId && !idsWithKeyframes.has(targetId)) { - clearKeyframeCacheForElement(targetPath, targetId); + const mutationSelector = (mutation as { targetSelector?: string }).targetSelector; + const mutated = mutationSelector ? resolveSelectorElementIds(mutationSelector, doc) : []; + const targetIds = mutated.length > 0 ? mutated : selectionId ? [selectionId] : []; + for (const targetId of targetIds) { + if (!idsWithKeyframes.has(targetId)) clearKeyframeCacheForElement(targetPath, targetId); } } diff --git a/packages/studio/src/hooks/gsapShared.ts b/packages/studio/src/hooks/gsapShared.ts index a92b2ae71..caa1c2625 100644 --- a/packages/studio/src/hooks/gsapShared.ts +++ b/packages/studio/src/hooks/gsapShared.ts @@ -110,6 +110,71 @@ export function idFromSelector(selector: string | undefined | null): string | nu return (attribute[1] ?? "").replace(/\\(["\\])/g, "$1"); } +/** Either shape {@link idSelector} emits, anchored to the WHOLE selector. */ +const WHOLE_SELECTOR_ID = /^(#[\w-]+|\[id="(?:\\.|[^"\\])*"\])$/; + +/** + * The id a selector addresses **as a whole**, or null. `"#stat3 .block"` animates + * the `.block` INSIDE `#stat3`, not `#stat3`, so the unanchored leading-id match + * of {@link idFromSelector} is wrong for attribution: it files the child's + * keyframes under its ancestor. `idFromSelector` stays unanchored on purpose + * (two non-attribution callers want the leading id); attribution goes through + * here, or through the DOM (see resolveSelectorElementIds). + */ +function wholeSelectorElementId(selector: string): string | null { + const trimmed = selector.trim(); + return WHOLE_SELECTOR_ID.test(trimmed) ? idFromSelector(trimmed) : null; +} + +/** + * Resolve a tween's target selector to the ids of the element(s) it animates. + * A whole-selector `#id` resolves directly; anything else (a class like `.dot`, + * a group `.a, .b`, or a descendant selector) is matched against the live + * preview DOM so class/selector tweens (e.g. `gsap.from(".dot", {stagger})`) + * attribute to every element they animate — not just one parsed from the string. + * With no DOM, only whole-selector ids resolve: a descendant selector has no + * answer that isn't a guess at its ancestor. + */ +export function resolveSelectorElementIds( + selector: string, + doc: Document | null | undefined, +): string[] { + const bareId = wholeSelectorElementId(selector); + if (bareId) return [bareId]; + const ids = new Set(); + for (const part of selector.split(",")) { + const sel = part.trim(); + if (!sel) continue; + if (!doc) { + const whole = wholeSelectorElementId(sel); + if (whole) ids.add(whole); + continue; + } + try { + for (const el of Array.from(doc.querySelectorAll(sel))) { + if (el.id) ids.add(el.id); + } + } catch { + // An unsupported/invalid selector never reached the DOM, so the leading id + // is the best available answer (`[id="01-hook"]:has(>*)` still names it). + const lead = idFromSelector(sel); + if (lead) ids.add(lead); + } + } + return Array.from(ids); +} + +/** + * The clip start in the frame the element's OWN tweens are measured in. An + * expanded sub-composition child sits on the master timeline at a host-absolute + * `start`, but its tweens are parsed from its own source file and are local to + * it, so the two must be brought into one frame before any clip-% math — or + * every keyframe rebases to a percentage far outside the clip. + */ +export function clipTimingStart(element: { start: number; expandedParentStart?: number }): number { + return element.start - (element.expandedParentStart ?? 0); +} + export function selectorFromSelection(selection: DomEditSelection): string | null { if (selection.id) return idSelector(selection.id); if (selection.selector) return selection.selector; diff --git a/packages/studio/src/hooks/keyframeCacheAstLoad.ts b/packages/studio/src/hooks/keyframeCacheAstLoad.ts index f940d6160..9ae59b3d5 100644 --- a/packages/studio/src/hooks/keyframeCacheAstLoad.ts +++ b/packages/studio/src/hooks/keyframeCacheAstLoad.ts @@ -11,50 +11,15 @@ import { elementCacheKeys, writeGsapAnimationsForElement, } from "./gsapKeyframeCacheHelpers"; -import { idFromSelector, resolveClipTimingBasis, toClipKeyframes } from "./gsapShared"; +import { resolveClipTimingBasis, resolveSelectorElementIds, toClipKeyframes } from "./gsapShared"; import { deduplicateKeyframes, isStaticPositionHold, synthesizeFlatTweenKeyframes, } from "./gsapTweenSynth"; -/** - * Resolve a tween's target selector to the ids of the element(s) it animates. - * A bare `#id` resolves directly; anything else (a class like `.dot`, a group - * `.a, .b`, or a descendant selector) is matched against the live preview DOM so - * class/selector tweens (e.g. `gsap.from(".dot", {stagger})`) attribute to every - * element they animate — not just one parsed from the string. Falls back to a - * leading `#id` when there's no DOM (so the cache still populates pre-iframe). - */ -// fallow-ignore-next-line complexity -export function resolveSelectorElementIds( - selector: string, - doc: Document | null | undefined, -): string[] { - // A whole-selector id match (either shape) addresses exactly one element. - const bareId = /^(#[\w-]+|\[id="(?:\\.|[^"\\])*"\])$/.test(selector) - ? idFromSelector(selector) - : null; - if (bareId) return [bareId]; - if (!doc) { - const lead = idFromSelector(selector); - return lead ? [lead] : []; - } - const ids = new Set(); - for (const part of selector.split(",")) { - const sel = part.trim(); - if (!sel) continue; - try { - for (const el of Array.from(doc.querySelectorAll(sel))) { - if (el.id) ids.add(el.id); - } - } catch { - const lead = idFromSelector(sel); - if (lead) ids.add(lead); - } - } - return Array.from(ids); -} +export { resolveSelectorElementIds }; + /** * The slice of the parse response callers actually read. The endpoint returns * the full `ParsedGsap` (preamble/postamble and all), but nothing downstream of diff --git a/packages/studio/src/hooks/useGsapScriptCommits.ts b/packages/studio/src/hooks/useGsapScriptCommits.ts index 566d2bc53..b4b7d6f63 100644 --- a/packages/studio/src/hooks/useGsapScriptCommits.ts +++ b/packages/studio/src/hooks/useGsapScriptCommits.ts @@ -186,6 +186,9 @@ function syncCommittedGsapMutation({ targetPath, selection.id ?? undefined, mutation, + // The live preview document is what resolves a class / descendant tween to + // the elements it really animates; without it only whole-id selectors do. + iframe?.contentDocument, ); } refreshMutationPreview(iframe, result, options, reloadPreview, onCacheInvalidate); diff --git a/packages/studio/src/hooks/useGsapTweenCache.test.ts b/packages/studio/src/hooks/useGsapTweenCache.test.ts index 0492a1f50..43cb03a41 100644 --- a/packages/studio/src/hooks/useGsapTweenCache.test.ts +++ b/packages/studio/src/hooks/useGsapTweenCache.test.ts @@ -86,16 +86,24 @@ describe("resolveSelectorElementIds", () => { expect(resolveSelectorElementIds(".a, .b", doc).sort()).toEqual(["x", "y"]); }); - it("falls back to a leading #id when there is no DOM", () => { - expect(resolveSelectorElementIds("#card .label", null)).toEqual(["card"]); + // A DOM-less LEADING-id fallback attributed `#card .label` to `#card`, the + // ancestor it merely scopes to. Without a DOM there is nothing to resolve the + // descendant against, so the honest answer is no element at all. + it("resolves nothing for a compound selector when there is no DOM", () => { + expect(resolveSelectorElementIds("#card .label", null)).toEqual([]); expect(resolveSelectorElementIds(".dot", null)).toEqual([]); }); + it("still resolves every whole-id part of a group selector without a DOM", () => { + expect(resolveSelectorElementIds("#a, #b", null)).toEqual(["a", "b"]); + }); + // The `[id="…"]` form is what writers emit for a CSS-unsafe id (digit-leading, // dotted). The old local `#id`-only regex read no id at all for those, so they // silently dropped out of both DOM-less paths. it("falls back to a bracketed id when there is no DOM", () => { - expect(resolveSelectorElementIds('[id="01-hook"] .label', null)).toEqual(["01-hook"]); + expect(resolveSelectorElementIds('[id="01-hook"]', null)).toEqual(["01-hook"]); + expect(resolveSelectorElementIds('[id="01-hook"] .label', null)).toEqual([]); }); it("falls back to a bracketed id when querySelectorAll rejects the selector", () => { diff --git a/packages/studio/src/player/components/TimelineLanes.tsx b/packages/studio/src/player/components/TimelineLanes.tsx index d8b622f2c..e6e0b8a60 100644 --- a/packages/studio/src/player/components/TimelineLanes.tsx +++ b/packages/studio/src/player/components/TimelineLanes.tsx @@ -6,6 +6,7 @@ import { TimelineClipDiamonds } from "./TimelineClipDiamonds"; import { TimelinePropertyLanes } from "./TimelinePropertyLanes"; import { TimelineTrackHeader } from "./TimelineTrackHeader"; import { resolveTrackKeyframeClip } from "./useTimelineTrackLayout"; +import { clipTimingStart } from "../../hooks/gsapShared"; import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity"; import type { MusicBeatAnalysis } from "@hyperframes/core/beats"; import { getTimelineEditCapabilities, resolveBlockedTimelineEditIntent } from "./timelineEditing"; @@ -527,7 +528,10 @@ export function TimelineLanes({ , +): GsapAnimation { + return { + id, + targetSelector: "#clip-1", + method: "to", + position: 0, + duration: 1, + properties, + }; +} + describe("TimelinePropertyLanes", () => { + // `{ x, opacity }` is the canonical HyperFrames entrance tween. The parser + // classifies it to `undefined` (two groups), which used to erase it from the + // lanes entirely — no caret, no reserved row, nothing to edit. + it("lanes a mixed-group tween once per group it animates", () => { + const lanes = getTimelinePropertyLanes( + [ungroupedAnimation("entrance", { x: 0, opacity: 1 })], + 0, + 1, + ); + + expect(lanes.map((lane) => lane.group).sort()).toEqual(["position", "visual"]); + for (const lane of lanes) { + expect(lane.keyframes.map((keyframe) => keyframe.percentage)).toEqual([0, 100]); + expect(lane.keyframes.every((keyframe) => keyframe.animationId === "entrance")).toBe(true); + } + }); + + it("lanes a tween whose properties are all unknown as one 'other' lane", () => { + const lanes = getTimelinePropertyLanes( + [ungroupedAnimation("rounded", { borderRadius: 12, fontSize: 24 })], + 0, + 1, + ); + + expect(lanes).toHaveLength(1); + expect(lanes[0]?.group).toBe("other"); + expect(groupLabel("other", lanes[0]!.keyframes[0]!.properties)).toBe("BorderRadius"); + }); + + // An expanded sub-composition child sits on the MASTER timeline at a + // host-absolute start while its tweens are parsed from its own file and are + // local to it. clipTimingStart is what brings the two into one frame. + it("keeps an expanded sub-comp child's lane percentages inside the clip", () => { + const child = { start: 16.5, duration: 2, expandedParentStart: 16 }; + const local = animation("pill-tween", "position", [ + { percentage: 0, properties: { x: 0 } }, + { percentage: 100, properties: { x: 100 } }, + ]); + local.position = 0.5; + local.resolvedStart = 0.5; + local.duration = 2; + + const percentages = getTimelinePropertyLanes( + [local], + clipTimingStart(child), + child.duration, + ).flatMap((lane) => lane.keyframes.map((keyframe) => keyframe.percentage)); + + expect(percentages).toHaveLength(2); + for (const percentage of percentages) { + expect(percentage).toBeGreaterThanOrEqual(0); + expect(percentage).toBeLessThanOrEqual(100); + } + // Falsifier: the raw host-absolute start is what used to be passed. + expect( + getTimelinePropertyLanes([local], child.start, child.duration)[0]?.keyframes[0]?.percentage, + ).toBeLessThan(0); + }); + + it("still lanes a single-group tween exactly once", () => { + const lanes = getTimelinePropertyLanes( + [animation("position-tween", "position", [{ percentage: 0, properties: { x: 0, y: 0 } }])], + 0, + 1, + ); + + expect(lanes.map((lane) => lane.group)).toEqual(["position"]); + }); + + // A lane can merge several tweens, so an edit routed from it must carry the + // clicked keyframe's own animation identity — group matching alone is + // ambiguous once two tweens feed the same lane. + it("routes a mixed-tween lane edit to the tween that owns the keyframe", () => { + const mixed = ungroupedAnimation("entrance", { x: 40, opacity: 1 }); + const sibling = animation("drift", "position", [ + { percentage: 0, properties: { x: 0 } }, + { percentage: 100, properties: { x: 9 } }, + ]); + const lanes = getTimelinePropertyLanes([mixed, sibling], 0, 1); + const position = lanes.find((lane) => lane.group === "position"); + + expect( + resolveTimelineKeyframeTarget(100, position?.keyframes ?? [], [ + { id: "entrance" }, + { id: "drift", propertyGroup: "position" }, + ]), + ).toEqual({ animId: "entrance", tweenPct: 100 }); + }); + it("returns a position lane with synthesized endpoints for a flat tween", () => { const lanes = getTimelinePropertyLanes( [flatAnimation("position-tween", "position", { x: 420 })], diff --git a/packages/studio/src/player/components/TimelinePropertyLanes.tsx b/packages/studio/src/player/components/TimelinePropertyLanes.tsx index 018aa4f8a..398360e44 100644 --- a/packages/studio/src/player/components/TimelinePropertyLanes.tsx +++ b/packages/studio/src/player/components/TimelinePropertyLanes.tsx @@ -29,11 +29,25 @@ export interface TimelinePropertyLanesProps { suppressClickRef?: RefObject; } +/** + * Keys that ride along in a tween's property bag without being animated: a + * transform modifier, Studio's internal endpoint marker, and GSAP's reserved + * `data`. Same exclusion list the parser's classifyTweenPropertyGroup applies — + * without it `{ x, transformOrigin }` would draw a spurious "Other" lane. + */ +const NON_ANIMATED_PROPERTIES = new Set(["transformOrigin", "_auto", "data"]); + +function isAnimatedProperty(property: string): boolean { + return !NON_ANIMATED_PROPERTIES.has(property); +} + function hasGroupProperty( properties: Record, group: PropertyGroupName, ): boolean { - return Object.keys(properties).some((property) => classifyPropertyGroup(property) === group); + return Object.keys(properties).some( + (property) => isAnimatedProperty(property) && classifyPropertyGroup(property) === group, + ); } /** The tween's editable keyframes: its real keyframes, or the start→end pair @@ -42,19 +56,41 @@ function animationKeyframes(animation: GsapAnimation) { return animation.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(animation)?.keyframes ?? []; } -/** A tween contributes a property lane when it has a group and at least one - * editable keyframe (real or synthesized). */ +/** + * Every property group a tween draws a lane for, classified PER PROPERTY. + * `animation.propertyGroup` is the parser's whole-tween verdict and is + * `undefined` for anything spanning more than one group — but `{ x, opacity }` + * is the canonical HyperFrames entrance tween, and reading that verdict gave it + * no caret, no reserved row and no diamonds. classifyPropertyGroup is total, so + * an unrecognised property still lands in "other" rather than vanishing. + * + * Single owner: the rendered lanes (sourceGroups) and the reserved row heights + * (computeLaneCounts) both count groups through here, or they drift. + */ +export function animationLaneGroups(animation: GsapAnimation): PropertyGroupName[] { + const groups = new Set(); + for (const keyframe of animationKeyframes(animation)) { + for (const property of Object.keys(keyframe.properties)) { + if (isAnimatedProperty(property)) groups.add(classifyPropertyGroup(property)); + } + } + return Array.from(groups); +} + +/** A tween contributes a property lane when it animates at least one property + * on at least one editable keyframe (real or synthesized). */ export function animationContributesLane(animation: GsapAnimation): boolean { - return !!animation.propertyGroup && animationKeyframes(animation).length > 0; + return animationLaneGroups(animation).length > 0; } function sourceGroups(animations: readonly GsapAnimation[]) { const groups = new Map(); for (const animation of animations) { - if (!animation.propertyGroup || !animationContributesLane(animation)) continue; - const groupAnimations = groups.get(animation.propertyGroup) ?? []; - groupAnimations.push(animation); - groups.set(animation.propertyGroup, groupAnimations); + for (const group of animationLaneGroups(animation)) { + const groupAnimations = groups.get(group) ?? []; + groupAnimations.push(animation); + groups.set(group, groupAnimations); + } } return groups; } diff --git a/packages/studio/src/player/components/TimelineTrackHeader.test.tsx b/packages/studio/src/player/components/TimelineTrackHeader.test.tsx index 46d4372b5..f8fb06f2c 100644 --- a/packages/studio/src/player/components/TimelineTrackHeader.test.tsx +++ b/packages/studio/src/player/components/TimelineTrackHeader.test.tsx @@ -60,6 +60,7 @@ const OPACITY = animation("opacity-tween", "visual", [ ]); interface RenderHeaderOptions { + keyframeClip?: TimelineElement; animations?: GsapAnimation[]; clipCount?: number; currentTime?: number; @@ -83,7 +84,7 @@ function renderHeader(options: RenderHeaderOptions = {}): { trackNumber={0} trackLabel="Hero card" contentOrigin={LABEL_COL_W} - keyframeClip={ELEMENT} + keyframeClip={next.keyframeClip ?? ELEMENT} clipCount={next.clipCount ?? 1} isExpanded={next.expanded !== false} animations={next.animations ?? [POSITION, OPACITY]} @@ -110,6 +111,53 @@ function click(host: HTMLElement, label: string) { } describe("TimelineTrackHeader", () => { + // An expanded sub-composition child sits on the MASTER timeline at a + // host-absolute start, but its tweens are parsed from its own file and are + // local to it. Feeding the raw start straight into the clip-% math put every + // lane keyframe far outside the clip. + it("keeps an expanded sub-comp child's lane percentages inside the clip", () => { + const child: TimelineElement = { + id: "pill", + tag: "div", + start: 16.5, + duration: 2, + track: 0, + expandedParentStart: 16, + sourceFile: "scene.html", + }; + const local: GsapAnimation = { + id: "pill-tween", + targetSelector: "#pill", + method: "to", + position: 0.5, + resolvedStart: 0.5, + duration: 2, + properties: {}, + propertyGroup: "position", + keyframes: { + format: "percentage", + keyframes: [ + { percentage: 0, properties: { x: 0 } }, + { percentage: 100, properties: { x: 100 } }, + ], + }, + }; + // Playhead at the clip's midpoint (master time), so the 100% keyframe is + // ahead of it. On the raw host-absolute basis every keyframe rebased to a + // large negative percentage and nothing was ever ahead of the playhead. + const view = renderHeader({ + keyframeClip: child, + animations: [local], + currentTime: 17.5, + }); + + expect( + view.host.querySelector('button[aria-label="Next Position keyframe"]') + ?.disabled, + ).toBe(false); + act(() => view.root.unmount()); + }); + // The header shows one clip's lanes, so how many clips the track holds is // otherwise invisible from the label column. A single-clip track stays silent. it("shows the track's clip count only once the track holds more than one clip", () => { diff --git a/packages/studio/src/player/components/TimelineTrackHeader.tsx b/packages/studio/src/player/components/TimelineTrackHeader.tsx index 090629789..06a17eed2 100644 --- a/packages/studio/src/player/components/TimelineTrackHeader.tsx +++ b/packages/studio/src/player/components/TimelineTrackHeader.tsx @@ -4,6 +4,7 @@ import { Music } from "../../icons/SystemIcons"; import type { TimelineElement } from "../store/playerStore"; import type { TimelineEditCallbacks } from "./timelineCallbacks"; import { getTimelinePropertyLanes } from "./TimelinePropertyLanes"; +import { clipTimingStart } from "../../hooks/gsapShared"; import { LayerDisclosureRow } from "./LayerDisclosureRow"; import { TrackClipCount } from "./TrackClipCount"; import { LABEL_COL_W, LANE_H, getTimelineLaneTop } from "./timelineLayout"; @@ -281,7 +282,9 @@ export function TimelineTrackHeader({ ? ((currentTime - keyframeClip.start) / keyframeClip.duration) * 100 : 0; const lanes = keyframeClip - ? getTimelinePropertyLanes(animations, keyframeClip.start, keyframeClip.duration) + ? // clipTimingStart, not the raw start: an expanded sub-comp child's start is + // host-absolute while its tweens are local to its own file. + getTimelinePropertyLanes(animations, clipTimingStart(keyframeClip), keyframeClip.duration) : []; // Label mode = keyframe view; the label column stays LABEL_COL_W (Timeline.tsx // owns the gutter past it, so a 0% diamond isn't clipped by this panel). diff --git a/packages/studio/src/player/components/useTimelineTrackLayout.test.ts b/packages/studio/src/player/components/useTimelineTrackLayout.test.ts index e2d1b6d29..dd07f7f37 100644 --- a/packages/studio/src/player/components/useTimelineTrackLayout.test.ts +++ b/packages/studio/src/player/components/useTimelineTrackLayout.test.ts @@ -6,6 +6,7 @@ import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import { afterEach, describe, expect, it } from "vitest"; import { usePlayerStore, type TimelineElement } from "../store/playerStore"; import { LANE_H, TRACK_H } from "./timelineLayout"; +import { getTimelinePropertyLanes } from "./TimelinePropertyLanes"; import { useTimelineTrackLayout } from "./useTimelineTrackLayout"; globalThis.IS_REACT_ACT_ENVIRONMENT = true; @@ -50,4 +51,38 @@ describe("useTimelineTrackLayout", () => { expect(layout?.rowHeights).toEqual([TRACK_H + LANE_H]); act(() => root.unmount()); }); + + // The row height reserved here and the lanes actually rendered are two + // readings of the same question. They used to be two inline copies of the + // group-set rule, and a mixed-group tween made them disagree: zero reserved + // rows under two rendered lanes. + it("reserves exactly as many rows as the lanes a mixed-group tween renders", () => { + const elements: TimelineElement[] = [ + { id: "clip-1", tag: "div", start: 0, duration: 1, track: 0 }, + ]; + const mixed: GsapAnimation = { + id: "entrance", + targetSelector: "#clip-1", + method: "to", + position: 0, + duration: 1, + properties: { x: 420, opacity: 1 }, + }; + const animations = new Map([["clip-1", [mixed]]]); + usePlayerStore.setState({ expandedClipIds: new Set(["clip-1"]) }); + + let layout: ReturnType | undefined; + function Probe() { + layout = useTimelineTrackLayout(elements, animations, null, new Set()); + return null; + } + + const root = createRoot(document.createElement("div")); + act(() => root.render(React.createElement(Probe))); + + expect(getTimelinePropertyLanes([mixed], 0, 1)).toHaveLength(2); + expect(layout?.laneCounts.get("clip-1")).toBe(2); + expect(layout?.rowHeights).toEqual([TRACK_H + 2 * LANE_H]); + act(() => root.unmount()); + }); }); diff --git a/packages/studio/src/player/components/useTimelineTrackLayout.ts b/packages/studio/src/player/components/useTimelineTrackLayout.ts index 4aad5a876..677831fb5 100644 --- a/packages/studio/src/player/components/useTimelineTrackLayout.ts +++ b/packages/studio/src/player/components/useTimelineTrackLayout.ts @@ -1,6 +1,6 @@ import { useMemo, useRef } from "react"; import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; -import { animationContributesLane } from "./TimelinePropertyLanes"; +import { animationLaneGroups } from "./TimelinePropertyLanes"; import { usePlayerStore, type TimelineElement } from "../store/playerStore"; import { STUDIO_KEYFRAMES_ENABLED } from "../../components/editor/manualEditingAvailability"; import type { DraggedClipState } from "./timelineClipDragTypes"; @@ -56,9 +56,9 @@ function computeLaneCounts( const clipId = element.key ?? element.id; const propertyGroups = new Set(); for (const animation of gsapAnimations.get(clipId) ?? []) { - if (animation.propertyGroup && animationContributesLane(animation)) { - propertyGroups.add(animation.propertyGroup); - } + // Same helper the rendered lanes count through, so a reserved row and a + // drawn lane can never disagree. + for (const group of animationLaneGroups(animation)) propertyGroups.add(group); } laneCounts.set(clipId, propertyGroups.size); }