feat(studio): add keyframe track headers#2785
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a9bdade to
6bc8d54
Compare
7f86018 to
86e9735
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — head 86e9735.
Track-header contract is clean, the extract-to-helper is safe (both KeyframeNavigation callsite and the new lane header type-check under the generic NavigableKeyframe), and the alignment test (assertAligned) pins the pixel-perfect lane contract with #2784. Findings below are all P2/P3, non-blocking.
Adversarial lenses
Lens D — Interaction bleed with property lanes (P2). Half the controls stop propagation, half don't. Consistent stoppers: LayerDisclosureRow's caret (packages/studio/src/player/components/LayerDisclosureRow.tsx:100-104, both onPointerDown and onClick) and VisibilityButton (TimelineTrackHeader.tsx:559-563). Non-stoppers: PropertyGroupNavigation's prev/next chevrons (TimelineTrackHeader.tsx:736,745 — onClick={() => seekTo(...)} with no stopPropagation) and the toggle diamond (TimelineTrackHeader.tsx:832-836). Since sibling buttons in the same absolute-positioned label column deliberately stop propagation, clicks that leak from the chevrons/toggle up to whatever the ancestor track-row does are the likely regression path here. Match the sibling pattern.
Lens B — Truncation without tooltip fidelity (P2). Three ellipsis'd spans have no title attribute → mouse users get no full-name reveal, touch users are stuck: layer name in LayerDisclosureRow.tsx:119 (min-w-0 flex-1 truncate font-medium), track label in LegacyTrackHeader at TimelineTrackHeader.tsx:590, and the value readout at TimelineTrackHeader.tsx:841. The two <button>s in the same subtree already carry title= — adding it to the truncatable spans is a two-line fix.
Lens E — Contract with #2784 (verified clean). Header count = lane count (both derive from getTimelinePropertyLanes(animations, ...); the LegacyTrackHeader fallback fires when lanes.length === 0). [data-timeline-lane-top] offsets match TimelinePropertyLanes via getTimelineLaneTop(laneIndex) — asserted by the alignment test at TimelineTrackHeader.test.tsx:342-386. No orphan header/lane path.
Lens A — Disclosure state (deferred, not new). State ownership is upstream (isExpanded is a prop). Caret click is isolated to the button; wrapper <div> has no click handler, so header-body vs. caret can't diverge here.
Lens C — Reorder (N/A). No drag-to-reorder in this PR.
Nits (P3)
TimelineTrackHeader.tsx:519-524—visualValueReadoutshows raw values when|opacity| > 1, which for a percentage readout is a leaky formatting branch. Won't fire on valid GSAP opacity.TimelineTrackHeader.test.tsx:245-249— locks in the linear-sampling behaviour (tweenPercentage: 25→opacity: 0.25). Per the PR body, the ease-following sampler lands at the stack tip; that PR will need to update this expectation.- The 4 deferred findings enumerated in the PR body all match the diff — verified.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 86e9735511ec.
R2 adversarial pass on the track-headers slice. Three orange a11y items on the interactive layer: LayerDisclosureRow trigger missing aria-expanded/aria-controls (disclosure widget's whole contract), the hover-only Eye button unmounting means keyboard users can't reach it on any lane where laneIndex !== 0, and the O(lanes × keyframes) hover-driven recompute on every pointer-enter/leave. Plus the same propertyGroup?: string widening pattern Via flagged in a different form.
🟢 Verified clean
- KeyframeNavigation.tsx: getKeyframeNavigationState generic extraction preserves prev/current/next semantics + TOLERANCE boundaries
- timelineLayout.ts: single additive LABEL_COL_W=232 constant, no behavior change
- TimelineTrackHeader.test.tsx: happy-dom coverage of disclosure + navigation
Complements Via's parallel adversarial pass (Family B rollup). Where Via found 0 P1 / 17 P2 / 18 P3, the adversarial subagent pass here surfaced additional depth on the mutation-authority thread and the Promise chain.
| background: gutterBackground, | ||
| }} | ||
| > | ||
| <button |
There was a problem hiding this comment.
🟠 Disclosure trigger missing aria-expanded and aria-controls
<button
type="button"
aria-label={`${isExpanded ? "Collapse" : "Expand"} ${name} keyframes`}
title={`${isExpanded ? "Collapse" : "Expand"} keyframe lanes`}
className="flex h-5 w-4 shrink-0 items-center justify-center rounded border-0 bg-transparent p-0 text-white/55 hover:text-white focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"Canonical WAI-ARIA disclosure pattern (button toggling a related content region — property-group lane rows when isExpanded). Should expose aria-expanded={isExpanded} so AT can announce the state change, and aria-controls pointing at the lane region id. Baking state into aria-label requires refocus/re-announce and doesn't surface to browsers/AT as a live expanded state.
Fix: Add aria-expanded={isExpanded} (and aria-controls={laneRegionId} with matching id on the lane container in TimelineTrackHeader.tsx). Keep aria-label static (e.g. ${name} keyframes) so state comes from aria-expanded.
| style={{ transform: isExpanded ? "rotate(90deg)" : undefined }} | ||
| /> | ||
| </button> | ||
| <span |
There was a problem hiding this comment.
🟡 aria-label on non-interactive is ignored by AT
<span
aria-label="Layer keyframe indicator"
className="shrink-0 text-[13px] leading-none text-white/40"
>
◇
</span>Per ARIA-in-HTML, aria-label is only exposed on elements with a widget/landmark role or implicit interactive role. Bare <span> is generic content, so screen readers ignore this label and read the literal ◇ (or nothing on font substitution). Meaningless to AT.
Fix: Either drop the aria-label and mark decorative with aria-hidden="true", or give the span role="img" so label is exposed. Neighboring layer name already communicates row context — aria-hidden is least noisy.
| onRazorSplit?: (element: TimelineElement, splitTime: number) => Promise<void> | void; | ||
| onRazorSplitAll?: (splitTime: number) => Promise<void> | void; | ||
| onDeleteKeyframe?: (elementId: string, percentage: number) => void; | ||
| onDeleteKeyframe?: ( |
There was a problem hiding this comment.
🟠 Type widening: propertyGroup?: string in callbacks that already have PropertyGroupName in scope
onDeleteKeyframe?: (
elementId: string,
percentage: number,
propertyGroup?: string,
tweenPercentage?: number,
animationId?: string,
) => void;File imports PropertyGroupName two lines above and uses it for TimelinePropertyGroupKeyframeToggle.propertyGroup. The three widened signatures here (onDeleteKeyframe, onMoveKeyframeToPlayhead, onMoveKeyframe) accept strictly-wider string, silently permitting typo'd or invalid group names (e.g. "positon", "opacity") at every call site.
Fix: Change all three occurrences of propertyGroup?: string to propertyGroup?: PropertyGroupName — import already in place; every producer already types the value as PropertyGroupName.
| type="button" | ||
| aria-label={`Previous ${label} keyframe`} | ||
| disabled={!navigation.prevKeyframe} | ||
| className="h-5 w-3 border-0 bg-transparent p-0 text-white/55 hover:text-white disabled:text-white/15" |
There was a problem hiding this comment.
🟡 WCAG 2.5.8 target-size failure on prev/next/toggle keyframe buttons (20×12 / 20×16)
className="h-5 w-3 border-0 bg-transparent p-0 text-white/55 hover:text-white disabled:text-white/15"
// prev/next chevrons: h-5 w-3 = 20×12 CSS px
// toggle diamond at line 441:
className="flex h-5 w-4 shrink-0 items-center justify-center border-0 bg-transparent p-0 text-[11px] text-[#3CE6AC] focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]"
// = 20×16 CSS pxWCAG 2.2 SC 2.5.8 requires 24×24 CSS-pixel target (or equivalent spacing). At h-5 w-3 (20×12) and h-5 w-4 (20×16) with adjacent gap-0.5 packing, no spacing exemption applies. Hard to hit for touch or low-precision-pointer users.
Fix: Bump to h-6 w-6 (24×24) with icon-sized inner glyph, or wrap in larger hit target while keeping visual glyph small (e.g. before:absolute before:inset-[-4px] spacers).
| expandedElement={expandedElement} | ||
| onSeek={onSeek} | ||
| > | ||
| <button |
There was a problem hiding this comment.
🟡 Toggle-state buttons missing aria-pressed
<button
type="button"
aria-label={`Toggle ${label} keyframe`}
title={`${navigation.currentKeyframe ? "Remove" : "Add"} ${label} keyframe`}
className="flex h-5 w-4 ... focus-visible:outline ..."
onClick={() => {
if (expandedElement && toggleTarget) {
void onTogglePropertyGroupKeyframe?.(expandedElement, toggleTarget);
}
}}
>
{navigation.currentKeyframe ? "◆" : "◇"}
</button>Two-state visual (filled ◆ = keyframe present, hollow ◇ = absent); click flips that state — exactly the aria-pressed pattern. Without it, AT users can't tell whether pressing adds or removes (glyph inaccessible; aria-label state-agnostic). Same applies to VisibilityButton around line 161-166 where hidden/visible is toggled.
Fix: Add aria-pressed={!!navigation.currentKeyframe} to the diamond toggle, and aria-pressed={hidden} (with state-agnostic aria-label like Track ${trackNumber} visibility) to VisibilityButton.
| clipPercentage, | ||
| ); | ||
| const showEye = | ||
| hoveredGroup === lane.group || |
There was a problem hiding this comment.
🟠 Hover-only eye button keyboard-unreachable on non-first lanes
const showEye =
hoveredGroup === lane.group ||
(hoveredGroup === null && laneIndex === 0 && (isActive || isHovered));
// VisibilityButton line 159 (when !visible):
if (!visible) return <span aria-hidden="true" className="h-6 w-6 shrink-0" />;When showEye is false, VisibilityButton returns aria-hidden span — button UNMOUNTED, not just hidden. hoveredGroup is only set by pointer-enter/leave (line 409-410), so keyboard-only user can't reach the eye on any lane where laneIndex !== 0, and can't reach it on lane 0 either when track is neither active nor hovered. Regresses keyboard parity with legacy header (which renders eye unconditionally, line 200-205).
Fix: Render the button in the DOM at all times; gate visibility with opacity/visibility CSS plus :focus-within/:focus-visible reveal rule. Ensure Tab order includes exactly one eye per row.
| (hoveredGroup === null && laneIndex === 0 && (isActive || isHovered)); | ||
|
|
||
| return ( | ||
| <div |
There was a problem hiding this comment.
🟠 Hover state triggers O(lanes × keyframes) recompute on every pointer enter/leave
onPointerEnter={() => setHoveredGroup(lane.group)}
onPointerLeave={() => setHoveredGroup(null)}Each pointer transition sets hoveredGroup on the parent TimelineTrackHeader, re-running getTimelinePropertyLanes(animations, keyframeClip.start, keyframeClip.duration) at line 485 (unmemoized) and resolveLaneHeaderState for every lane. That runs getKeyframeNavigationState (filter over keyframes), findNearestLaneKeyframe (reduce), findAnimationAtTime (iter animations), and valuesAt (nested iter over keyframes × properties with classifyPropertyGroup per property). Sliding mouse across N lanes fires 2N re-renders, each O(lanes × keyframes × propsPerKeyframe).
Fix: Move hover-visibility to pure CSS (e.g. group-hover/peer-hover/:hover on lane row) so no React state change; or lift hoveredGroup into per-lane state, and useMemo getTimelinePropertyLanes + resolveLaneHeaderState per lane keyed on animations/currentTime/clipPercentage.
| ? ((currentTime - keyframeClip.start) / keyframeClip.duration) * 100 | ||
| : 0; | ||
| const lanes = keyframeClip | ||
| ? getTimelinePropertyLanes(animations, keyframeClip.start, keyframeClip.duration) |
There was a problem hiding this comment.
🟡 Label-column overflow when contentOrigin < LABEL_COL_W in keyframe mode
const showTrackLabel = contentOrigin >= LABEL_COL_W;
const isKeyframeLayer = !!keyframeClip && lanes.length > 0;
...
// line 494:
width: showTrackLabel ? LABEL_COL_W : contentOrigin,When isKeyframeLayer && !showTrackLabel (a keyframe clip on a narrow track-label gutter), the outer sticky div's width collapses to contentOrigin, but LayerDisclosureRow and every PropertyGroupHeaderRow lay out at fixed width: LABEL_COL_W (232px). Children extend past parent, potentially overlapping timeline lane content to the right and clipping visibility/focus for buttons inside. No explicit guard for this combination.
Fix: Either force showTrackLabel = true (clamp width to LABEL_COL_W) whenever isKeyframeLayer, or short-circuit keyframe layout back to LegacyTrackHeader when contentOrigin < LABEL_COL_W.
6bc8d54 to
913fe08
Compare
86e9735 to
0b9eff8
Compare
The prev/next keyframe chevrons and the group toggle diamond let their click bubble to the ancestor track row, so seeking to a keyframe also reselected the track. The disclosure caret and the eye already stop it; these now match. Truncated labels (layer name, track label, group label, value readout) also carry a title so the full text is reachable on hover.
The header file owned value sampling, readout formatting, lane-state resolution and the JSX at once, so a formatting change and a layout change edited the same file. Sampling and formatting now live in trackHeaderLaneValues, lane-state resolution in trackHeaderLaneState, and resolveLaneHeaderState returns only the four fields its caller reads. Also shows the track's clip count next to the track identity, which the header promised but never rendered.
LegacyTrackHeader reads as deprecated code. It is the live path for every track that has no keyframe clip to disclose, so call it PlainTrackHeader and say so in a comment.
0b9eff8 to
cf1cc8c
Compare
913fe08 to
0509de7
Compare
What
Adds compact keyframe-aware track headers and lane expansion controls.
Why
Users need a clear, low-density way to understand track identity, clip count, visibility, and whether property lanes are expanded.
How
Introduces the track-header presentation and interaction contract while keeping selection and expansion state owned by the existing timeline model. This is B5 of the Family B Graphite stack.
Review fixes land in the stack tip (#2690): state-specific toggle labels (
Add <Group> keyframe/Remove <Group> keyframe), and property-value sampling that follows the segment's own ease instead of interpolating linearly. The linear sampling reported a value the element never has mid-segment, and stamped that wrong value onto keyframes inserted from the header.Test plan
Exact Family B tip validation: 2,865 Studio tests, 67 Studio Server route tests, both package typechecks, formatting, lint, diff check, file-size gate, and Fallow audit passed.
Deferred review findings
Every blocker and high finding raised on this PR is fixed in the stack. The 4 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/05-pr-2687-deferred-review-findings.md:packages/studio/src/player/components/timelineCallbacks.ts:74— New optional keyframe-callback params widen propertyGroup to string, diverging from PropertyGroupName used on the sibling typepackages/studio/src/player/components/TimelineTrackHeader.tsx:482— hoveredGroup useState causes full lane recompute on every pointer enter/leave (flicker-lens)packages/studio/src/player/components/LayerDisclosureRow.tsx:47— Decorative ◇ glyph labels an unlabelablepackages/studio/src/player/components/TimelineTrackHeader.tsx:491— Keyframe-layer branch renders LABEL_COL_W children inside a parent whose width can be smallerSupersedes #2687, which was closed when
mainwas rewritten to unwind an early landing of this stack. Same head commit, same review history.R1 review follow-ups
Fixed in this PR:
titleattributes, plus the layer disclosure row label.The 2 remaining low findings are parked in
.scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.R2 review follow-ups
Fixed in this PR:
TimelineTrackHeader.tsxowned value sampling, readout formatting, lane-state resolution and the JSX at once, so a formatting change and a layout change edited the same file. Sampling and formatting moved totrackHeaderLaneValues, lane-state resolution totrackHeaderLaneState, andresolveLaneHeaderStatereturns only the four fields its caller reads. The header also renders the per-track clip count it previously described but never showed.LegacyTrackHeaderread as deprecated code while being the live path for every track with no keyframe clip to disclose. It is nowPlainTrackHeader, with a comment saying which tracks take it.