From af4cc45cfd9ee1d6421479c2a2aa86ff4d7a7f4c Mon Sep 17 00:00:00 2001 From: d fei Date: Tue, 29 Sep 2026 18:02:05 -0700 Subject: [PATCH 1/2] 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) {}; From 57f7a775737e7ee51277b81dbeb432f36565edc1 Mon Sep 17 00:00:00 2001 From: d fei Date: Wed, 30 Sep 2026 08:35:06 -0700 Subject: [PATCH 2/2] fix(ui): restore focus only when the overlay was actually open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ``, 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. --- src/web/public/panels-ui.js | 24 ++++++++++++++++++++--- test/command-palette-ui.test.ts | 34 ++++++++++++++++++++++++++++----- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 81c7c990..0e5fdfec 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -380,7 +380,15 @@ Object.assign(CodemanApp.prototype, { prev.focus(); return; } - if (this.activeSessionId) this.terminal?.focus?.(); + // ⚠️ `activeSessionId` alone only covers the welcome screen. On a touch device + // with the keyboard down, focus sits on ``, so focusing the terminal here + // would summon the on-screen keyboard — `selectSession()` deliberately skips the + // focus for exactly that reason, and this would override it. The app's own + // predicate already encodes the rule (true on desktop, on touch only while the + // keyboard is open); the optional call keeps the vm test harness working. + if (this.activeSessionId && this._shouldFocusTerminalForTabSwitch?.() !== false) { + this.terminal?.focus?.(); + } }, openCommandPalette() { @@ -404,7 +412,15 @@ Object.assign(CodemanApp.prototype, { closeCommandPalette() { const modal = document.getElementById('commandPaletteModal'); - if (modal) modal.classList.remove('active'); + // ⚠️ Bail out when it was not open. The global Escape handler calls this on + // EVERY Escape (app.js), in the CAPTURE phase, so an unconditional restore + // runs before the focused element's own Escape handler and steals focus into + // the terminal: keys typed after Escape in split Pane B land in Pane A, keys + // typed in any text field land in the terminal, and the inline tab rename's + // Escape fires the input's blur (which commits) before its own handler + // (which cancels), turning a cancel into a rename. + if (!modal?.classList?.contains('active')) return; + modal.classList.remove('active'); this._restoreOverlayFocus('_commandPalettePrevFocus'); }, @@ -674,7 +690,9 @@ Object.assign(CodemanApp.prototype, { closeSessionManager() { const modal = document.getElementById('sessionManagerModal'); - if (modal) modal.classList.remove('active'); + // Same guard as closeCommandPalette — see the note there. + if (!modal?.classList?.contains('active')) return; + modal.classList.remove('active'); this._restoreOverlayFocus('_sessionManagerPrevFocus'); }, diff --git a/test/command-palette-ui.test.ts b/test/command-palette-ui.test.ts index 70e19ced..00048010 100644 --- a/test/command-palette-ui.test.ts +++ b/test/command-palette-ui.test.ts @@ -111,7 +111,7 @@ function loadPaletteHarness(overrides: Record = {}) { app.getSessionName = (session: any) => session.name || session.workingDir?.split('/').pop() || app.getShortId(session.id); - return { app, elements, listeners }; + return { app, elements, listeners, makeClassList }; } describe('Command-K session palette', () => { @@ -492,7 +492,7 @@ describe('overlay focus restoration (Escape must not strand the keyboard)', () = const priorElement = { focus: vi.fn(), isConnected: true, tagName: 'TEXTAREA' }; const body = { tagName: 'BODY' }; let active: any = priorElement; - const { app, elements } = loadPaletteHarness({ + const { app, elements, makeClassList } = loadPaletteHarness({ document: { getElementById: (id: string) => (globalThis as any).__els?.[id] ?? null, get activeElement() { @@ -509,7 +509,7 @@ describe('overlay focus restoration (Escape must not strand the keyboard)', () = elements.commandPaletteSearch.focus = vi.fn(() => { active = elements.commandPaletteSearch; }); - return { app, elements, priorElement, terminalTextarea, body, setActive: (v: any) => (active = v) }; + return { app, elements, priorElement, terminalTextarea, body, makeClassList, setActive: (v: any) => (active = v) }; } it('returns focus to whatever had it when the command palette closes', () => { @@ -546,9 +546,33 @@ describe('overlay focus restoration (Escape must not strand the keyboard)', () = expect(terminalTextarea.focus).toHaveBeenCalledTimes(1); }); + it('leaves focus alone when neither overlay was open — the path every Escape takes', () => { + // app.js's global Escape handler calls both close methods on EVERY Escape, + // in the capture phase. Nothing was saved, so an unguarded restore would fall + // through to the terminal and steal focus from split Pane B, from any text + // field, and turn the inline rename's Escape into a commit. + const { app, elements, terminalTextarea, priorElement, makeClassList } = focusHarness(); + elements.sessionManagerModal = { classList: makeClassList(), addEventListener: vi.fn() }; + app.closeCommandPalette(); + app.closeSessionManager(); + expect(terminalTextarea.focus).not.toHaveBeenCalled(); + expect(priorElement.focus).not.toHaveBeenCalled(); + }); + + it('does not focus the terminal on touch while the keyboard is down', () => { + const { app, terminalTextarea, body, setActive } = focusHarness(); + app._shouldFocusTerminalForTabSwitch = () => false; + setActive(body); + app.openCommandPalette(); + app.closeCommandPalette(); + expect(terminalTextarea.focus).not.toHaveBeenCalled(); + }); + 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() }; + const { app, elements, priorElement, makeClassList } = focusHarness(); + // A real classList: the close guard reads `contains('active')`, and a stub + // without it reports "not open" and skips the restore this test is about. + elements.sessionManagerModal = { classList: makeClassList(), addEventListener: vi.fn() }; elements.sessionManagerSearch = { value: '', focus: vi.fn(), addEventListener: vi.fn() }; elements.sessionManagerList = { replaceChildren: vi.fn(), appendChild: vi.fn() }; app._loadSessionManagerList = vi.fn();