fix(keyboard): stop the NiiVue tab from capturing keys and double-applying c/v (#223, #224)#242
Merged
Merged
Conversation
7f10e75 to
b45bd2b
Compare
Contributor
🚀 PWA Preview DeploymentYour PWA preview has been deployed! Preview URL: https://niivue.github.io/niivue-vscode/pr-242/ This preview will be updated automatically when you push new commits to this PR. |
…223, #224) VS Code shortcut keybindings (1-5, r, i, b, x, m, z, s, u, ...) were gated only by `activeCustomEditorId == niiVue.default`, which stays true even when focus moves to Quick Open or the command palette. Because the bound commands are no-ops (the webview's own keydown listener does the real work), the bindings merely swallowed those keystrokes, so they could not be typed into the "top box". Add `&& !inputFocus` to every binding so they only fire when the viewer holds focus (#223). NiiVue's built-in `c` (clip plane) and `v` (view mode) hotkeys ran on the focused canvas on top of the app's useKeyboardShortcuts handler, which already broadcasts to every selected canvas. The focused canvas was therefore acted on twice (once on keydown by the app, once on keyup by NiiVue). Disable NiiVue's clipPlaneHotKey/viewModeHotKey so the app is the single source of truth (#224). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b45bd2b to
5400bb6
Compare
Contributor
Coverage ReportOverall line coverage: 38.5% (−2.7) vs
|
Contributor
🧹 PWA Preview CleanupThe preview deployment for this PR has been removed. |
korbinian90
added a commit
that referenced
this pull request
Jun 17, 2026
Resolve the conflict in apps/pwa/tests/keyboard-shortcuts.spec.ts. Keep the e2e->unit migration (the single wiring smoke test) and preserve #242's #224 clip-plane regression as an integration test: that NiiVue's built-in clipPlaneHotKey is disabled so one 'c' press is one step is a claim the jsdom hook unit test cannot make (it has no real NiiVue canvas). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #223 and #224. Both are keyboard-handling conflicts between the VS Code/host layer and NiiVue's own canvas hotkeys.
#223 — NiiVue tab captures keys meant for the "top box"
The extension contributes keybindings for
1-5,r,i,b,x,m,z,s,u,shift+u,shift+d, etc., gated only bywhen: activeCustomEditorId == niiVue.default. Those commands are intentional no-ops (extension.ts) — the webview's ownkeydownlistener does the real work; the VS Code commands exist only for command-palette discovery and keybinding customization.The problem:
activeCustomEditorIdstaysniiVue.defaulteven when keyboard focus moves to Quick Open or the command palette. So when you open the "top box" over a NiiVue tab and type, the single-key bindings fire their no-op commands and swallow the keystrokes instead of letting them reach the input.Fix: add
&& !inputFocusto every contributed keybinding.inputFocusis true when a VS Code input widget (Quick Open, command palette, etc.) is focused and false when the webview itself is focused, so the shortcuts now only act when the viewer holds focus.#224 —
c/vapplied twice on the focused canvasThe app's
useKeyboardShortcutshook handlesc/von keydown and broadcasts the action to every selected canvas. NiiVue's own canvas handler also handles them on keyup (clipPlaneHotKey/viewModeHotKey), but only for the focused/in-bounds canvas. Result: the focused canvas is acted on twice per press — e.g. oneclands on clip-plane index 2 instead of 1.Fix: construct each
ExtendedNiivuewithclipPlaneHotKey: ''andviewModeHotKey: ''. NiiVue matches hotkeys with a strictKeyboardEvent.code === hotKey, so an empty string never matches and its built-inc/vhandlers are disabled. The app's broadcast handler becomes the single source of truth, so every selected canvas advances exactly once.Tests
apps/vscode/test/keybindings.test.ts(new): asserts every contributed keybinding carries bothactiveCustomEditorId == niiVue.defaultand!inputFocus, with an extra guard over the bare printable keys.apps/pwa/tests/keyboard-shortcuts.spec.ts: new e2e case asserting a singlecpress lands on clip-plane index1.Note on arrow keys (investigated, intentionally not changed)
An earlier revision also tried to make the app the single source of truth for the ←/→ (4D frame) keys, on the assumption they double-applied like
c/v. CI disproved that: the app'svolumeNext/volumePrevpass a volume object tonv.setFrame4D(id, frame), which expects a volume id, so they are silent no-ops — NiiVue's own handler is the only thing that advances the frame on arrow press. Suppressing it broke arrow navigation (and the pre-existing4d-synckeyboard test). That change has been reverted; NiiVue keeps owning the arrow keys, and there is no double-apply to fix. (ThevolumeNext/volumePrevno-op is a real but separate latent bug, worth a follow-up.)Reviewer notes
events.tschange lives in the shared@niivue/reactpackage, so the [bug] clip plane shortcut c is applied twice #224 fix reaches the apps that mount the sharedMenu/useKeyboardShortcutshandler: vscode, PWA, Jupyter, Tauri, and Streamlit's styled viewer.UnstyledCanvas→Containeronly, noMenu) has no app keyboard handler, so disabling NiiVue's built-inc/vremoves them there with no replacement. That mode has no shortcut UI, so this is considered acceptable; called out in the changeset.!inputFocuschange is VS Code-only (apps/vscode/package.json).Known residual / follow-ups (out of scope here)
volumeNext/volumePrevkeyboard handlers are no-ops (wrong arg tosetFrame4D); NiiVue currently covers arrow navigation, so it is not user-visible, but the app-level broadcast for arrows never actually runs.mtriggers two different actions when a canvas is focused (app: toggle crosshair; NiiVue: cycle drag mode) — same conflict root, but not a double-apply.showHeaderkeybinding mismatch (ctrl+hvs maccmd+shift+h, while the webview handler expectsctrl+shift+h).🤖 Generated with Claude Code