fix(timeline): let the keyboard activate a lane pill, not just the pointer - #547
fix(timeline): let the keyboard activate a lane pill, not just the pointer#547EtienneLescot wants to merge 1 commit into
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 all required sections, explains the keyboard behavior and implementation, identifies the related issue, documents release and platform impact, and provides detailed testing results. The related issue reference does not use the template's preferred "Refs
✨ 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