fix(studio): sub-composition clip timing and expanded rows - #2845
Conversation
…me percentages A ruler press with no pointer movement settled the playhead at t=0 instead of the clicked time. handlePointerUp replays pendingClientXRef, which only the pointermove path wrote, so a plain click fell back to the ref's initial 0 and overwrote the correct pointerdown seek. Seed the ref on pointerdown. The keyframe retime move branch also returned the raw quotient while the resize branch rounded to 3dp, so values like 74.81203007518799% landed in the user's source and churned the diff on every drag. Round at the point of computation so the no-op test and the written value agree.
The 24x24 WCAG 2.5.8 overlay sits on a wrapper that outranks the diamonds, so on a segment narrower than 24px it overhung them and won their hit test at fit zoom. Gate the overlay on the clear span between the two diamonds and let the button keep its 16x16 box below that. Also renames the pointer target suite to say it asserts the classes that produce the size, not the measured geometry, which happy-dom cannot see.
…ide press Split hex-draft ownership so the hex input is the sole author of its own text while editing (updateColorDraft no longer stamps a canonical hex back over every keystroke), fixing snap-back on backspace and the silent wrong-colour clobber on non-repeating hex values. Route hex typing through the shared gesture transaction so outside-click and Escape settle/cancel it like the other inspector fields, instead of relying on a private onBlur commit that never fires once the panel unmounts on outside-click.
Swap the panel's hand-rolled bubble-phase mousedown listener for the shared useContextMenuDismiss hook, which adds Escape support and fixes outside-click dismissal when a canvas gesture (e.g. marquee start) calls preventDefault on pointerdown, which otherwise suppresses the mousedown compat event entirely. Also wires up dialog ARIA (role, aria-modal, id/aria-controls) between the trigger and panel.
The panel does not trap focus and leaves the rest of the editor operable, so aria-modal would tell assistive tech the whole app is inert while it is open. role=dialog plus aria-controls and Escape is the correct non-modal disclosure shape.
The gesture resolver gated on six digits while parseCssColor accepts both lengths, so #F00 previewed as nothing and committed nothing. The old onBlur path parsed it, making this a behavioural loss rather than a pre-existing gap. Also asserts that an incomplete hex is restored on outside-click, not merely left uncommitted.
A sub-composition tween's resolvedStart is composition-local, while the timeline element resolved for it is the sub-comp HOST, whose start is main-timeline absolute. toClipPercentage subtracted the two frames from each other, so a host mounted at 1.5s cached its 0s tween at -12% and its last tween's end keyframe at 88% instead of 100%. A clip-relative percentage can never be negative. resolveClipTimingBasis now returns the clip start in the frame the tween's own times are measured in: the composition mount (expandedParentStart for an expanded child, the parent composition clip's start otherwise, 0 for a root-composition element) is subtracted, and a sub-comp inner element that falls back to its host's window starts at 0 in that window. It moves to gsapShared so the post-commit cache writer can share it instead of resolving its own basis, which also gives that writer the sub-comp host fallback it was missing.
buildExpandedElements synthesized DOM-only sub-composition children against the top-level element rather than the parentHost it resolves immediately after. Under two-level nesting every child row therefore inherited the top-level window instead of its own host's, so the rows drew at the wrong offset and duration.
The sub-composition drill-in replaced the host row with its children. Since expansion is also driven by the playhead alone (paused auto-expand), an ordinary seek into a sub-composition made the host row disappear, taking its keyframe lane with it: diamonds render per row from keyframeCache.get( elementKey), so no row means no diamonds. Reproduced live with no drag at all, seek 0 gave 3 diamonds, seek 7.68 gave 0, seek 0.2 gave 3. Make the expansion additive instead. The host row stays and its children are appended directly below it. The synthetic fractional lanes already used for children sit strictly between the host's lane and the next integer, so the host keeps its own row without colliding with anything. The time-keyed auto-expand itself is unchanged.
da08719 to
d48440a
Compare
7be28e4 to
86f633f
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at d48440a.
Load-bearing correctness on all four bugs, with the two-frame semantic shift (element .start is main-timeline absolute, tween resolvedStart is composition-local) captured properly in the new resolveClipTimingBasis docblock. The bug-1 test at gsapShared.test.ts:143 ("puts a tween on the host's first frame at 0%, never below zero") is exactly the shape of regression test that would have caught the -12% cache miss the first time. The seek-0/7.68/0.2 repro test at useExpandedTimelineElements.test.ts:393 is a nice port of the live-observed defect straight into the harness.
The move to gsapShared is the right consolidation — before this PR, updateKeyframeCacheFromParsed at gsapKeyframeCacheHelpers.ts:23-35 had its own inline basis lookup (elements.find(...)) while keyframeCacheAstLoad.ts had the sub-comp-host fallback, so the two writers really could disagree on where a keyframe belonged. One helper, one truth. Verified all four call sites now go through the shared path: useTimelineEditCallbacks.ts:269 (drag commit), gsapKeyframeCacheHelpers.ts:37 (post-commit), keyframeCacheAstLoad.ts:136 (AST load), useGsapTweenCache.ts:262 (main hook).
Concerns
-
resolveClipTimingBasisfallback for a missing host defaultselDurationto1.gsapShared.ts:322returns{ elStart: 0, elDuration: host?.duration ?? 1 }when neither the element nor a host resolves. Any tween that lands on this path (a "ghost" element the store doesn't know about — should not happen but is defensively defaulted) will have every keyframe rebased against a 1-second clip: a resolvedStart of 5 →toClipPercentage(5, 0, 1)= 500%. Not new — the pre-move code did the same — but worth trapping with an assertion or aconsole.warnin dev builds so a real production case surfaces instead of silently landing 500%-percentages in the cache. Non-blocking. -
buildExpandedElementsfallback whenparentHostis missing.useExpandedTimelineElements.ts:265usesparentHost ?? topLevelElementwhen synthesizing DOM-only children. If the nested sub-comp host has no manifest entry (only a DOM presence), the fallback still lands ontopLevelElement— the same bug the fix addresses for the manifest-present case. Unclear whether this can happen in practice (adata-composition-srcelement without a manifest clip); if it can, the test at:425covers only the manifest-present shape. Non-blocking; worth understanding whether there's a real path here or the manifest is always the source of truth.
Nits
- The unit-window fallback
{elStart: 0, elDuration: 1}atgsapShared.ts:322is defensive but has the same shape as the pre-PRelDuration: 1default the PR body calls out as the original bug ("without a basis, elDuration defaulted to 1 and clip-relative keyframe percentages blew past 100%"). It's fine for a ghost element to fall through here, but the choice deserves a one-line note explaining the fallback IS the terminal case, not the sub-comp fallback.
What I didn't verify
- Manual repro of the drill-in-vanishes-on-seek bug in a real Studio session — trusting the "3011 passing" + the
resolveTimelineExpansionRawId-based test coverage. - Sibling PR #2846's use of
resolveClipTimingBasis— reviewing it next as the top of the stack.
— Review by Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Three fixes in one primitive: resolveClipTimingBasis moves to gsapShared, gains expandedParentStart/parentCompositionId-aware mount subtraction, and is now the single source of truth for all three keyframe-cache writers (gsapKeyframeCacheHelpers, useGsapTweenCache, keyframeCacheAstLoad) plus the drag-retime path (useTimelineEditCallbacks). Row expansion becomes additive so the host row survives paused auto-expand, and DOM-only children synthesize against parentHost instead of the top-level element. Math is right, coverage is targeted, PR body faithfully describes the four defects.
Findings
P2 — Deep-drill (≥2 levels) middle-host row still vanishes
packages/studio/src/player/hooks/useExpandedTimelineElements.ts:294-296 preserves only the outermost topLevelElement in the additive filter ((el.key ?? el.id) === parentKey). Every intermediate composition host between topLevelId and siblingParentId is in parentMap, has a key ≠ parentKey, and is filtered out. In playhead-driven auto-expand, findActiveExpandableCompositionId returns the deepest active composition (useExpandedTimelineElements.ts:87-95), so drilling from A > B > C picks siblingParentId = B, keeps A, replaces A's original expansion with [B's children], and drops B — which is exactly the class of defect Bug 4 fixes at level 1. B's diamonds render from keyframeCache.get(B.key), so its lane vanishes on a plain seek that lands inside B. Recommendation: extend the OR clause to walk the drill chain, e.g. (el.key ?? el.id) === parentKey || drillChainKeys.has(el.key ?? el.id) || !parentMap.has(el.domId ?? el.id), where drillChainKeys collects every ancestor from siblingParentId up to topLevelId. The spans DOM-only children over their nested host's window test synthesizes sub-host only in the manifest (not in elements), so the current suite doesn't exercise this.
P2 — Sub-comp inner with parentCompositionId but parent absent from elements silently reverts to main-timeline frame
packages/studio/src/hooks/gsapShared.ts:298-303. mount = direct.expandedParentStart ?? parent?.start ?? 0. When parentCompositionId is set but the parent element is not in elements (e.g. a transient store update where the child observes before the parent, or the parent has been pruned), parent is undefined, mount falls to 0, and elStart becomes the raw main-timeline direct.start — the exact frame mismatch bug 1 fixes at the domClipChildren fallback. It would then read dropAbsTime in useTimelineEditCallbacks.ts:276 as main-timeline while resolveTweenStart(anim) remains composition-local, corrupting the retime decision. Recommendation: when the direct hit declares a parentCompositionId we can't resolve, prefer the domClipChildren host-mount fall-through over mount=0, or at least add a defensive test that pins the current behavior.
Nit — Retime round-trip not covered end-to-end
packages/studio/src/hooks/gsapShared.test.ts:166-173 proves the basis round-trips through toClipPercentage, but the composition-local semantics also silently flow into useTimelineEditCallbacks.ts:269-282 (dropAbsTime) → resolveKeyframeRetime, and from there into handleGsapUpdateMeta/handleGsapResizeKeyframedTween which persist position. Sub-comp drag-retime is fixed by proxy but has no dedicated regression test; a future semantics tweak to resolveClipTimingBasis (e.g. an inverted mount sign) would corrupt persisted tween.position on sub-comp drags without any assertion firing. One integration test that drags a sub-comp keyframe and asserts the persisted position stays composition-local would close this.
Nit — Hard-coded "index.html" in the host-key fallback
packages/studio/src/hooks/gsapShared.ts:307. `index.html#${hostId}` is pre-existing behavior (unchanged from the old keyframeCacheAstLoad version), and the el.domId === hostId OR-branch typically covers it in practice, but a sub-comp host authored in a non-root file with no domId would fall through. Worth parameterising to the caller's sourceFile in a follow-up.
Verdict
COMMENT. Fix is correct, minimal-scope, well-factored — all three cache writers converge on one basis primitive, and the semantics shift is internally consistent across toClipKeyframes ↔ useTimelineEditCallbacks. B+ (bug 4's class of defect still persists at ≥2 nesting depth; the fix is scoped strictly to the outermost host).
— Review by Via
Drilling two levels deep spared only the top-level row, so the middle host lost its row and its keyframe lane with it. Spare every host between the drilled one and the top, and anchor the children under the deepest host that actually has a row. Also stops resolveClipTimingBasis handing back a main-timeline start when a clip names a parent composition that is absent from the element list. The mount is unknowable there, so the child's own window is the only safe frame.
…-clip-timing fix(studio): sub-composition clip timing and expanded rows
3 participants
What
Sub-composition rows in the timeline were timed against the wrong frame of reference, and could vanish entirely on an ordinary seek.
Bugs fixed
resolvedStartis composition-local, while the timeline element resolved for it is the sub-comp HOST, whose start is main-timeline absolute.toClipPercentagesubtracted those two frames from each other, so a host mounted at 1.5s cached its 0s tween at -12% and its last tween's end keyframe at 88% instead of 100%. A clip-relative percentage can never be negative.buildExpandedElementssynthesized DOM-only sub-comp children against the top-level element rather than theparentHostit resolves immediately after, so children inherited the top-level window instead of their own host's.keyframeCache.get(elementKey), so no row means no diamonds. Reproduced live with no drag at all: seek 0 gave 3 diamonds, seek 7.68 gave 0, seek 0.2 gave 3.Why
Bug 1 puts keyframes outside the clip they belong to, which is not a display nicety: the cached percentage is what the diamonds, the lanes, and the retime math all read. Bug 4 makes keyframes disappear during normal playback scrubbing.
How
resolveClipTimingBasisnow returns the clip start in the frame the tween's own times are measured in, and moves togsapSharedso the post-commit writer shares it instead of resolving its own. A sub-comp inner element that falls back to its host's window starts at 0 in that window.Expansion becomes additive: the host row stays and its children are appended below it. The synthetic fractional lanes already used for children sit strictly between the host's lane and the next integer, so the host keeps its own row without colliding. The time-keyed auto-expand itself is unchanged.
Test plan
Studio suite green on this branch: 3011 passing, 0 failures. Typecheck, lint and format clean.
Not covered
packages/core, not Studio: a decorative element carryingdata-composition-idahead of the real root wins document order, which collapses every authored scene into one timeline row. It is deliberately not in this stack, because it changes runtime resolution for every consumer and needs its own PR gated on the producer render baselines.