Skip to content

fix(keybinds): palette and ⌘W work in an extension modal before its iframe has focus - #1394

Merged
Juliusolsson05 merged 4 commits into
mainfrom
fix/extension-modal-palette-chord
Sep 27, 2026
Merged

Juliusolsson05 merged 4 commits into
mainfrom
fix/extension-modal-palette-chord

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Fixes #1307.

Plan: docs/plans/2026-09-27-extension-modal-palette-chord.md.

Problem

  • Where focus sits. When an extension modal opens, focus goes into the host shell. Radix autofocuses the shell's close button while viewBridge keeps the iframe hidden until ready, and focus stays in the shell until the user clicks into the extension.
  • The chord goes nowhere. Main forwards nothing, because the focused frame is the host. In the renderer the key targets the shell, not the iframe, so useKeybinds's extension-modal exemption misses it and the app-ownership gate drops ⌘⇧P.
  • Same for ⌘W: it falls through to "prevent default, do nothing" and leaves the modal open.

Change

  • One lookup. extensionModalFrameForTarget returns the modal iframe when the target is that iframe (the forwarded case), or when the target's own app-owner element contains it (the host-shell case).
  • Both exemptions use it, and the close event fires on the found iframe.
  • Scoped to the containing dialog. A surface stacked above the modal, such as the palette or a confirmation, contains no extension iframe, so it keeps the gate.
  • Focus on <body> (review b). Closing the palette leaves focus on <body>, because the palette is a triggerless Radix Dialog. A key outside every owner resolves against the topmost owned surface, the last in DOM order. That is the extension modal once the palette has closed, and a stacked confirmation while one is open.

Tests

Three new cases in focusModeKeyboardOwnership.renderer.test.tsx, driving the real useKeybinds with AppHostSurface's DOM shape:

  • ⌘⇧P on the shell opens the palette. Red on main.
  • ⌘W on the shell fires agent-code-extension-close on the iframe and invokes no command. Red on main.
  • Control: ⌘⇧P in an owned dialog without an extension frame stays dropped.
  • Review b: body-targeted ⌘⇧P and ⌘W reach the extension after the palette closes, and stay blocked while a stacked dialog is open.
  • A dialog stacked above the extension modal keeps both chords: no palette, no extension close. It kills the document-wide-lookup mutation, which the first three tests survived.

The new cases clean up their owner-marked DOM after every test. Otherwise a failing assertion leaves the marker behind and cascades into the other 9 tests in the file.

Verification

🤖 Generated with Claude Code

…t shell (#1307)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05 Juliusolsson05 added type:bug Something works wrong class:C4-input-trap A modal/latch/overlay traps input sev:P3 Minor labels Sep 27, 2026
…xtension modal (review a)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Disposition for review a (head a02dbd4)

  • Finding 1 (P2), fixed. The new test leaves both chords to a dialog stacked above the extension modal mounts the extension modal and then a separate owner-marked dialog, and presses ⌘⇧P and ⌘W on the upper one. It asserts no palette and no extension close. I confirmed the document-wide querySelector mutation now fails it (1 failed, 19 passed); it survived before.
  • Focus diagnosis, corrected. In the helper's WHY comment, the plan and the body: Radix autofocuses the shell's close button (the iframe is hidden until ready), not DialogContent itself. The fix is unchanged, because either target sits inside the owner-marked content.
  • Body-focus case, noted, no change. As the review found, no normal AppHostSurface sequence leaves focus on body, because Radix traps focus in the dialog.

…d surface (review b)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Disposition for review b (head 8271290)

  • Finding: both chords go dead after the palette closes, because focus falls to <body>. Fixed.
    • extensionModalFrameForTarget resolves a key that sits outside every owner against the topmost owned surface: the last in DOM order, since Radix portals append as they open.
    • Once the palette has closed, that is the extension modal. While a confirmation is stacked above it, the confirmation is topmost and has no extension iframe, so the gate holds.
    • Tests: body-targeted ⌘⇧P and ⌘W reach the extension after a stacked dialog is removed, and stay blocked while it is present. Removing the fallback fails the first; using the first owner instead of the topmost fails the second.
  • Focus restoration inside the palette itself, returning focus to the shell on close, is not done. The chord routing no longer depends on it, and changing the palette's close focus is a wider UX change.

Juliusolsson05 added a commit that referenced this pull request Sep 27, 2026
…s, pin shell and layer values (#1398 review)

The dismiss stack's wiring contract was wrong (a capture-phase Escape pre-empts
two-phase inner handlers; effect order is not stacking order for nested
mounts). It returns with the useKeybinds wiring after #1394. Stacking between
dialogs is by open order, not registry order; the comments now say so. The
SidePanel test pins children and layout, and the layer test pins exact values.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Disposition for review c (MERGE-READY; head 2d208304)

  • F1: the closest() scope was unpinned. Fixed with the test you proposed. With the extension modal and another owner both mounted, ⌘⇧P aimed at the extension shell opens the palette. The "always use the topmost owner" mutation now fails it.
  • F2: the ⌘⇧P exemption skips preventDefault(). No change. As you note, Cmd+Shift+P has no Electron or Chromium default. The ⌘W path, the one that matters, does prevent its default.

@Juliusolsson05

Copy link
Copy Markdown
Owner Author

READY (W1). Head 2d208304. Exact-head CI is green. Every reviewer is MERGE-READY: a (verify-report-a), b (verify-report-b) and c (report-c). Dispositions for all three rounds are posted above. merge-gate.sh --member: GATE PASS. Mergeable and clean.

@Juliusolsson05
Juliusolsson05 merged commit 75d68c8 into main Sep 27, 2026
2 checks passed
Juliusolsson05 added a commit that referenced this pull request Sep 27, 2026
@Juliusolsson05
Juliusolsson05 deleted the fix/extension-modal-palette-chord branch September 27, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

class:C4-input-trap A modal/latch/overlay traps input sev:P3 Minor type:bug Something works wrong

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Palette chord is dropped in an extension modal until its iframe has focus

1 participant