diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index 6e22ab35..85facf5c 100644 --- a/frontend/src/components/ui/Modal.dom.test.tsx +++ b/frontend/src/components/ui/Modal.dom.test.tsx @@ -23,7 +23,7 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} })); import Modal from "./Modal"; import { clearAll, notify } from "../../lib/notifications"; import { pushLayer, removeLayer, useLayer } from "../../lib/layerStack"; -import { flushSync } from "react-dom"; +import { createPortal, flushSync } from "react-dom"; // Fire a keydown on window (where Modal installs its handler), inside act. const press = (key: string, shiftKey = false) => @@ -311,6 +311,70 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => { document.body.removeChild(trigger); }); + it("hands its trigger to the dialog left standing above it, which restores it on close", () => { + // The standing upper dialog captured ITS restore target while the focus was inside the lower + // one — a control that is gone once the lower closes. So the lower skipping its own restore + // (above) handed the trigger to no one, and closing the upper left the focus on . + // The arrangement keyboard help or the error dialog opened over a dialog actually has. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + function Siblings() { + const [lower, setLower] = useState(true); + const [upper, setUpper] = useState(false); + return ( + <> + {lower && ( + {}}> + + + + )} + {upper && ( + {}}> + + + )} + + ); + } + render(); + screen.getByText("open upper").focus(); + act(() => void screen.getByText("open upper").click()); // opened from inside the lower dialog + const inUpper = screen.getByText("close upper"); + inUpper.focus(); + // The lower closes on its own under the standing upper (`.click()` moves no focus). + act(() => void screen.getByText("close lower").click()); + expect(screen.queryByText("close lower")).toBeNull(); + expect(document.activeElement).toBe(inUpper); + act(() => void inUpper.click()); + expect(document.activeElement).toBe(trigger); + document.body.removeChild(trigger); + }); + + it("restores the trigger when it closes with the focus in one of its menus", () => { + // The popover engine portals a menu out of the dialog's root (it carries no aria-modal), so a + // focus on a menu row read as "the user walked away": Back with a menu open inside a dialog + // left the focus on . The portal marker is the one `ui/AnchoredPopover` reads too. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + const portal = document.createElement("div"); + portal.setAttribute("data-floating-ui-portal", ""); + document.body.appendChild(portal); + const { unmount } = render( + {}}> + + {createPortal(, portal)} + , + ); + screen.getByText("menu-row").focus(); + unmount(); + expect(document.activeElement).toBe(trigger); + document.body.removeChild(trigger); + document.body.removeChild(portal); + }); + it("leaves the focus alone when the user moved it out of the dialog themselves", () => { // The other half of the same rule, and the branch nothing drove: taking focus back from where // the user deliberately put it is the rudest thing a closing dialog can do. diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index c757b169..35a00c20 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -75,6 +75,16 @@ export function lockBodyScroll(): () => void { }; } +// A popover engine surface (`ui/AnchoredPopover`): portalled out of the dialog's root and carrying no +// aria-modal, so a focus on one of a dialog's menu rows is inside the dialog all the same. +const FLOATING = "[data-floating-ui-portal]"; + +// Restore targets handed to a dialog left standing above one that closed (see the cleanup below): +// that dialog's own target was captured while the focus was in the one that closed, and is gone. +// Keyed by the standing dialog's aria-modal element; a WeakMap, so a dialog that never closes +// through this hook leaves nothing behind. +const handedRestore = new WeakMap(); + /** The rest of the contract: lock the page behind, open focused, hand the focus back on close. * `scopeRef` is the panel the focus should land in; `ownerRef`, when the dialog is bigger than its * panel, is that whole dialog — it must be the SAME element `trapTab` is scoped to, or the two @@ -128,6 +138,11 @@ export function useDialogChrome( // Today that reads the same either way (a detached owner and a null one both fail every // `contains` below), but the cleanup should not depend on a ref React has already let go of. const ownerAtOpen = ownerRef?.current ?? scope; + // This dialog's aria-modal element, the key a closing dialog under it hands its target by. + const modalAtOpen = + ownerAtOpen?.closest('[aria-modal="true"]') ?? + ownerAtOpen?.querySelector('[aria-modal="true"]') ?? + null; if (scope && !scope.contains(document.activeElement)) { const back = focusedRef.current; const target = @@ -144,10 +159,12 @@ export function useDialogChrome( // it — and while it held the focus we read that as the user walking away. Then both closing // together left the focus on : ours skipped the restore, and the child's own // `restoreTo` was already detached (passive cleanups run parent-first). Measured in jsdom, - // and it is the ordinary case — an error dialog raised over a dialog. + // and it is the ordinary case — an error dialog raised over a dialog. And one of its own + // menus (see FLOATING): Back with a menu open closed the dialog with the focus on a menu row. heldRef.current = (owner?.contains(active) ?? false) || - (active instanceof Element && active.closest('[aria-modal="true"]') != null); + (active instanceof Element && + (active.closest('[aria-modal="true"]') != null || active.closest(FLOATING) != null)); // Where to put the focus back on a re-mount, though: the PANEL's own last focus. A toast is // gone by then, so restoring onto it would strand the focus exactly as before. if (scopeRef.current?.contains(document.activeElement)) @@ -172,16 +189,28 @@ export function useDialogChrome( // this, a dialog closing on its own (an in-flight mutation's `onSuccess`) under a modal raised // above it pulled the focus out of that standing dialog onto our own trigger, behind its // scrim — where Enter still activated it. + // + // Skipped, the trigger is handed to that standing dialog instead: ITS target was captured + // while the focus was in ours, so it is gone now, and it falls back to the one handed to it. + // A popover still standing (a menu of a dialog that stays open) is not ours to take from + // either, though there is no dialog to hand the trigger to. const owner = ownerAtOpen; const active = document.activeElement; + const target = restoreTo?.isConnected + ? restoreTo + : modalAtOpen + ? handedRestore.get(modalAtOpen) + : undefined; const above = active instanceof Element ? active.closest('[aria-modal="true"]') : null; const standingAbove = above !== null && above.isConnected && !above.contains(owner as Node | null) && !(owner?.contains(above) ?? false); - if (heldRef.current && !standingAbove && restoreTo && restoreTo.isConnected) - restoreTo.focus(); + const standingMenu = active instanceof Element && active.closest(FLOATING)?.isConnected; + if (!heldRef.current || !target?.isConnected) return; + if (standingAbove) handedRestore.set(above, target); + else if (!standingMenu) target.focus(); }; }, [restoreTo, scopeRef, ownerRef, focusScope]); }