From 90a6149140c33fe3d1ecd0d4471f2bdb2505840b Mon Sep 17 00:00:00 2001 From: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Date: Mon, 28 Sep 2026 14:21:08 -0400 Subject: [PATCH 1/3] Keep focus where the user moved it when a popup finishes closing Base UI returns focus when a Menu, Popover or Dialog popup finishes its exit transition. For the default `finalFocus` of `true`, it first checks whether the user already moved focus elsewhere. An explicit target skips that check (FloatingFocusManager: hasExplicitReturnFocus). So pressing Escape on the channel row menu and then clicking another control lost that click: the row menu finished closing and pulled focus back to the row, and a newly opened menu closed on focus loss. useFinalFocusUnlessMoved wraps an explicit finalFocus in the shared Menu, Popover and Dialog primitives. It returns the target only while focus is still in the closing popup or on the page body. Every caller of the primitives gets the fix. menu-dismiss.spec slows the exit transition so the next click lands first. Without this change it fails in 4 of 6 runs; with it, 6 of 6 pass. Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> --- src/shared/design-system/DESIGN.md | 4 ++ src/shared/design-system/ui/Dialog.tsx | 5 +- src/shared/design-system/ui/Menu.tsx | 6 +++ src/shared/design-system/ui/Popover.tsx | 6 +++ src/shared/design-system/ui/finalFocus.ts | 62 +++++++++++++++++++++++ tests/browser/menu-dismiss.spec.mjs | 48 ++++++++++++++++++ 6 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 src/shared/design-system/ui/finalFocus.ts create mode 100644 tests/browser/menu-dismiss.spec.mjs diff --git a/src/shared/design-system/DESIGN.md b/src/shared/design-system/DESIGN.md index 27f30571c..0692cec72 100644 --- a/src/shared/design-system/DESIGN.md +++ b/src/shared/design-system/DESIGN.md @@ -292,6 +292,10 @@ close button and actions. Pending operations set preventClose so Escape and the close button agree. It retains the app's explicit dismissal behavior: outside clicks do not discard a form. Provide initialFocus for search dialogs and finalFocus when a flow has an external trigger or opens a second dialog. +An explicit finalFocus applies only while focus is still in the closing popup +or on the page body. If the user already moved focus elsewhere, closing leaves +it there, as the default `true` does (`ui/finalFocus.ts`, shared by Menu, +Popover and Dialog). Editors can supply `headerActions` beside Close and `leadingActions` before the trailing footer actions. `onEscape` may return true to consume Escape for an inline layer (such as an inspector) before dismissing the dialog. Nested modal diff --git a/src/shared/design-system/ui/Dialog.tsx b/src/shared/design-system/ui/Dialog.tsx index aad2fbcd1..68cc31a6f 100644 --- a/src/shared/design-system/ui/Dialog.tsx +++ b/src/shared/design-system/ui/Dialog.tsx @@ -2,6 +2,7 @@ import { Dialog as BaseDialog } from "@base-ui/react/dialog"; import { XIcon } from "../icons"; import { useState, type ComponentProps, type ReactNode } from "react"; import { IconButton } from "./IconButton"; +import { useFinalFocusUnlessMoved } from "./finalFocus"; type PopupProps = ComponentProps; export type DialogProps = { @@ -56,6 +57,7 @@ export function Dialog({ finalFocus, }: DialogProps) { const [instantClose, setInstantClose] = useState(false); + const focus = useFinalFocusUnlessMoved(finalFocus, undefined); const transition = motion === "none" || (!open && instantClose) ? "none" : "default"; return ( @@ -93,7 +95,8 @@ export function Dialog({ data-motion={transition} aria-modal="true" initialFocus={initialFocus} - finalFocus={finalFocus} + ref={focus.ref} + finalFocus={focus.finalFocus} >
diff --git a/src/shared/design-system/ui/Menu.tsx b/src/shared/design-system/ui/Menu.tsx index d650335fe..751a29d7e 100644 --- a/src/shared/design-system/ui/Menu.tsx +++ b/src/shared/design-system/ui/Menu.tsx @@ -1,6 +1,7 @@ import { ContextMenu as BaseContextMenu } from "@base-ui/react/context-menu"; import { Menu as BaseMenu } from "@base-ui/react/menu"; import { CheckIcon, CaretRightIcon } from "../icons/index"; +import { useFinalFocusUnlessMoved } from "./finalFocus"; import type { ComponentProps, ReactNode } from "react"; export const MenuRoot = BaseMenu.Root; @@ -40,8 +41,11 @@ function PositionedPopup({ side = "bottom", sideOffset = 4, sticky, + finalFocus, + ref, ...props }: PopupProps) { + const focus = useFinalFocusUnlessMoved(finalFocus, ref); return ( ["finalFocus"]; +type CloseType = Parameters< + Extract unknown> +>[0]; + +/** + * Keep an explicit `finalFocus` from pulling focus back after the user moved it. + * + * Base UI returns focus when a popup finishes closing. For the default `true` + * it first checks whether focus already moved somewhere else, but an explicit + * target skips that check. A user who pressed Escape and at once clicked + * another menu button then lost that new menu: the old popup finished its + * exit, focused its own trigger, and the new menu closed on focus loss. + * + * The popup element is kept after unmount so the check still works when Base + * UI resolves focus after React clears the ref. + */ +export function useFinalFocusUnlessMoved( + finalFocus: FinalFocus, + ref: Ref | undefined, +) { + const popup = useRef(null); + const setPopup = useCallback( + (element: HTMLDivElement | null) => { + if (element) popup.current = element; + if (typeof ref === "function") return ref(element); + if (ref) ref.current = element; + }, + [ref], + ); + const resolve = useCallback( + (closeType: CloseType) => { + const target = + typeof finalFocus === "function" + ? finalFocus(closeType) + : typeof finalFocus === "object" + ? finalFocus.current + : finalFocus; + const doc = popup.current?.ownerDocument ?? document; + const active = doc.activeElement; + const moved = + active instanceof HTMLElement && + active !== doc.body && + active !== target && + !popup.current?.contains(active) && + !active.closest("[data-closed]"); + return moved ? false : target; + }, + [finalFocus], + ); + return { + ref: setPopup, + finalFocus: + finalFocus === undefined || typeof finalFocus === "boolean" + ? finalFocus + : resolve, + }; +} diff --git a/tests/browser/menu-dismiss.spec.mjs b/tests/browser/menu-dismiss.spec.mjs new file mode 100644 index 000000000..8bc712e6c --- /dev/null +++ b/tests/browser/menu-dismiss.spec.mjs @@ -0,0 +1,48 @@ +import { test, expect } from "./fixture.mjs"; +import { open } from "./timeline.mjs"; + +// Browser-only boundary: Base UI restores focus when a popup's exit +// transition ends. A slow transition lets the user's next click land first. +const slowMenus = `:is(.buzz-menu-popup, .buzz-popover-popup) { + transition-duration: 400ms !important; +}`; +const settled = (locator) => + expect + .poll(() => locator.evaluate((element) => element.getAnimations().length)) + .toBe(0); + +test("closing a menu keeps focus where the user moved it", async ({ + page, + app, +}) => { + await open(page, app); + await page.addStyleTag({ content: slowMenus }); + const row = page + .getByRole("navigation", { + name: "Subscribed channels", + includeHidden: true, + }) + .locator('[data-channel-id="beta"]'); + const rowMenu = page.getByRole("menu", { name: "Actions for Beta" }); + const composer = page.getByRole("textbox", { + name: "Message #Alpha", + exact: true, + }); + + await row.click({ button: "right" }); + await settled(rowMenu); + await page.keyboard.press("Escape"); + await composer.click(); + await expect(composer).toBeFocused(); + // Only now does the row menu finish closing. Its explicit final focus (the + // row) must not pull focus back out of the composer. + await expect(rowMenu).toHaveCount(0); + await expect(composer).toBeFocused(); + + // With nothing else focused, Escape still returns focus to the row. + await row.click({ button: "right" }); + await settled(rowMenu); + await page.keyboard.press("Escape"); + await expect(rowMenu).toHaveCount(0); + await expect(row).toBeFocused(); +}); From 35dd419c37420c894ab20f4487da1894cd1101c4 Mon Sep 17 00:00:00 2001 From: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Date: Mon, 28 Sep 2026 14:51:38 -0400 Subject: [PATCH 2/3] Name the final-focus hook's return type for author declarations The author package emits declarations for the design system. The hook's inferred return type used React's private UNDEFINED_VOID_ONLY, so tsc could not name it (TS4058) and author:build failed. Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> --- src/shared/design-system/ui/finalFocus.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/shared/design-system/ui/finalFocus.ts b/src/shared/design-system/ui/finalFocus.ts index b8276e7eb..80f2d3f0d 100644 --- a/src/shared/design-system/ui/finalFocus.ts +++ b/src/shared/design-system/ui/finalFocus.ts @@ -1,5 +1,11 @@ import type { Menu } from "@base-ui/react/menu"; -import { useCallback, useRef, type ComponentProps, type Ref } from "react"; +import { + useCallback, + useRef, + type ComponentProps, + type Ref, + type RefCallback, +} from "react"; /** Menu, Popover and Dialog popups share this Base UI prop. */ type FinalFocus = ComponentProps["finalFocus"]; @@ -22,9 +28,9 @@ type CloseType = Parameters< export function useFinalFocusUnlessMoved( finalFocus: FinalFocus, ref: Ref | undefined, -) { +): { ref: RefCallback; finalFocus: FinalFocus } { const popup = useRef(null); - const setPopup = useCallback( + const setPopup = useCallback>( (element: HTMLDivElement | null) => { if (element) popup.current = element; if (typeof ref === "function") return ref(element); From f416250425a11668e3fbc4a3179a3aa13af085ae Mon Sep 17 00:00:00 2001 From: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Date: Mon, 28 Sep 2026 17:15:43 -0400 Subject: [PATCH 3/3] Count only closing popups as the closing tree, and gate the exit in the test Review found two problems. useFinalFocusUnlessMoved treated focus under any [data-closed] ancestor as not moved. Base UI also marks collapsed Collapsible and Accordion roots data-closed, and visible controls can sit inside them. The exception now matches only popup roles (menu, dialog, alertdialog, listbox). menu-dismiss.spec relied on a 400ms transition, so the race might not happen. The test now pauses the menu's exit transition when it starts, checks that closing has started, moves focus while the menu is still mounted, and then releases the transition. It also waits until the entrance has settled: an Escape before the entrance starts closes with no transition to hold. A new case moves focus under an unrelated data-closed container. Fail-before, 10 runs per engine, chromium and webkit: - without the hook's moved check: 10/10 fail (focus pulled to the row) - with the old broad [data-closed] match: 10/10 fail (collapsed case) - with this change: 10/10 pass Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> --- src/shared/design-system/ui/finalFocus.ts | 10 +- tests/browser/menu-dismiss.spec.mjs | 112 +++++++++++++++++----- 2 files changed, 98 insertions(+), 24 deletions(-) diff --git a/src/shared/design-system/ui/finalFocus.ts b/src/shared/design-system/ui/finalFocus.ts index 80f2d3f0d..fe3648d65 100644 --- a/src/shared/design-system/ui/finalFocus.ts +++ b/src/shared/design-system/ui/finalFocus.ts @@ -13,6 +13,14 @@ type CloseType = Parameters< Extract unknown> >[0]; +/** + * Focus inside another closing popup (a submenu, or the popup this one is + * nested in) is part of the closing tree, not a user move. Collapsed + * disclosures also carry `data-closed`, so match popup roles only. + */ +const closingPopup = + ':is([role="menu"], [role="dialog"], [role="alertdialog"], [role="listbox"])[data-closed]'; + /** * Keep an explicit `finalFocus` from pulling focus back after the user moved it. * @@ -53,7 +61,7 @@ export function useFinalFocusUnlessMoved( active !== doc.body && active !== target && !popup.current?.contains(active) && - !active.closest("[data-closed]"); + !active.closest(closingPopup); return moved ? false : target; }, [finalFocus], diff --git a/tests/browser/menu-dismiss.spec.mjs b/tests/browser/menu-dismiss.spec.mjs index 8bc712e6c..c880aa32e 100644 --- a/tests/browser/menu-dismiss.spec.mjs +++ b/tests/browser/menu-dismiss.spec.mjs @@ -2,21 +2,57 @@ import { test, expect } from "./fixture.mjs"; import { open } from "./timeline.mjs"; // Browser-only boundary: Base UI restores focus when a popup's exit -// transition ends. A slow transition lets the user's next click land first. -const slowMenus = `:is(.buzz-menu-popup, .buzz-popover-popup) { - transition-duration: 400ms !important; -}`; -const settled = (locator) => - expect - .poll(() => locator.evaluate((element) => element.getAnimations().length)) +// transition ends. The test holds that transition, so the user's next focus +// move lands while the old menu is still closing. +async function holdMenuExit(page) { + await page.evaluate(() => { + document.addEventListener( + "transitionrun", + (event) => { + if ( + event.target instanceof HTMLElement && + event.target.matches(".buzz-menu-popup[data-ending-style]") + ) + for (const animation of event.target.getAnimations()) + animation.pause(); + }, + true, + ); + }); + return { + // Closing has started and cannot finish until release(). + held: (popup) => + expect + .poll(() => + popup.evaluate((element) => + element.getAnimations().some((a) => a.playState === "paused"), + ), + ) + .toBe(true), + release: () => + page.evaluate(() => { + for (const animation of document.getAnimations()) + if (animation.playState === "paused") animation.finish(); + }), + }; +} + +// The entrance has started and settled. Animations alone are not enough: an +// Escape before the entrance transition starts closes without any transition. +async function opened(popup) { + await expect(popup).toHaveAttribute("data-open", ""); + await expect(popup).not.toHaveAttribute("data-starting-style"); + await expect + .poll(() => popup.evaluate((element) => element.getAnimations().length)) .toBe(0); +} test("closing a menu keeps focus where the user moved it", async ({ page, app, }) => { await open(page, app); - await page.addStyleTag({ content: slowMenus }); + const exit = await holdMenuExit(page); const row = page .getByRole("navigation", { name: "Subscribed channels", @@ -29,20 +65,50 @@ test("closing a menu keeps focus where the user moved it", async ({ exact: true, }); - await row.click({ button: "right" }); - await settled(rowMenu); - await page.keyboard.press("Escape"); - await composer.click(); - await expect(composer).toBeFocused(); - // Only now does the row menu finish closing. Its explicit final focus (the - // row) must not pull focus back out of the composer. - await expect(rowMenu).toHaveCount(0); - await expect(composer).toBeFocused(); + try { + await row.click({ button: "right" }); + await opened(rowMenu); + await page.keyboard.press("Escape"); + await exit.held(rowMenu); + await composer.click(); + await expect(composer).toBeFocused(); + await expect(rowMenu).toHaveCount(1); + await exit.release(); + // The row menu has finished closing. Its explicit final focus (the row) + // must not pull focus back out of the composer. + await expect(rowMenu).toHaveCount(0); + await expect(composer).toBeFocused(); + + // A collapsed disclosure (Base UI marks it data-closed, like a closing + // popup) is ordinary page content. Focus moved into it also stays. + await page.evaluate(() => { + const collapsed = document.createElement("div"); + collapsed.setAttribute("data-closed", ""); + collapsed.innerHTML = ''; + document.getElementById("main-content").append(collapsed); + }); + const collapsedControl = page.getByRole("button", { + name: "Collapsed control", + }); + await row.click({ button: "right" }); + await opened(rowMenu); + await page.keyboard.press("Escape"); + await exit.held(rowMenu); + await collapsedControl.focus(); + await expect(rowMenu).toHaveCount(1); + await exit.release(); + await expect(rowMenu).toHaveCount(0); + await expect(collapsedControl).toBeFocused(); - // With nothing else focused, Escape still returns focus to the row. - await row.click({ button: "right" }); - await settled(rowMenu); - await page.keyboard.press("Escape"); - await expect(rowMenu).toHaveCount(0); - await expect(row).toBeFocused(); + // With nothing else focused, Escape still returns focus to the row. + await row.click({ button: "right" }); + await opened(rowMenu); + await page.keyboard.press("Escape"); + await exit.held(rowMenu); + await exit.release(); + await expect(rowMenu).toHaveCount(0); + await expect(row).toBeFocused(); + } finally { + await exit.release(); + } });