Merge pull request #509 from dignfei/fix/overlay-focus-restore

fix(ui): stop Escape from stranding the keyboard after closing an overlay

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 `<body>` — 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 `<body>`, 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. */

test/command-palette-ui.test.ts

@@ -111,7 +111,7 @@ function loadPaletteHarness(overrides: Record<string, any> = {}) {
   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 <body>, 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 <body>, 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) {};