Skip to content

fix(ui): make I hotkey work in Canvas, Gallery, and Viewer - #9569

Open
DustyShoe wants to merge 4 commits into
invoke-ai:mainfrom
DustyShoe:fix/disabled-hotkeys-block-shared-keys
Open

fix(ui): make I hotkey work in Canvas, Gallery, and Viewer#9569
DustyShoe wants to merge 4 commits into
invoke-ai:mainfrom
DustyShoe:fix/disabled-hotkeys-block-shared-keys

Conversation

@DustyShoe

@DustyShoe DustyShoe commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #9482.

The Canvas eyedropper and Viewer metadata panel both use I. Disabled hotkeys were wrapped in an enabled callback, causing react-hotkeys-hook to keep their listeners registered. A disabled listener could then block the active handler for the same key.

This keeps enabled: false static 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 I binding 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:

  • When Canvas is active, I selects the eyedropper after interacting with Gallery.
  • When Viewer is active, I toggles 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

  • Switch to Canvas and confirm I selects the eyedropper.
  • Select another Canvas tool and confirm I selects the eyedropper again.
  • Switch to Viewer and confirm I toggles the metadata panel.
  • Select another image in Gallery and confirm that I toggles the info panel without clicking the Viewer.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Changes to a redux slice have a corresponding migration
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions Bot added the frontend PRs that change frontend files label Sep 4, 2026
@DustyShoe
DustyShoe marked this pull request as ready for review September 4, 2026 14:23
@DustyShoe DustyShoe changed the title fix(ui): prevent disabled hotkeys from blocking shared keys fix(ui): make I hotkey work in Canvas, Gallery, and Viewer Sep 5, 2026
@lstein lstein self-assigned this Sep 6, 2026
@lstein lstein added the 6.14.2 label Sep 6, 2026

@lstein lstein left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 when options.enabled === false, so no listener is attached (dist/react-hotkeys-hook.esm.js:389).
  • When enabled is a function, the listener is attached and, on a key match, calls stopPropagation(e)stopImmediatePropagation() + preventDefault() before consulting the predicate (:424-427). Every registered hotkey binds to document, 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:

  • ToggleMetadataViewerButton is only rendered when galleryItem?.kind === 'image' | 'video' (ImageViewerToolbar.tsx:28) — i.e. only when an item is selected and its DTO has resolved.
  • gallery.selection starts as [] and is in persistDenylist (gallerySlice.ts:22, :233), so every page reload starts with no selection. It is also emptied by imageSelected(null) (:52) and showVirtualBoardsChanged (:150).
  • The panel container is tabIndex={-1} (AutoLayoutPanelContainer.tsx:45), so clicking gallery whitespace, the scrollbar, or an empty board focuses the gallery region 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). ImageViewerPanel does 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 onlyWhenVisible only 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: false can't weaken it — an unregistered hotkey can't fire.
  • Newly-unmasked esc/enter collisions: the canvas apply/cancel handlers all gate on isCanvasFocused and clearSelection on isGalleryFocused — disjoint, so unmasking them doesn't create new double-fires.

@github-actions github-actions Bot added the Root label Sep 6, 2026
@DustyShoe

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review.

Blocker - addressed. I is now routed according to the active central panel rather than Gallery focus alone. When Canvas is active, interacting with the adjacent Gallery no longer blocks the eyedropper. When Viewer is active, selecting another Gallery item keeps I assigned to metadata without requiring another click in Viewer. The metadata hotkey is also registered independently of the conditionally rendered toolbar button.

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 I hotkey triggers the same UI state and behavior as that button, so this is not a regression introduced by the PR.

2. Browser-default masking - acknowledged. Preserving literal enabled: false means disabled handlers no longer mount solely to suppress another handler or the browser default. That is an intentional tradeoff of the generic fix and should be called out in the PR description.

3. Predicate gates - acknowledged. The helper only avoids mounting a listener when enabled is literally false. Predicate-based gates remain mounted by react-hotkeys-hook; the helper documentation should state that limitation explicitly. The existing predicate call sites are unchanged by this PR.

4. Test coverage - addressed. The follow-up adds logic-level regression coverage for I ownership across active Canvas and Viewer states, Gallery and Viewer focus, and a missing metadata target. It also covers enabled: undefined and replaces the reference-identity assertion with value equality. The rendered interaction remains part of manual QA because the repository has no approved DOM test framework.

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.
@DustyShoe
DustyShoe force-pushed the fix/disabled-hotkeys-block-shared-keys branch from 9d297f5 to c6b92a5 Compare September 6, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.2 frontend PRs that change frontend files Root

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: "i" hotkey on canvas no longer activates the eyedropper tool

2 participants