From c1c15af6bf1f5efa568d777e2c8e78922beec0fd Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Thu, 29 Oct 2026 20:00:19 -0700 Subject: [PATCH] =?UTF-8?q?fix(app):=20give=20focus=20back=20when=20?= =?UTF-8?q?=E2=8C=98W=20closes=20the=20side=20panel's=20last=20New=20tab?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #60126 made closing a strip's last New tab close the side region, as closing its last tab does. #59002 assumed the panel stayed open after that press and then refocused the closed panel, so its e2e failed on dev. The region now gives focus back to where it was opened from whenever emptying it closes it, and ⌘W only keeps focus in the panel while it is still open. The e2e expectation is updated to #60126's intended behaviour: the first ⌘W closes the New tab with the panel, the next press closes nothing, and only then does ⌘W close the session tab. --- packages/app/e2e/regression/tabs.spec.ts | 10 +++---- packages/app/src/runtime/plugin/panels.tsx | 26 +++++++++++++------ .../session/commands/use-session-commands.tsx | 6 +++-- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packages/app/e2e/regression/tabs.spec.ts b/packages/app/e2e/regression/tabs.spec.ts index f413c7fb5d15..df428935c84f 100644 --- a/packages/app/e2e/regression/tabs.spec.ts +++ b/packages/app/e2e/regression/tabs.spec.ts @@ -1235,7 +1235,7 @@ test("⌘W closes the session tab again once the window loses focus after the si await expect(sessions.alphaTab).toHaveCount(0) }) -test("⌘W in the side panel closes its New tab, then the panel, and only then the session tab", async ({ page }) => { +test("⌘W in the side panel closes its last New tab with the panel, and only then the session tab", async ({ page }) => { await page.setViewportSize({ width: 1440, height: 900 }) const alpha = { id: "ses_side_new_alpha", title: "Side new alpha" } const beta = { id: "ses_side_new_beta", title: "Side new beta" } @@ -1254,14 +1254,14 @@ test("⌘W in the side panel closes its New tab, then the panel, and only then t // A launcher row in the New tab view holds focus as ⌘W removes it. await panel.getByRole("button", { name: /^Open file/ }).focus() + // The New tab was the panel's last tab, so closing it closes the panel too, and focus goes back. await page.keyboard.press("ControlOrMeta+w") await expect(newTab).toHaveCount(0) - await expect(toggle).toHaveAttribute("aria-expanded", "true") - - // Nothing is left to close in the panel, so ⌘W closes the panel rather than the session, and focus goes back. - await page.keyboard.press("ControlOrMeta+w") await expect(toggle).toHaveAttribute("aria-expanded", "false") await expect(composer).toBeFocused() + + // One press too many closes nothing rather than the session. + await page.keyboard.press("ControlOrMeta+w") await expect(alphaTab).toHaveCount(1) await expectPath(page, sessionHref(alpha.id)) diff --git a/packages/app/src/runtime/plugin/panels.tsx b/packages/app/src/runtime/plugin/panels.tsx index a33a98b1f49a..c4ca26abbafb 100644 --- a/packages/app/src/runtime/plugin/panels.tsx +++ b/packages/app/src/runtime/plugin/panels.tsx @@ -340,12 +340,27 @@ export function createRegion(input: { const empty = createMemo(() => strip().length === 0) + // Focus leaves before the close starts, back to where it was; otherwise the panel hands it to its toggle. + const giveBackFocus = () => { + const inside = !!document.getElementById("review-panel")?.contains(document.activeElement) + const back = returnFocus + returnFocus = undefined + + if (inside && back?.isConnected) back.focus({ preventScroll: true }) + } + + // Emptying the region closes it, so focus goes back as the user's own close would give it. + const emptied = () => { + giveBackFocus() + input.onEmptied?.() + } + // Closing the last New tab of a strip with no tabs closes the region, as closing its last tab does. const dismissNewTab = (id: string) => batch(() => { setState("adding", (list) => list.filter((value) => value.id !== id)) - if (empty() && !state.adding.some(live)) input.onEmptied?.() + if (empty() && !state.adding.some(live)) emptied() }) return { @@ -409,12 +424,7 @@ export function createRegion(input: { const focused = document.activeElement if (side.opened()) { - const inside = !!document.getElementById("review-panel")?.contains(focused) - const back = returnFocus - returnFocus = undefined - - // Focus leaves before the close starts, back to where it was; otherwise the panel hands it to its toggle. - if (inside && back?.isConnected) back.focus({ preventScroll: true }) + giveBackFocus() side.toggle() return @@ -444,7 +454,7 @@ export function createRegion(input: { batch(() => { input.tabs().close(key) - if (last) input.onEmptied?.() + if (last) emptied() }) if (entry) diff --git a/packages/app/src/session/commands/use-session-commands.tsx b/packages/app/src/session/commands/use-session-commands.tsx index 3d2ca6ccf909..2d953398c3f3 100644 --- a/packages/app/src/session/commands/use-session-commands.tsx +++ b/packages/app/src/session/commands/use-session-commands.tsx @@ -241,8 +241,10 @@ export const useSessionCommands = (actions: SessionCommandContext) => { if (actions.region.closeNewTab(focusedPane())) { const panel = document.getElementById("review-panel") - // A launcher row that held focus is gone; focus stays in the panel so closing it next returns focus. - if (panel && !panel.contains(document.activeElement)) panel.focus({ preventScroll: true, focusVisible: false }) + // A launcher row that held focus is gone; focus stays in the panel so closing it next returns focus. Closing the + // strip's last New tab closes the panel too, which already gave focus back. + if (panel && actions.session.layout.view().side.opened() && !panel.contains(document.activeElement)) + panel.focus({ preventScroll: true, focusVisible: false }) return }