feat(studio): add keyframe track headers#2687
Conversation
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 3b9d73aadcc4.
Track-header slice. Three orange a11y items on the interactive layer: disclosure trigger missing aria-expanded (its whole contract), stateful keyframe-toggle button missing aria-pressed, prev/next chevron buttons missing focus-visible outline AND < 24px hit target. Plus one type-widening + one hover-driven full-lane-recompute perf item.
🟢 Verified clean
- Selection/expansion ownership check: isExpanded, keyframeClip, and onToggleClipExpanded are all passed as props — no local expansion useState duplicates upstream state. Only hoveredGroup is local, and it is UI-only (pointer feathering) — no race with the store.
- getKeyframeNavigationState refactor is behavior-preserving for the KeyframeNavigation caller (property is always passed); the new no-property branch (property ? filter : keyframes) is exercised only by the new lane callers.
- TimelineTrackHeader.test.tsx covers the group-scoped toggle target, per-group prev/next seeking, diamond fill state, value readout formatting, prev-disabled at first keyframe, and lane-row top alignment vs TimelinePropertyLanes.
Cross-stack notes
No aria-attribute assertions in the tests would catch findings 1 & 2 automatically — worth suggesting a test-suite pattern in the rollup. Nothing that raises concern for #2688-#2690 consumers of these callbacks; propertyGroup widening (finding 4) is contained to this file.
| background: gutterBackground, | ||
| }} | ||
| > | ||
| <button |
There was a problem hiding this comment.
🟠 LayerDisclosureRow trigger lacks aria-expanded/aria-controls — the required disclosure a11y contract
<button
type="button"
aria-label={`${isExpanded ? "Collapse" : "Expand"} ${name} keyframes`}
title={`${isExpanded ? "Collapse" : "Expand"} keyframe lanes`}
onPointerDown={(event) => event.stopPropagation()}
onClick={...}
>
<CaretRight ... />
</button>This is the disclosure widget the code comment explicitly names ("disclosure lives here ... re-expands a collapsed layer") but the trigger has neither aria-expanded={isExpanded} nor aria-controls pointing at the lane region it toggles. SR users get a swapping verb in the label but no programmatic expanded/collapsed state — the one attribute WAI-ARIA requires for a disclosure trigger. This is the widget's whole a11y contract.
Fix: Add aria-expanded={isExpanded}. If the property-lane region has a stable id, also set aria-controls to it (PropertyGroupHeaderRow container would be a good anchor).
| expandedElement={expandedElement} | ||
| onSeek={onSeek} | ||
| > | ||
| <button |
There was a problem hiding this comment.
🟠 Property-group keyframe toggle button label is static across on/off states
<button
type="button"
aria-label={`Toggle ${label} keyframe`}
title={`${navigation.currentKeyframe ? "Remove" : "Add"} ${label} keyframe`}
...
>
{navigation.currentKeyframe ? "◆" : "◇"}
</button>Stateful toggle (empty diamond → add keyframe, filled diamond → remove existing). Sighted users get title="Remove/Add" plus the ◆/◇ glyph. SR users get "Toggle Position keyframe" regardless of state — state is only exposed by a decorative glyph.
Fix: Add aria-pressed={!!navigation.currentKeyframe} (best fit — this is the pressed/unpressed pattern) OR mirror the title into the aria-label.
| }; | ||
| return ( | ||
| <span className="flex shrink-0 items-center gap-0.5"> | ||
| <button |
There was a problem hiding this comment.
🟠 Prev/next keyframe chevron buttons drop focus-visible outline + hit target < 24×24
<button
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"
onClick={() => seekTo(navigation.prevKeyframe)}
>
‹
</button>Every other interactive control in this PR (VisibilityButton, LayerDisclosureRow disclosure, group-keyframe toggle) carries focus-visible:outline focus-visible:outline-1 focus-visible:outline-[#3CE6AC]; these two chevron buttons — the ONLY way to move the playhead between group keyframes from the header — have none. Also the hit target is 12×20 px, below WCAG 2.5.8's 24 px minimum. Keyboard-only users tabbing through see no focus ring.
Fix: Apply the same focus-visible outline classes used on sibling buttons; consider widening beyond w-3.
| 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.
🟡 New optional keyframe-callback params widen propertyGroup to string, diverging from PropertyGroupName used on the sibling type
import type { PropertyGroupName } from "@hyperframes/core/gsap-parser";
export interface TimelinePropertyGroupKeyframeToggle {
animationId: string;
propertyGroup: PropertyGroupName;
...
}
...
onDeleteKeyframe?: (
elementId: string,
percentage: number,
propertyGroup?: string,
tweenPercentage?: number,
animationId?: string,
) => void;PropertyGroupName is already imported and used verbatim two dozen lines earlier for TimelinePropertyGroupKeyframeToggle.propertyGroup, yet the three new optional params on onDeleteKeyframe, onMoveKeyframeToPlayhead, onMoveKeyframe type propertyGroup as plain string. Every real call site passes a PropertyGroupName (values come out of classifyPropertyGroup and the lane structure), so the widening buys nothing and costs exhaustiveness checks + autocomplete + refactor safety when the enum grows.
Fix: Change propertyGroup?: string to propertyGroup?: PropertyGroupName across all three new signatures.
| onTogglePropertyGroupKeyframe, | ||
| onSeek, | ||
| }: TimelineTrackHeaderProps) { | ||
| const [hoveredGroup, setHoveredGroup] = useState<PropertyGroupName | null>(null); |
There was a problem hiding this comment.
🟡 hoveredGroup useState causes full lane recompute on every pointer enter/leave (flicker-lens)
const [hoveredGroup, setHoveredGroup] = useState<PropertyGroupName | null>(null);
const clipPercentage = keyframeClip
? ((currentTime - keyframeClip.start) / keyframeClip.duration) * 100
: 0;
const lanes = keyframeClip
? getTimelinePropertyLanes(animations, keyframeClip.start, keyframeClip.duration)
: [];hoveredGroup is local state changed on every onPointerEnter/onPointerLeave inside PropertyGroupHeaderRow; its only downstream consumer is the eye-visibility feathering (showEye at :404-406). Yet every hover setState re-runs the parent, which re-runs getTimelinePropertyLanes (iterates all animations × keyframes) and re-runs resolveLaneHeaderState per lane. HF flicker-lens: O(animations × keyframes) work on every mouse motion between adjacent lanes. Fine for 3-lane clip; noticeable if a track carries many animated properties.
Fix: useMemo(() => getTimelinePropertyLanes(...), [animations, keyframeClip?.start, keyframeClip?.duration]); OR hoist hoveredGroup into PropertyGroupHeaderRow so its updates don't re-render the parent.
| style={{ transform: isExpanded ? "rotate(90deg)" : undefined }} | ||
| /> | ||
| </button> | ||
| <span |
There was a problem hiding this comment.
🟢 Decorative ◇ glyph labels an unlabelable
<span
aria-label="Layer keyframe indicator"
className="shrink-0 text-[13px] leading-none text-white/40"
>
◇
</span>Per ARIA in HTML §5.4, aria-label is prohibited on generic (no implicit role, no accessible name computation) — the label is inert for AT. Meanwhile ◇ is a decorative glyph AT will attempt to read ("white diamond suit") through the text alternative.
Fix: role="img" aria-label="Layer keyframe indicator" — or simpler, aria-hidden="true" and drop the aria-label.
| : []; | ||
| // 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). | ||
| const showTrackLabel = contentOrigin >= LABEL_COL_W; |
There was a problem hiding this comment.
🟢 Keyframe-layer branch renders LABEL_COL_W children inside a parent whose width can be smaller
const showTrackLabel = contentOrigin >= LABEL_COL_W;
const isKeyframeLayer = !!keyframeClip && lanes.length > 0;
return (
<div
className={`sticky left-0 z-[12] shrink-0 ${...}`}
style={{
width: showTrackLabel ? LABEL_COL_W : contentOrigin,
...
}}
>When both isKeyframeLayer=true and showTrackLabel=false (timeline zoomed/narrow enough that contentOrigin < LABEL_COL_W), parent width becomes contentOrigin, but children — LayerDisclosureRow and every PropertyGroupHeaderRow — are absolutely positioned with hardcoded width: LABEL_COL_W. No overflow-hidden on the parent, so rows spill past the gutter column into the timeline canvas.
Fix: In the keyframe branch, use width: LABEL_COL_W unconditionally, or pass an effective width down to LayerDisclosureRow / PropertyGroupHeaderRow.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 3b9d73aadcc4.
Track-header + LayerDisclosureRow. Rames caught three orange a11y items (missing aria-expanded, aria-pressed, focus-visible + hit-target on chevrons), one type-widening, and one hover-driven full-lane-recompute perf item. My additive read confirms two of the a11y items and adds one small note on decorative markup.
Prior review
Rames D Jusso R1: (1) LayerDisclosureRow's disclosure <button> missing aria-expanded, (2) property-group toggle diamond missing aria-pressed, (3) PropertyGroupNavigation's ‹/› chevrons missing focus-visible outline AND <24px hit target, (4) propertyGroup widening, (5) hoveredGroup triggers full-lane-recompute. Overlap: I confirmed findings 1 and 3 in the diff.
🟢 Confirmed
- Finding 1:
LayerDisclosureRowatLayerDisclosureRow.tsx:30-40is a disclosure widget (expands/collapses child content) but the<button>hasaria-labelonly — noaria-expanded={isExpanded}. Screen readers cannot announce the widget state to the user, and the WAI-ARIA disclosure pattern requiresaria-expandedon the trigger. Same pattern applies toTimelineTrackHeaderwhen it wraps the lane rows. - Finding 3: chevrons at
TimelineTrackHeader.tsx:342-360useclassName="h-5 w-3"= 20×12px. WCAG 2.5.8 target size is 24×24px; the chevron<button>is also missingfocus-visible:outlinewhile every other button in this file has it — inconsistent focus discoverability.
🟠 Additive nit — decorative ◇ span carries aria-label
packages/studio/src/player/components/LayerDisclosureRow.tsx:47-52
<span
aria-label="Layer keyframe indicator"
className="shrink-0 text-[13px] leading-none text-white/40"
>
◇
</span><span> is non-interactive; ARIA labeling on a non-interactive element without a role has no effect for most AT (it's silently dropped). If the ◇ is purely decorative (it is — the interactive control is the caret next to it), the correct annotation is aria-hidden="true", matching the <Music aria-hidden="true"> icon in LegacyTrackHeader. Rename or remove aria-label.
🟢 Verified clean
- Selection/expansion ownership:
isExpanded,keyframeClip,onToggleClipExpandedall come from props; onlyhoveredGroupis local. No local expansionuseStatethat would drift from the store — Rames's ownership check matches. getKeyframeNavigationStaterefactor is behavior-preserving for the pre-existingKeyframeNavigationcaller (property is always passed); new no-property branch is exercised only by the lane callers introduced here.- Group-scoped toggle target is derived correctly in
resolveLaneHeaderState—animationId,propertyGroup,tweenPercentage,properties,removeall populated from the resolved animation identity. - Test coverage covers the toggle target, per-group prev/next seek, diamond fill state, value readout formatting, and lane-row top alignment vs
TimelinePropertyLanes(Rames confirmed).
Cross-stack
The aria-expanded gap Rames flagged is the disclosure-widget contract that #2690 inherits when it wires this header into the full timeline. Fixing here is more surgical than at the integration tip.
Verdict
Approving with nits — Rames covers the substantive a11y gaps and the perf item; I confirm 1 & 3 and add the decorative-span nit. The gaps are follow-up-able, none are blockers on their own.
— Review by Via
c8708ec to
c4d4c12
Compare
3b9d73a to
f5a2098
Compare
c4d4c12 to
a9bdade
Compare
f5a2098 to
7f86018
Compare
The base branch was changed.
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 smaller