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
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
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.
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.
6bc8d54 to
913fe08
Compare
86e9735 to
0b9eff8
Compare
3 participants
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.