fix(ui): restore focus only when the overlay was actually open

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.
This commit is contained in:
d fei
2026-09-30 08:35:06 -07:00
parent af4cc45cfd
commit 57f7a77573
2 changed files with 50 additions and 8 deletions
+21 -3
View File
@@ -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 `<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() {
@@ -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');
},
+29 -5
View File
@@ -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', () => {
@@ -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();