fix(ui): stop Escape from stranding the keyboard after closing an overlay - #509
Conversation
…rlay Both the Command Palette and the Session Manager call `search.focus()` on open, and both closed by removing the `active` class and nothing else. Hiding a focused input does not hand focus back to anyone — the browser drops it on `<body>` — so after Escape closed the overlay every keystroke went nowhere and the user had to click the terminal before they could type again. Measured in headless chromium against a real shell session, one overlay at a time: overlay activeElement after Esc can type afterwards App Settings XTERM yes Session Options XTERM yes Token Stats XTERM yes Monitor Panel XTERM yes Session Manager BODY no <- fixed here Command Palette BODY no <- fixed here The four that worked did so because they use `FocusTrap`, whose `deactivate()` restores focus to whatever held it before. These two never got one. Every close path has the same hole — Escape, the close method, picking an item — so the restore lives in the close functions rather than in the global Escape chain. Deliberately only the save/restore half of `FocusTrap`, not the whole thing: `FocusTrap.activate()` moves focus to the first focusable element, which in neither overlay is the search box, so adopting it wholesale would trade "type a filter the moment it opens" for "focus survives the close" — and the former is the reason Cmd+K exists. The terminal fallback is gated on there being an active session: an overlay opened from the welcome screen has no terminal to return to, and focusing one on a phone summons the on-screen keyboard over a screen with no input on it. The five new cases were checked against the unfixed code first: four of them fail without this change.
|
Thanks for this, @dignfei, and welcome to Codeman! This PR saves whatever had focus before the Command Palette or the Session Manager opens and hands it back on close, so Escape no longer leaves the keyboard stranded on Two things need to change before it goes in. The first is the important one. 1. Every Escape press now moves focus into the main terminal, not only the ones that close these overlays ( The global Escape handler in
The fix is to restore focus only when the overlay was actually open: closeCommandPalette() {
const modal = document.getElementById('commandPaletteModal');
if (!modal?.classList.contains('active')) return;
modal.classList.remove('active');
this._restoreOverlayFocus('_commandPalettePrevFocus');
},and the same shape in
2. The terminal fallback pops the on-screen keyboard on a tablet ( Gating on if (this.activeSessionId && this._shouldFocusTerminalForTabSwitch?.() !== false) this.terminal?.focus?.();(The optional call keeps your vm test harness working as it is.) With those two changes this is ready to merge. Thanks again for the careful measurement work, the before/after table made this review much faster. |
Review feedback. The global Escape handler in app.js calls both
`closeSessionManager()` and `closeCommandPalette()` on every Escape, whether or
not either overlay is open, in the capture phase. Nothing was saved in that
case, so `_restoreOverlayFocus()` fell through to `terminal.focus()` and moved
focus before the focused element's own Escape handler ran:
- split view: with focus in Pane B, keys typed after Escape went to Pane A
- any text field (File Viewer editor, search and history filters, case picker):
keys typed after Escape went into the terminal
- inline tab rename: the capture-phase focus fired the input's blur (which
commits) before its own Escape handler (which cancels), so Escape committed
the rename instead of cancelling it
Both close methods now bail out on `classList.contains('active')`.
Separately, gating the terminal fallback on `activeSessionId` alone only covered
the welcome screen. On a touch device with the keyboard down, focus sits on
`<body>`, so closing the Session Manager focused the terminal and brought the
keyboard up — `selectSession()` deliberately skips that focus, and this
overrode it. It now goes through `_shouldFocusTerminalForTabSwitch()`.
Tests: the Session Manager case's modal stub now uses the harness's
`makeClassList()` (without `contains` the new guard reads it as "not open" and
skips the restore the case is about), plus two new cases — closing either
overlay without opening it first with an active session asserts the terminal was
not focused, which is the path the global Escape chain takes and none of the
five existing cases covered, and a touch device with the keyboard down asserts
the same. Each was checked against the unguarded code: removing either guard
turns exactly its own case red.
|
Both changed, thanks for catching the first one — the capture-phase detail is what I missed, and the rename case in particular would have been an unpleasant surprise. 1. 2. The fallback now goes through Tests:
I checked both new cases against the unguarded code rather than just watching them go green — removing either guard turns exactly its own case red, and nothing else.
|
…ager (#509 review) - _restoreOverlayFocus(key, modal) now leaves focus alone when something outside the overlay already holds it (not <body>, not inside the modal). The Session Manager's "Switch to session" and "Open folder" call selectSession() before closeSessionManager(), and the restore was pulling focus back from the terminal to the header button. Both close methods pass their modal; a regression test drives that order. - Test harness: focusHarness() routes getElementById through a local binding instead of leaking globalThis.__els, and its modal stubs report their own search box as contained, as the real DOM does. - CLAUDE.md and docs/architecture-invariants.md: record that the global Escape handler calls every close method on every Escape (capture phase), so a close method with side effects must return early when not open. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Merged, thanks @dignfei! This ships in 1.33.3. The One commit at merge time (988f111) for an edge in the Session Manager's row menu. "Switch to session" and "Open folder" call On #510, the two items from this morning's review are all that's left before it can go in too. |
2 participants
The bug
After Escape closes the Command Palette or the Session Manager, the keyboard is dead: every keystroke goes nowhere until you click the terminal again.
Both overlays call
search.focus()on open, and both closed by removing theactiveclass and nothing else. Hiding a focused input does not hand focus back to anyone — the browser drops it on<body>.Measured
headless chromium against a real shell session, one overlay at a time:
activeElementafter EscXTERMXTERMXTERMXTERMBODYBODYThe four that work do so because they use
FocusTrap, whosedeactivate()restores focus to whatever held it before. These two never got one.The fix
Save
document.activeElementbefore the overlay steals focus, restore it on close. Every close path has the same hole — Escape, the close method, picking an item — so the restore lives in the close functions rather than in the global Escape chain.Deliberately only the save/restore half of
FocusTrap, not the whole thing:FocusTrap.activate()moves focus to the first focusable element, which in neither overlay is the search box, so adopting it wholesale would trade "type a filter the moment it opens" for "focus survives the close" — and the former is the reason Cmd+K exists.The terminal fallback is gated on there being an active session: an overlay opened from the welcome screen has no terminal to return to, and focusing one on a phone summons the on-screen keyboard over a screen with no input on it.
Testing
Five new cases in
test/command-palette-ui.test.ts. Checked against the unfixed code first — four of the five fail without this change. Re-verified in a real browser afterwards: all six overlays now reportXTERMand accept typing.npm testgreen (the 2 pre-existingdocker-entrypointfailures on this machine reproduce on unmodified master), plus typecheck, lint, format:check and check:frontend-syntax.