From af4cc45cfd9ee1d6421479c2a2aa86ff4d7a7f4c Mon Sep 17 00:00:00 2001 From: d fei Date: Tue, 29 Sep 2026 18:02:05 -0700 Subject: [PATCH] fix(ui): stop Escape from stranding the keyboard after closing an overlay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `` — 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. --- src/web/public/panels-ui.js | 50 +++++++++++++++++++++ test/command-palette-ui.test.ts | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 8bdf5df8..81c7c990 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -339,6 +339,50 @@ Object.assign(CodemanApp.prototype, { return true; }, + /** + * Save what had focus before a search-first overlay takes it. + * + * The Command Palette and the Session Manager both call `search.focus()` on + * open, and both used to close by removing the `active` class and nothing + * else. Hiding the focused input does not hand focus back to anyone — the + * browser drops it on `` — so after Escape closed the overlay every + * keystroke went nowhere and the user had to click the terminal to type + * again (measured: `document.activeElement` is BODY afterwards and the + * terminal emits no onData at all). Every close path has the same hole, so + * the restore lives in the close functions, not in the global Escape chain. + * + * ⚠️ Deliberately only the save/restore half of {@link FocusTrap}, not the + * whole thing. `FocusTrap.activate()` moves focus to the first focusable + * element, which in both of these overlays is not the search box — adopting + * it wholesale would fix the focus loss by breaking the thing Cmd+K exists + * for, typing a filter the moment it opens. + */ + _rememberOverlayFocus(key) { + this[key] = (typeof document !== 'undefined' && document.activeElement) || null; + }, + + /** + * Hand focus back to whatever {@link _rememberOverlayFocus} saved. + * + * ⚠️ 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 that + * has no input on it. + */ + _restoreOverlayFocus(key) { + const prev = this[key]; + this[key] = null; + const body = typeof document !== 'undefined' ? document.body : null; + // `isConnected === false` means the element was removed while the overlay + // was open (a re-render of the tab strip, say); anything else — including + // a stub with no such property — is treated as still focusable. + if (prev && prev !== body && prev.isConnected !== false && typeof prev.focus === 'function') { + prev.focus(); + return; + } + if (this.activeSessionId) this.terminal?.focus?.(); + }, + openCommandPalette() { const modal = document.getElementById('commandPaletteModal'); const search = document.getElementById('commandPaletteSearch'); @@ -351,6 +395,9 @@ Object.assign(CodemanApp.prototype, { this._wireCommandPalette(); this.renderCommandPalette(); + // BEFORE the steal, not after: `search.focus()` below is what loses the + // caller's focus, so the read has to happen while it is still there. + this._rememberOverlayFocus('_commandPalettePrevFocus'); search.focus(); search.select?.(); }, @@ -358,6 +405,7 @@ Object.assign(CodemanApp.prototype, { closeCommandPalette() { const modal = document.getElementById('commandPaletteModal'); if (modal) modal.classList.remove('active'); + this._restoreOverlayFocus('_commandPalettePrevFocus'); }, _wireCommandPalette() { @@ -618,6 +666,7 @@ Object.assign(CodemanApp.prototype, { }); } search.value = ''; + this._rememberOverlayFocus('_sessionManagerPrevFocus'); search.focus(); } await this._loadSessionManagerList(''); @@ -626,6 +675,7 @@ Object.assign(CodemanApp.prototype, { closeSessionManager() { const modal = document.getElementById('sessionManagerModal'); if (modal) modal.classList.remove('active'); + this._restoreOverlayFocus('_sessionManagerPrevFocus'); }, /** Replace the Session Manager list body with a single status line. */ diff --git a/test/command-palette-ui.test.ts b/test/command-palette-ui.test.ts index bbb6813d..70e19ced 100644 --- a/test/command-palette-ui.test.ts +++ b/test/command-palette-ui.test.ts @@ -480,6 +480,84 @@ describe('Session Manager unified list', () => { }); }); +describe('overlay focus restoration (Escape must not strand the keyboard)', () => { + /** + * Both overlays focus their search box on open. Closing them used to leave + * focus on , so after Escape every keystroke went nowhere until the + * user clicked the terminal — measured in a real browser against a shell + * session: activeElement BODY, zero onData for anything typed afterwards. + */ + function focusHarness() { + const terminalTextarea = { focus: vi.fn(), isConnected: true }; + const priorElement = { focus: vi.fn(), isConnected: true, tagName: 'TEXTAREA' }; + const body = { tagName: 'BODY' }; + let active: any = priorElement; + const { app, elements } = loadPaletteHarness({ + document: { + getElementById: (id: string) => (globalThis as any).__els?.[id] ?? null, + get activeElement() { + return active; + }, + body, + }, + }); + (globalThis as any).__els = elements; + app.terminal = { focus: terminalTextarea.focus }; + app.activeSessionId = 'sess-beta'; + // The overlay's own focus() is what moves focus in a real browser; the + // fake document needs the same transition or the test proves nothing. + elements.commandPaletteSearch.focus = vi.fn(() => { + active = elements.commandPaletteSearch; + }); + return { app, elements, priorElement, terminalTextarea, body, setActive: (v: any) => (active = v) }; + } + + it('returns focus to whatever had it when the command palette closes', () => { + const { app, priorElement } = focusHarness(); + app.openCommandPalette(); + expect(priorElement.focus).not.toHaveBeenCalled(); + app.closeCommandPalette(); + expect(priorElement.focus).toHaveBeenCalledTimes(1); + }); + + it('falls back to the terminal when the prior element is gone, but only with a live session', () => { + const { app, priorElement, terminalTextarea } = focusHarness(); + app.openCommandPalette(); + priorElement.isConnected = false; + app.closeCommandPalette(); + expect(priorElement.focus).not.toHaveBeenCalled(); + expect(terminalTextarea.focus).toHaveBeenCalledTimes(1); + }); + + it('never focuses the terminal from the welcome screen (a phone would pop the keyboard)', () => { + const { app, priorElement, terminalTextarea } = focusHarness(); + app.activeSessionId = null; + app.openCommandPalette(); + priorElement.isConnected = false; + app.closeCommandPalette(); + expect(terminalTextarea.focus).not.toHaveBeenCalled(); + }); + + it('does not restore focus to , which is the bug itself', () => { + const { app, terminalTextarea, body, setActive } = focusHarness(); + setActive(body); + app.openCommandPalette(); + app.closeCommandPalette(); + expect(terminalTextarea.focus).toHaveBeenCalledTimes(1); + }); + + it('restores focus on the session manager too, not just the palette', async () => { + const { app, elements, priorElement } = focusHarness(); + elements.sessionManagerModal = { classList: { add: vi.fn(), remove: vi.fn() }, addEventListener: vi.fn() }; + elements.sessionManagerSearch = { value: '', focus: vi.fn(), addEventListener: vi.fn() }; + elements.sessionManagerList = { replaceChildren: vi.fn(), appendChild: vi.fn() }; + app._loadSessionManagerList = vi.fn(); + await app.openSessionManager(); + app.closeSessionManager(); + expect(priorElement.focus).toHaveBeenCalledTimes(1); + }); +}); + describe('panel close helpers', () => { it('closes panels when the mobile header helper is unavailable', () => { const CodemanApp = function CodemanApp(this: any) {};