fix(timeline): let the keyboard activate a lane pill, not just the pointer - #547
Conversation
…inter
Every lane pill renders with `role="button"` and `tabIndex={0}`, so it is reachable
by Tab and announced as activatable. Nothing answered: the only selection path was
`onPointerDown`, and the file had no `onKeyDown` at all.
The consequence is not that a keystroke was missing — it is that focus led nowhere.
Delete, copy/paste and the inspector all act on `tl.selection`, so a keyboard user
could reach a zoom, speed, trim, annotation or full-camera pill and then do nothing
with any of them.
Enter and Space now select, and Shift+Enter adds to the selection, matching
shift-click. Every other key is left alone: the editor binds single letters (Z, T,
D…) on window, and swallowing them here would disable the shortcuts while a pill has
focus.
`e.nativeEvent.stopPropagation()`, not just the synthetic one. The shell's shortcut
handler listens on WINDOW, above React's root container, and Space is bound to
play/pause there — stopping only the synthetic event would select the pill and toggle
playback in the same keystroke. A test pins that, and pins that an ignored key still
reaches the window listener.
Selection is now one named callback (`selectPill`) that both doors call, rather than
`tl.selectRegion` inlined in the pointer path. That is what stops the two drifting
again, and it is the single line a new pill kind has to touch.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughLane-pill selection now uses a shared callback for pointer and keyboard input. Enter and Space activate pills, support Shift-based additive selection, and do not trigger window-level playback handling. Tests cover activation, ignored keys, and propagation. ChangesLane-pill selection
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Focused timeline pills can now be selected with Enter or Space without also triggering playback, while existing pointer selection remains unchanged; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KeyboardUser
participant LanePill
participant V4Timeline
participant WindowPlaybackHandler
KeyboardUser->>LanePill: Press Enter or Space
LanePill->>V4Timeline: preventDefault and stopPropagation
V4Timeline->>V4Timeline: Call selectPill with optional Shift selection
V4Timeline--xWindowPlaybackHandler: Do not trigger playback
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes the required summary, issue context, change type, release impact, platform impact, testing details, and screenshot rationale. It clearly explains the keyboard behavior and validation results.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Summary
Every timeline lane pill renders with
role="button"andtabIndex={0}— reachable by Tab, announced as activatable — and nothing answered. The only selection path wasonPointerDown;V4Timeline.tsxhad noonKeyDownat all (grep -c onKeyDownonmainreturns 0).The consequence is not a missing keystroke, it is that focus led nowhere. Delete, copy/paste and the inspector all act on
tl.selection, so a keyboard user could tab to a zoom, speed, trim, annotation or full-camera pill and then do nothing with any of them.window, and swallowing them here would disable the shortcuts while a pill has focus.e.nativeEvent.stopPropagation(), not just the synthetic one: the shell's handler listens on window, above React's root container, and Space is bound to play/pause there. Stopping only the synthetic event would select the pill and toggle playback in one keystroke.Selection is now one named callback (
selectPill) that both doors call, instead oftl.selectRegioninlined in the pointer path. That is what stops the two drifting apart again, and it is the single line a new pill kind has to touch.Related issue
Raised by CodeRabbit on #543 and deliberately left out of that PR: the bug predates it, and the fix changes keyboard behaviour for every lane kind rather than the two #543 adds.
Interaction with #543: both touch
renderOnePill, so whichever lands second rebases over the other. If this lands first, #543's rebase needsselectPillto map its two audio lanes onto the document kind"audio"— one line, in the one place selection now lives.Type of change
Release impact
Desktop impact
Screenshots / video
None — no visual change. The pill already had focus styling; what changes is that pressing Enter or Space on a focused one now does something.
Testing
npm run test— 2220 passed, 5 skipped, 0 failed (186 files)npx tsc --noEmitandnpx tsc -p tsconfig.test.json --noEmit— cleannpm run lint(Biome) — clean (15 pre-existing warnings in untouched files)Five tests in
V4Timeline.geometry.test.tsx: Enter selects, Space selects, Shift makes it additive, an ignored key does not select, and the window listener sees the ignored key but not Enter or Space.Verified as a real regression test: removing the handler fails four of the five. The fifth (other keys are left alone) passes either way by construction — it guards against over-capture rather than proving the feature.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests