Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 70 additions & 2 deletions src/web/public/panels-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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() {
Expand Down Expand Up @@ -618,14 +682,18 @@ Object.assign(CodemanApp.prototype, {
});
}
search.value = '';
this._rememberOverlayFocus('_sessionManagerPrevFocus');
search.focus();
}
await this._loadSessionManagerList('');
},

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. */
Expand Down
104 changes: 103 additions & 1 deletion test/command-palette-ui.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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) {};
Expand Down
Loading