fix(ui): a closing dialog's focus restore reaches the dialog left standing above it, and survives a menu
A dialog closing on its own under a standing modal skipped its restore, and the standing modal's own target was a control inside the closed one: closing it left the focus on <body>. The skipped target is now handed to the standing dialog, which falls back to it. Focus on a row of one of the dialog's own menus (the popover engine portals them out of the dialog root) read as the user walking away, so Back with a menu open left the focus on <body>. A focus in a popover portal now counts as held, and one still standing is not taken from.
This commit is contained in:
@@ -23,7 +23,7 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} }));
|
|||||||
import Modal from "./Modal";
|
import Modal from "./Modal";
|
||||||
import { clearAll, notify } from "../../lib/notifications";
|
import { clearAll, notify } from "../../lib/notifications";
|
||||||
import { pushLayer, removeLayer, useLayer } from "../../lib/layerStack";
|
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.
|
// Fire a keydown on window (where Modal installs its handler), inside act.
|
||||||
const press = (key: string, shiftKey = false) =>
|
const press = (key: string, shiftKey = false) =>
|
||||||
@@ -311,6 +311,70 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => {
|
|||||||
document.body.removeChild(trigger);
|
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 <body>.
|
||||||
|
// 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 && (
|
||||||
|
<Modal initialFocus="close" title="lower" onClose={() => {}}>
|
||||||
|
<button onClick={() => setUpper(true)}>open upper</button>
|
||||||
|
<button onClick={() => setLower(false)}>close lower</button>
|
||||||
|
</Modal>
|
||||||
|
)}
|
||||||
|
{upper && (
|
||||||
|
<Modal initialFocus="close" title="upper" onClose={() => {}}>
|
||||||
|
<button onClick={() => setUpper(false)}>close upper</button>
|
||||||
|
</Modal>
|
||||||
|
)}
|
||||||
|
</>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
render(<Siblings />);
|
||||||
|
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 <body>. 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(
|
||||||
|
<Modal initialFocus="close" title="t" onClose={() => {}}>
|
||||||
|
<Body />
|
||||||
|
{createPortal(<button>menu-row</button>, portal)}
|
||||||
|
</Modal>,
|
||||||
|
);
|
||||||
|
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", () => {
|
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 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.
|
// the user deliberately put it is the rudest thing a closing dialog can do.
|
||||||
|
|||||||
@@ -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<Element, HTMLElement>();
|
||||||
|
|
||||||
/** The rest of the contract: lock the page behind, open focused, hand the focus back on close.
|
/** 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
|
* `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
|
* 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
|
// 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.
|
// `contains` below), but the cleanup should not depend on a ref React has already let go of.
|
||||||
const ownerAtOpen = ownerRef?.current ?? scope;
|
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)) {
|
if (scope && !scope.contains(document.activeElement)) {
|
||||||
const back = focusedRef.current;
|
const back = focusedRef.current;
|
||||||
const target =
|
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
|
// it — and while it held the focus we read that as the user walking away. Then both closing
|
||||||
// together left the focus on <body>: ours skipped the restore, and the child's own
|
// together left the focus on <body>: ours skipped the restore, and the child's own
|
||||||
// `restoreTo` was already detached (passive cleanups run parent-first). Measured in jsdom,
|
// `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 =
|
heldRef.current =
|
||||||
(owner?.contains(active) ?? false) ||
|
(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
|
// 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.
|
// gone by then, so restoring onto it would strand the focus exactly as before.
|
||||||
if (scopeRef.current?.contains(document.activeElement))
|
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
|
// 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
|
// above it pulled the focus out of that standing dialog onto our own trigger, behind its
|
||||||
// scrim — where Enter still activated it.
|
// 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 owner = ownerAtOpen;
|
||||||
const active = document.activeElement;
|
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 above = active instanceof Element ? active.closest('[aria-modal="true"]') : null;
|
||||||
const standingAbove =
|
const standingAbove =
|
||||||
above !== null &&
|
above !== null &&
|
||||||
above.isConnected &&
|
above.isConnected &&
|
||||||
!above.contains(owner as Node | null) &&
|
!above.contains(owner as Node | null) &&
|
||||||
!(owner?.contains(above) ?? false);
|
!(owner?.contains(above) ?? false);
|
||||||
if (heldRef.current && !standingAbove && restoreTo && restoreTo.isConnected)
|
const standingMenu = active instanceof Element && active.closest(FLOATING)?.isConnected;
|
||||||
restoreTo.focus();
|
if (!heldRef.current || !target?.isConnected) return;
|
||||||
|
if (standingAbove) handedRestore.set(above, target);
|
||||||
|
else if (!standingMenu) target.focus();
|
||||||
};
|
};
|
||||||
}, [restoreTo, scopeRef, ownerRef, focusScope]);
|
}, [restoreTo, scopeRef, ownerRef, focusScope]);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user