diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 8bdf5df8..0e5fdfec 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -339,6 +339,58 @@ 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; + } + // ⚠️ `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() { const modal = document.getElementById('commandPaletteModal'); const search = document.getElementById('commandPaletteSearch'); @@ -351,13 +403,25 @@ 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?.(); }, 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'); }, _wireCommandPalette() { @@ -618,6 +682,7 @@ Object.assign(CodemanApp.prototype, { }); } search.value = ''; + this._rememberOverlayFocus('_sessionManagerPrevFocus'); search.focus(); } await this._loadSessionManagerList(''); @@ -625,7 +690,10 @@ 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'); }, /** 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..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', () => { @@ -480,6 +480,108 @@ 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, makeClassList } = 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, makeClassList, 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('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, 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(); + 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) {};