fix(studio): announce real track numbers and wire lane aria-controls - #2848
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.
23bed54 to
d6aa760
Compare
ff16b42 to
295f98b
Compare
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.
Two halves of one inversion in the expanded timeline lanes: the tweens
that should show were filtered out, and a tween that should not be there
was the only survivor.
Lane classification read the parser's whole-tween verdict, which is
undefined for anything spanning more than one property group. `{x,
opacity}` is the canonical HyperFrames entrance tween, so five of the
seven tweens in the swiss-grid graphics example had no caret, no
reserved row and no diamonds. Classify per property instead, through one
helper both the rendered lanes and the reserved row heights count
through so they cannot drift again.
Attribution matched an unanchored leading id, so `#stat3 .block` was
filed 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-percentage merge then resolved by dropping the ease. Route
attribution through resolveSelectorElementIds, which anchors a
whole-selector id and otherwise resolves through the live preview DOM,
and anchor its no-DOM fallback so a descendant selector resolves to
nothing rather than to its ancestor. The merge rule is unchanged.
Also brings the last property-lane call site onto the shared clip timing
basis: an expanded sub-composition child's start is host-absolute while
its tweens are local to its own file.
295f98b to
37140af
Compare
d6aa760 to
9cec37c
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R1 adversarial review — accessibility lens set (12 a11y + 12 editor-UI)
Head: 9cec37c15834000377fbeb08a5e56fede95abf21 (base fix/studio-playhead-keyframe-target, downstack pending)
Verdict: APPROVE — no P0 / P1. This is a clean fix with the right seams.
The PR does two independent things and both hold up under grep:
- Announcement layer:
TimelineElement.trackis a fractional z-order sort key (host.track + n / (siblings + 2)), so labels built from it read out "Hide track 0.16666666666666666". The fix threads a separatetrackDisplayNumber(1-based row) through the announcement path —TimelineTrackHeader'saria-label+title, theTrack Nfallback, andtoggleTimelineTrackHidden's undo-history label — while every callback and lookup still routes by the rawtrackkey. Ownership of key → display row is single-sourced in the newtimelineTrackDisplay.ts, and both call sites (renderer + undo history) go through it, so the header and the history entry cannot drift apart. aria-controlstarget: the caret'saria-controlsused to name a<div>in the sticky label column whose children were absolute-positioned, so it computed to 0x0 and held no diamonds.TimelinePropertyLanesnow wraps its lanes in one static<div id={id}>; static (notposition: relative) so it is not a containing block and every absolutely-positioned lane still resolves against the track-content div — verified explicitly byleaves every lane's inline offsets untouched by the wrapper.TimelineLanesmints the id (only place that sees both ends of the disclosure), keys it by the integer row index rather than by the fractionaltrackNum(so ids can never be...-0.16666666666666666, which would also break CSS#idselectors), and mounts the wrapper for the track's keyframe clip in both disclosure states soaria-controlsresolves while collapsed.
Lens sweep
aria-controlsid validity — grep forTimelinePropertyLanesJSX invocations: only one production caller (TimelineLanes.tsx), always passingid={lanesId}. Only one wrapper minted per track-row (guarded byisTrackKeyframeClip = STUDIO_KEYFRAMES_ENABLED && elementKey === keyframeClipKey, at most one such element per track). No hits for#timeline-lanes-track-anywhere else in the repo — no orphaned CSS/JS depending on the old ergonomics. WhenSTUDIO_KEYFRAMES_ENABLEDis off,keyframeClipbecomesnulland both the caret (!keyframeClip || lanes.length === 0→PlainTrackHeader, noaria-controls) and the wrapper are dropped in lockstep. No dangling reference in that branch.- Track number stability — display row shifts when tracks are added/removed above; this is inherent to any 1-based announcement scheme and dramatically better than the fractional key it replaces. Not a defect.
- Route-vs-announce split at both surfaces —
TimelineTrackHeader.test.tsxannounces the display track number but toggles with the real fractional keyandTimelineLanes.test.tsxhands the visibility toggle the real track key, not the display index+hands the lane context menu the real track key, not the display indexpin both callback contracts. The undo history file addslabels the undo entry with the display row, not the fractional track keyontrack: 1 / 6explicitly — a test ontrack: 0would format cleanly and prove nothing (the PR body flags this too). role/aria-label— the wrapper is a role-less<div>(fine for anaria-controlstarget); the per-group lane rows keeprole="group" aria-label="{group} keyframes".LayerDisclosureRow's caret keepsaria-expanded+aria-label="Expand/Collapse {name} keyframes". No new interactive element is un-named.aria-live/ focus / tabindex / reduced-motion / contrast — nothing new added and nothing existing touched.- Consumer/producer split (aria-controls has one) — verified. The caret and the wrapper are minted in the same map iteration in
TimelineLanes(id derived from the samerow), so they cannot fall out of sync within a single render. - Multi-drag remount —
does not remount the lanes while a multi-clip drag slides the formationpins node identity (.toBe(before), not just.not.toBeNull()) across a preview-input update, which was the failure mode the propertyLanes-key comment describes. - Byte-identical geometry — asserted lane-by-lane on
style.top/.left/.width/.heightin the newTimelinePropertyLanestest, plus a specific guard thatwrapper.style.position === ""(a slip torelativewould silently reflow every lane). - Blast radius on the new wrapper —
TimelinePropertyLaneshas one production caller (TimelineLanes.tsx); a fragment→<div>change would only have broken direct-child selectors, and there are none in either the diff or the rest of the studio tree. - Editor-UI 12 lens set — key/callback split, virtualization (unchanged; already
fallow-ignorenoted for the render loop), remount-safety, memoization (useMemodeps unchanged), keyboard reachability of the eye (still on the always-mounted layer row), pointer-cancel semantics (stopPropagationpreserved), RTL neutrality, nouseEffectsynchronization introduced, prop-drilling minimal (two new props:trackDisplayNumberandlanesId, both documented at declaration).
Findings
Nit 1 — real-AT verification unclaimed in body. packages/studio/src/player/components/TimelineLanes.tsx:140 / undo history label at timelineTrackVisibility.ts:214. Unit tests assert the DOM aria-label string, but the PR body's "Manual testing performed" checkbox does not commit to an NVDA/VoiceOver pass. The DOM assertion is a strong proxy — aria-label is what the AT reads — but shipping this without a live SR spot-check is an evidence gap given the bug is by definition about what an AT user hears. Non-blocking; a one-line "Verified with that the row announces Hide track 2 after fractional-key toggle" in the PR body would close the loop.
Nit 2 — aria-controls could be space-separated to name both subtrees. packages/studio/src/player/components/LayerDisclosureRow.tsx:48 — the disclosure expands two disjoint subtrees (the label-column property rows and the canvas diamond lanes), and the code comment at TimelineTrackHeader.tsx:362 already acknowledges that. aria-controls accepts a space-separated list of ids; a version that names both would let AT surface both when following the reference. The current choice (canvas lanes only, because that's what a sighted user watches appear) is defensible ergonomics, not a defect. Non-blocking; the label-column controls are still keyboard-reachable via normal traversal after the caret is toggled.
Nit 3 — Track ${displayNumber} fallback string is not localized. packages/studio/src/player/components/TimelineLanes.tsx:157, TimelineTrackHeader.tsx:65. Studio does not appear to i18n elsewhere in this file — this matches the surrounding surface, so this is a "note for later" only. If Studio ever adopts i18n, Hide track {n} / Show track {n} / Track {n} are all announcement-critical strings that will need wrapping.
Verdict
Grade: A — surgical, correctly seamed, thorough tests. The split of "fractional key routes callbacks; 1-based row routes announcements" is captured in the type contract (trackNumber vs trackDisplayNumber are distinct required props with JSDoc), not just in convention, so future regressions have a real chance of being caught at the boundary. The three nits above are all non-blocking.
— Review by 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 9cec37c.
Really clean a11y fix — three bugs found, three landed together, all with tests that exercise the actual failure mode. The timelineTrackDisplay.ts single-owner pattern is the right shape: trackDisplayNumber(timelineTrackOrder(elements), trackNum) now owns every user-facing row number (header labels, Track N fallback, undo history entries), so the fractional z-order key can't leak through any of them again. The TimelineTrackHeader.tsx prop-shape split (trackNumber for callbacks, trackDisplayNumber for labels) makes the invariant impossible to violate at the type level — good downstream defense.
The aria-controls retarget is the tricky one and worth calling out for anyone reviewing follow-ups:
- Wrapper subtree change is correct. Old: the id lived on a static div in the sticky label column, whose children were absolute-positioned inside a
.relative— so the box computed to 0x0 and the diamonds a sighted user watched appear were actually in a different subtree (canvas side). Screen readers following the reference landed on an empty box. New: the id lives onTimelinePropertyLanes's<div id={id}>on the canvas subtree, with the diamond lanes as DOM children (still absolute-positioned so the box still computes to 0x0 in-flow, but the accessibility tree walks DOM children regardless of visual extent — which is what NVDA / VoiceOver / JAWS all do). - Always-mounted in both disclosure states (
TimelineLanes.tsx:452-462—isTrackKeyframeClipgates mounting, notshowsLanes) soaria-controlsresolves while collapsed too, withanimations={[]}feeding an empty inner tree. Confirmed by the collapsed-state test atTimelineLanes.test.tsx:209. - Multi-drag key stability (
key={${clipKey}-property-lanes}atTimelineLanes.tsx:460) prevents the passenger-transform-wrapper from remounting mid-drag. Directly asserted by the node-identity test atTimelineLanes.test.tsx:221.
Concerns
Latent id-uniqueness assumption. lanesId = timeline-lanes-track-${row} at TimelineLanes.tsx:140 uses the 0-based render row as the id suffix. Unique within one TimelineLanes instance. But if Studio ever renders two TimelineLanes on the same page (e.g., a preview timeline + main timeline, or a mini-timeline in a modal), every track's aria-controls id collides across instances — #timeline-lanes-track-0 matches whichever instance mounts first. Non-blocking now, but useId() for a stable prefix on the enclosing component would future-proof it against a second consumer.
Screen-reader verification is manual, unspecified. The PR body says "Manual testing performed" but doesn't say which screen reader(s). Given the wrapper's 0x0-in-flow shape, a quick VoiceOver + NVDA pass on the caret's expand announcement + subsequent lane navigation would confirm the accessibility tree actually surfaces the diamonds as expected — the DOM contract is right, but the AT rendering surface is the ground truth.
Nit
timelineTrackDisplay.ts:22-23 — const row = trackOrder.indexOf(track); return row < 0 ? trackOrder.length + 1 : row + 1;. The < 0 branch always announces "the row it would land on at the end" (length + 1), even when a drag-preview is snapping between existing rows (say between row 2 and row 3). Probably fine for the current callers (visibility labels + undo history don't fire during in-flight drag), but if a future call site announces during drag-preview, the label would drift from the visible insertion caret.
Nothing blocking, CI green (perf, regression, preview all pass on the head SHA). LGTM from my side.
animIdForProp matched on the parser's whole-tween propertyGroup, which is
undefined for a legacy mixed tween such as {x, opacity}. Such a tween never
matched, so an edit to either property fell through to the selection's
default animation, a different tween than the lane the user is editing.
Resolve through animationLaneGroups, the same per-keyframe helper the
rendered lanes and the reserved row heights already count groups with.
"Add keyframe at playhead" on an element with no id authored the bare class
buildStableSelector hands back, so one add on a `.group` wrote
`tl.to(".group", ...)`: a tween that animates all five siblings and that
resolveSelectorElementIds reads back as all five, collapsing their timeline
rows into one. It survived a reload, so the written file stayed un-editable.
writeTargetSelector is the write-side counterpart to selectorFromSelection
(which must keep returning the exact string findTweenAtTime compares against).
It resolves the element's own identity to a selector that addresses exactly
one element: `#id`, else `[data-hf-id="..."]`, else the selection's selector
when it is already unique, else a `:nth-child` path anchored on the nearest
identifiable ancestor (the selector + selectorIndex pair, resolved through the
DOM the index was counted in).
Applied to the two paths that author a NEW tween: the no-animation branch of
useEnableKeyframes and commitKeyframeAtTimeImpl. replace-with-keyframes still
writes the selection's own selector, since retargeting a tween the author
aimed at a whole group is a different decision from adding a keyframe.
writeTargetSelector returned the selection's bare selector whenever the structural walk failed, including when a live DOM was there to check against. An element detached between selecting and committing takes that path, so the add re-authored the exact `.group` string the function exists to replace. Return null instead: a failed walk against a live DOM is evidence, not absence of it. Callers that cannot drop a user edit opt back in with `?? selectorFromSelection` where the trade is visible. The replace-with-keyframes paths had the mirror defect. The server deletes and re-adds the tween, so their target string is a full rewrite, and they derived it from the selection: promoting a set on a tween already narrowed to `#scene > div:nth-child(3)` widened it back onto every class sibling, undoing the narrowing an earlier add had made. They now keep the tween's own authored target, matching what eight sibling commit modules already do.
…he lanes The timeline's track key is a fractional z-order sort value, and the header built its visibility label straight from it, so screen readers announced "Hide track 0.16666666666666666". A track's 1-based display row is now passed alongside the key: the row number goes in every label, the key keeps routing every callback (visibility toggle, lane context menu). The same fix covers the `Track N` fallback used when a track holds no labelled element. The layer disclosure caret's aria-controls named a div in the sticky label column. That subtree is not empty, it holds the per-lane keyframe controls, but its children are all absolutely positioned so the div computes to 0x0, and the diamonds the caret visibly reveals live on the canvas instead. The caret expands two disjoint subtrees and was naming the less useful one. TimelinePropertyLanes now renders one static wrapper (static, not relative, so it establishes no containing block and the absolutely-positioned lanes keep resolving against the track-content div with identical geometry) and takes the id. TimelineLanes mints that id, since it is the only place that sees both ends of the disclosure, and mounts the wrapper for the track's keyframe clip in both disclosure states so the reference still resolves while collapsed. TimelineLaneBaseProps moves to its own module: it is the contract shared by TimelineCanvas and TimelineLanes, and lifting it out keeps TimelineLanes.tsx well under the 600-line cap instead of pushing past it.
The timeline track key is a fractional z-order sort key: an expanded sub-composition child gets `host.track + n / (siblings + 2)`. The undo history entry for the eye toggle interpolated that key directly, so hiding an expanded child recorded "Hide track 0.16666666666666666". Give the key-to-display-row conversion a single owner (timelineTrackDisplay.ts) and route both the track header labels and the history label through it, so the two cannot drift apart again. The raw key still routes the callbacks and lookups that need it. Adds a regression test that toggles a track keyed 1 / 6; a test on track 0 formats cleanly and proves nothing.
37140af to
f04cdb7
Compare
9cec37c to
477916c
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 re-review — approval carries
Head: 477916cbe25dd6dd1877c09df3d826cd60b609e5 (was 9cec37c15834000377fbeb08a5e56fede95abf21 at R1).
Delta since R1. The R1 head's single commit was split during rebase into two: 1faa0cbd (fractional-key labels + aria-controls retarget + timelineLaneProps.ts split) and 477916cb (undo-history label routed through the shared display-row owner). The base advanced by 3 commits from the downstack. Combined PR diff against the current base is the same 13-file shape I stamped at R1: same file set, same +/- envelope (+618 / -129), same two new modules, same new tests. No behavior change from the reorganization; the second commit merely lifts the row + 1 derivation out of TimelineLanes.tsx's inline JSX into trackDisplayNumber(displayTrackOrder, trackNum), which is the same function toggleTimelineTrackHidden now calls — so the header label and the history label are guaranteed to render the same row number for the same track key.
Invariant recheck at new head.
- Byte-identical geometry —
TimelinePropertyLanes.tsxstill wraps in one static<div id={id}>, andleaves every lane's inline offsets untouched by the wrapperstill pinsstyle.top/left/width/heightlane-by-lane plusexpect(wrapper.style.position).toBe(""). HOLDS. - Type-contract split —
TimelineLaneBasePropsstill lives intimelineLaneProps.tsand is imported astypeby bothTimelineCanvas.tsxandTimelineLanes.tsx.TimelineTrackHeaderstill requirestrackNumber(fractional key) andtrackDisplayNumber(1-based row) as separate props with JSDoc. HOLDS. aria-controlsalways resolves —TimelinePropertyLanesstill returns the identified wrapper even whenanimationsis empty (still renders the identified wrapper when there are no lanes to show), andTimelineLanes.tsxstill mounts it for the track's keyframe clip in both disclosure states (isTrackKeyframeClip, notshowsLanes, gates the mount). HOLDS.
R1 nits — outcome at new head.
- Nit 1 (no NVDA/VoiceOver claim in body): UNCHANGED. PR body still checks "Manual testing performed" without naming a screen reader. Rames flagged the same at R1; non-blocking, would still be closed by a one-line "Verified on " in the body.
- Nit 2 (
aria-controlscould space-separate both subtrees): UNCHANGED. Caret still names only the canvas lanes wrapper. The new inline comment inTimelineTrackHeader.tsx:353explicitly documents why (a sighted user watches the canvas diamonds appear, not the label-column controls), so the choice is now narrated at the call site. - Nit 3 (
Track ${n}fallback not i18n-wrapped): UNCHANGED. StillTrack ${displayNumber}atTimelineLanes.tsx:157, consistent with the rest of the studio surface.
Fresh adversarial pass — new code in 477916cb.
timelineTrackDisplay.ts—timelineTrackOrderdedupes viaSetthen sorts ascending; track keys are deterministic (integer orhost.track + n / (siblings + 2)), so same-value floats compare equal.trackDisplayNumberfalls back totrackOrder.length + 1forindexOf === -1; documented as "the row it would land on at the end". Rames' R1 nit about this branch always announcing the end row (even for drag-preview insertions between existing rows) still applies but has no current caller during drag — both call sites (visibility labels + undo history) fire outside in-flight drag. Non-blocking.timelineTrackVisibility.ts— order derived fromtimelineElements(all elements), then filter toelement.track === track. If a caller ever toggles a track with no elements,displayNumberreturns fallbacklength + 1, which reads as "one past the last visible row" — degenerate but not wrong. Non-blocking; hard to see a caller reaching this path.- New tests key at
1 / 6, not0— the PR body explicitly calls out "a test on track0formats cleanly and proves nothing", which is the discipline these regression tests need. BothHideandShowpaths asserted. - Header/history parity — both label sites now go through
trackDisplayNumberwith the same input shape (displayTrackOrderupstream,timelineTrackOrder(timelineElements)in the undo path). One theoretical drift: if a caller oftoggleTimelineTrackHiddenpasses atimelineElementsarray that differs from whatTimelineCanvasderiveddisplayTrackOrderfrom at the same moment, the numbers could disagree. Every producer I could find hands the same source-of-truth store selector to both — non-blocking. - Latent id-uniqueness (Rames' R1 concern): still
timeline-lanes-track-${row}, still unique within oneTimelineLanesinstance. No new consumers introduced. Non-blocking.
CI: 8 pass, 4 skipping, 1 pending (Graphite mergeability — expected for a stacked PR).
Verdict
Grade: A — approval carries. Delta is a commit-shape reorganization plus a small readability tidy (lifting row + 1 into the shared helper), no material change. Three R1 nits unchanged, all still non-blocking.
— Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 reviewed at 477916c (delta from 9cec37c).
The R2 delta on this PR is a hoisted helper rather than a response to R1 — resolveAnimIdForProperty in TimelinePropertyLanes.tsx, exported so the property panel can route an edit to the tween that actually animates the property's group instead of the selection's default animation. Reads clean; the docblock names the exact bug it fixes (mixed tweens like {x, opacity} had undefined propertyGroup and got dropped from the match).
Test coverage for the new helper is thorough — mixed tween routes both groups to itself, non-matching groups fall through, single-group tween wins over mixed for its own property. Nice.
One small note on the helper: animations?.find((a) => animationLaneGroups(a).includes(group)) returns first-parsed match. If a source ever has TWO tweens that both animate the property's group (e.g. two opacity tweens), the router picks whichever the parser saw first, which isn't obvious from a caller. Not surfacing as a concern since it matches the previous behavior and the docblock is scoped to the mixed-tween bug — just a seam to be aware of.
R1 concerns from pullrequestreview-4800579862 are unchanged in this delta:
lanesId = \timeline-lanes-track-${row}`(TimelineLanes.tsx:140) — row-based key would collide if twoTimelineLanesinstances ever co-mount. Not exercised today, butuseId()`-prefixed would future-proof cheaply.- Screen-reader manual-test claim doesn't specify which SRs — VoiceOver + NVDA pass would confirm the 0x0-in-flow wrapper actually surfaces diamonds into the AT tree.
trackDisplayNumberalways-length+1fallback for an unfound track (timelineTrackDisplay.ts:11) — likely-unreachable, but a sentinel or invariant assert would beat silently returning a plausible-looking row.
None are blockers; leaving here rather than re-stacking inline comments.
— Review by Rames D Jusso
Two latent defects in the announcement path this branch adds. - lanesId was keyed by render row alone, so a second TimelineLanes on the page (a mini-timeline beside the main one) would mint the same timeline-lanes-track-0 and every caret's aria-controls would resolve to whichever instance mounted first. The prefix now comes from useId, with the colons stripped so the id stays a legal CSS selector. - trackDisplayNumber returned trackOrder.length + 1 for a key it could not find, which is indistinguishable from a real row: the label announced a row the user can see is wrong and nothing upstream could tell it had guessed. It returns null now, and trackDisplaySuffix drops the number from the label rather than inventing one.
|
Both R1 concerns that were code-actionable are fixed here at
Screen-reader claim (Via nit 1, Rames R1). Not fixable by code, so the body now states it plainly instead of leaving "Manual testing performed" to imply more than was done: manual testing here means driving the timeline in the browser and reading the DOM, plus unit assertions on the exact Via nit 2 (space-separated Rames' R2 note on PR body updated with both new bugs. Studio suite: 3087 passing, 0 failures. Typecheck, lint, format clean. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 reviewed at ba0d640 (delta from 477916c — silent restack alongside the #2849 R2 restack).
Clean R3. Two of three R2 concerns closed silently:
-
lanesIduniqueness — CLOSED.TimelineLanes.tsx:93now mintslanesIdPrefixfromuseId()(with colons stripped so the id stays legal inside a CSS#idselector). TwoTimelineLanesinstances co-mounted (mini-timeline in a modal + main timeline) no longer collide on...-track-0. Docblock spells out the exact scenario — nice. -
trackDisplayNumberalways-length+1fallback — CLOSED.timelineTrackDisplay.ts:26-32now returnsnumber | null, with the reasoning that "the only numbers available to return are indistinguishable from a real answer: a label would announce a row the user can see is wrong, and nothing upstream would ever learn it had guessed." Plus a companiontrackDisplaySuffixhelper —null → "", non-null →" 3"— that propagates the null case through every label (Track${suffix},Hide track${suffix}, etc.) so no site invents a number.TimelineTrackHeader.tsx:59andtimelineTrackVisibility.ts:218both flow through it. Same shape I was hoping for.
Still open (not restated inline):
- The screen-reader manual-test claim in the PR body still doesn't specify which SRs were tested — a VoiceOver + NVDA pass would confirm the 0x0-in-flow
TimelinePropertyLaneswrapper surfaces the diamond children into the AT tree as intended.
Nothing net-new in this delta introduces problems. Ready from where I sit; leaving as COMMENTED (stamp routing per usual).
— Review by Rames D Jusso
The base branch was changed.
…umbers-aria fix(studio): announce real track numbers and wire lane aria-controls
3 participants
What
Timeline track accessibility: screen readers announced a raw floating point sort key, and the disclosure caret pointed at the wrong subtree.
Bugs fixed
host.track + n / (siblings + 2)), and the track header built its visibility label straight from it. TheTrack Nfallback used when a track holds no labelled element had the same defect.aria-controlsnames a 0x0 element. It pointed at a div in the sticky label column. That subtree is not empty, it holds the per-lane keyframe controls, but its children are all absolutely positioned so the div computes to 0x0. The diamonds the caret visibly reveals live on the canvas instead, so the caret expands two disjoint subtrees and was naming the less useful one.timeline-lanes-track-${row}from the render row alone. TwoTimelineLanesinstances (a mini-timeline in a modal beside the main one) would both minttimeline-lanes-track-0, and every caret'saria-controlswould resolve to whichever mounted first. Not reachable with today's single consumer, fixed before it gains a second.trackDisplayNumberreturnedtrackOrder.length + 1for a key it could not find. That is indistinguishable from a real answer, so the label announced a row the user can see is wrong and nothing upstream could tell it had guessed.Why
A screen reader user cannot act on "track 0.16666666666666666". There is no such track from their point of view, and no way to map it back to what they are looking at.
How
Pass a track's 1-based display row alongside its key. The row number goes in every label, the key keeps routing every callback (visibility toggle, lane context menu) because those need the real sort value.
Give the key-to-display-row conversion a single owner (
timelineTrackDisplay.ts) and route both the header labels and the undo history label through it, so the two cannot drift apart again.For the caret,
TimelinePropertyLanesrenders one static wrapper and takes the id. Static, not relative, so it establishes no containing block and the absolutely-positioned lanes keep resolving against the track-content div with identical geometry.TimelineLanesmints the id, since it is the only place that sees both ends of the disclosure, and mounts the wrapper in both disclosure states so the reference still resolves while collapsed.The id prefix comes from
useId, so it is per instance rather than per render row. React embeds colons in that value, which are legal in an id and inaria-controlsbut need escaping in a CSS#idselector, so they are stripped and the prefix stays plain.trackDisplayNumberreturnsnumber | null, andtrackDisplaySuffixdrops the number from the label when there is no row:Hide trackrather than an inventedHide track 4. Both callers build the order from the same elements the key came from, so null is unreachable by construction today.TimelineLaneBasePropsmoves to its own module. It is the contract shared byTimelineCanvasandTimelineLanes, and lifting it out keepsTimelineLanes.tsxunder the 600-line cap.Test plan
Studio suite green on this branch: 3087 passing, 0 failures. Typecheck, lint and format clean.
The undo history regression test toggles a track keyed
1 / 6on purpose. A test on track0formats cleanly and proves nothing.Manual testing here means driving the timeline in the browser and reading the resulting DOM, plus unit assertions on the exact
aria-labelandaria-controlsstrings and on the resolved target's contents. No live NVDA / VoiceOver / JAWS pass was run, so the accessibility tree is verified through the DOM contract an AT reads, not through an AT.Not covered
Track Nfallback, the undo history label, and the disclosure caret. It is not a full timeline screen reader audit.aria-controlsstill names only the canvas lanes wrapper, not both subtrees the caret expands. The attribute accepts a space-separated list, but naming the label column too surfaces a subtree a sighted user is not watching appear; the call site documents the choice. The label-column controls stay keyboard-reachable either way.Track N/Hide track Nstrings are not i18n-wrapped, matching the rest of the studio surface, which has no i18n layer to wrap them with.