fix(ui): make I hotkey work in Canvas, Gallery, and Viewer - #9569
fix(ui): make I hotkey work in Canvas, Gallery, and Viewer#9569DustyShoe wants to merge 4 commits into
Conversation
lstein
left a comment
There was a problem hiding this comment.
Adversarial review of 9eb484c. I verified the premise in react-hotkeys-hook@4.5.0 before judging the change, and it holds:
useHotkeys' layout effect returns early whenoptions.enabled === false, so no listener is attached (dist/react-hotkeys-hook.esm.js:389).- When
enabledis a function, the listener is attached and, on a key match, callsstopPropagation(e)→stopImmediatePropagation()+preventDefault()before consulting the predicate (:424-427). Every registered hotkey binds todocument, so a disabled-by-predicate handler suppresses every same-key handler registered after it.
So the generic fix in getRegisteredHotkeyOptions is the right shape. Checks on my side: the 3 new tests pass with no type errors, eslint and prettier are clean on the changed files. The branch is BEHIND main but mergeable.
One blocker below, plus some non-blocking notes.
Blocker — I becomes a dead key on Canvas when the gallery is focused with nothing selected
ToolColorPickerButton.tsx now gates the eyedropper on !isGalleryFocused && !isViewerFocused, on the assumption that the metadata handler always covers the complement. It doesn't:
ToggleMetadataViewerButtonis only rendered whengalleryItem?.kind === 'image' | 'video'(ImageViewerToolbar.tsx:28) — i.e. only when an item is selected and its DTO has resolved.gallery.selectionstarts as[]and is inpersistDenylist(gallerySlice.ts:22,:233), so every page reload starts with no selection. It is also emptied byimageSelected(null)(:52) andshowVirtualBoardsChanged(:150).- The panel container is
tabIndex={-1}(AutoLayoutPanelContainer.tsx:45), so clicking gallery whitespace, the scrollbar, or an empty board focuses thegalleryregion without selecting anything.
Repro: reload → Canvas tab → click empty space in the gallery panel → press I. Nothing happens.
On main the eyedropper hotkey passed no options at all, so it was unconditionally enabled, and with no selection there was no metadata listener to mask it — I selected the eyedropper. So this is a regression against main for the very key the PR is fixing. It also hits transiently while useGalleryItemDTO is still resolving right after a selection.
The registration site is the root of it: toggleMetadata lives inside the viewer toolbar, whose render conditions are strictly narrower than the focus predicate now gating the eyedropper. GlobalImageHotkeys.tsx:39 already establishes isFocusOK = isGalleryFocused || isViewerFocused for exactly this family of hotkeys and registers them unconditionally at app level — that's where this one belongs. Note it can't be moved verbatim: GlobalImageHotkeys filters out videos (:23) and the metadata toggle supports them.
Non-blocking
1. isDisabledOverride leaves a second dead-key hole. With a progress image showing and not temporarily overridden, toggleMetadata is enabled: false → unregistered, and the eyedropper is off because gallery/viewer holds focus → I does nothing. Same outcome on main (for a different reason), so not a regression, but QA step 3 in the description ("Switch to Viewer and confirm I toggles the metadata panel") fails in that state.
2. The generic change also drops preventDefault masking app-wide. maybePreventDefault runs before the enabled check, so statically-disabled hotkeys used to swallow their key's browser default. They no longer do. Concretely: workflows.selectAll is mod+a + preventDefault gated on isWorkflowsFocused (Flow.tsx:581), and gallery.selectAllOnPage is mod+a + preventDefault gated on isGalleryFocused (GallerySelectionCountTag.tsx:27). Focus the left panel on the Workflows tab and Ctrl/Cmd+A now select-alls the page text; previously one of those two disabled listeners ate it. Same class for mod+c/mod+v/mod+z and delete/backspace. This looks like an acceptable trade for the fix, but it is an app-wide behaviour change that deserves a line in the PR description.
3. The helper's stated guarantee doesn't cover predicate gates. getRegisteredHotkeyOptions short-circuits only a literal false. Two call sites pass a function (useNextPrevEntity.ts:76,95), which still register listeners that stopImmediatePropagation when the predicate returns false — the exact bug class the doc comment claims to close. Harmless today (both merely duplicate the text-session guard), but the comment overstates what is enforced.
4. Tests don't cover the behavioural change. The 3 new tests exercise only the pure helper; nothing pins which region owns I, which is the risky half of the diff. The enabled: undefined → true branch is untested. And expect(...).toBe(options) asserts reference identity, which is stronger than the contract — a correct {...options} refactor would fail it.
5. Listener churn on the eyedropper. ToolColorPickerButton now passes an inline options literal, so _options gets a fresh identity every render → a fresh enabled closure → deepEqual compares functions by reference (dist:265) → the document keydown/keyup listeners are torn down and re-added on every render. On main it registered once. This is idiomatic for the codebase (every other call site does the same), but it does keep moving the eyedropper listener to the end of the document listener list.
Attacks that failed
For completeness, things I tried to break and couldn't:
- Double-fire: eyedropper
!(g‖v)and metadata(g‖v) && …are mutually exclusive — no state fires both. - Duplicate registration: only one tab layout mounts at a time (
AppContent.tsx:48-49).ImageViewerPaneldoes render two<ImageViewer/>when!lastSelectedItem(ImageViewerPanel.tsx:23,25), but in that state the metadata button isn't rendered at all, so no double-toggle. - Hidden viewer panel: dockview's default
onlyWhenVisibleonly detaches the DOM element (dockview.cjs.js:4996); the React portal survives (ReactPart.createPortal,:11290), so the viewer's hotkeys stay live behind the Canvas tab — which is what made #9467 reproducible in the first place. - Losing the canvas text-session guard: skipping the wrapper for
enabled: falsecan't weaken it — an unregistered hotkey can't fire. - Newly-unmasked
esc/entercollisions: the canvas apply/cancel handlers all gate onisCanvasFocusedandclearSelectiononisGalleryFocused— disjoint, so unmasking them doesn't create new double-fires.
|
Thanks for the thorough review. Blocker - addressed. 1. Progress image override - expected behavior. After clicking a Gallery thumbnail, the selected image is displayed briefly. During that interval, the existing info button turns blue when activated but does not display metadata. The 2. Browser-default masking - acknowledged. Preserving literal 3. Predicate gates - acknowledged. The helper only avoids mounting a listener when 4. Test coverage - addressed. The follow-up adds logic-level regression coverage for 5. Listener churn - addressed. The eyedropper and metadata hotkey options are now memoized, so unrelated component renders no longer cause listener teardown and re-registration. |
Route the shared I hotkey based on the active central panel. Register the metadata hotkey independently of the toolbar button. Add routing regression coverage and memoize hotkey options.
9d297f5 to
c6b92a5
Compare
Summary
Follow-up to #9482.
The Canvas eyedropper and Viewer metadata panel both use
I. Disabled hotkeys were wrapped in anenabledcallback, causingreact-hotkeys-hookto keep their listeners registered. A disabled listener could then block the active handler for the same key.This keeps
enabled: falsestatic so disabled hotkeys are not registered, while preserving the Canvas Text session guard and custom predicates. As a result, disabled handlers also no longer suppress native browser behavior when no enabled application hotkey owns the key.The shared
Ibinding is now routed according to whether Canvas or Viewer is active in the central workspace, even when focus moves to Gallery in the right sidebar:Iselects the eyedropper after interacting with Gallery.Itoggles the metadata panel for the selected Gallery item without requiring another click in Viewer.The metadata hotkey is registered independently of the conditionally rendered toolbar button, preventing gaps while selection data is resolving. Hotkey options are memoized so unrelated renders do not tear down and re-register the listeners.
Related Issues / Discussions
Re-Closes #9467
QA Instructions
Iselects the eyedropper.Iselects the eyedropper again.Itoggles the metadata panel.Itoggles the info panel without clicking the Viewer.Checklist
What's Newcopy (if doing a release after this PR)