mirror of
https://github.com/block/buzz.git
synced 2026-10-29 21:41:33 +00:00
fix(desktop): capture the cover drawer's opener once across a StrictMode replay
The drawer captured `document.activeElement` in a layout effect with an
empty dependency list. The app root is wrapped in `React.StrictMode`, which
in development replays every effect as setup -> cleanup -> setup, so by the
second setup the drawer had already focused itself and recorded *itself* as
the element it had covered. On a real close the deferred restore then
focused a node React had detached, which is a silent no-op that drops focus
to `<body>` -- keyboard-stranded, no visible focus ring, in the exact dev
build people use the feature in.
A one-shot ref guards the capture. Refs survive the replay, so the second
setup keeps the opener the first one recorded. The flag is deliberately not
reset in cleanup: the only cleanup it sees before a real close is the
simulated one, which is precisely what it exists to ignore.
The focus-slot claim stays unconditional. Re-claiming on the replayed setup
is what makes the first cleanup's deferred restore stand down, so the
coordinator was already behaving correctly here -- only the capture was
wrong.
The existing focus-handoff tests could not see this because they render
without StrictMode, so the new case renders under it via
`render(..., { reactStrictMode: true })`. Verified to fail at the previous
head (focus lands on `HTMLBodyElement` instead of the opener) and to pass
with the guard.
Signed-off-by: ss-dev-00 <a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
This commit is contained in:
@@ -184,6 +184,14 @@ export function CoverDrawer({
|
||||
const travelPx = prefersReducedMotion ? 0 : COVER_DRAWER_TRAVEL_PX;
|
||||
const drawerRef = React.useRef<HTMLDivElement>(null);
|
||||
const previousFocusRef = React.useRef<HTMLElement | null>(null);
|
||||
/**
|
||||
* Whether the opener has been captured for this drawer instance.
|
||||
*
|
||||
* Distinct from `previousFocusRef.current === null`, which is a legitimate
|
||||
* capture (nothing was focused) and must not be retried. See the capture
|
||||
* effect for why one attempt is all this gets.
|
||||
*/
|
||||
const hasCapturedPreviousFocusRef = React.useRef(false);
|
||||
|
||||
React.useEffect(() => {
|
||||
if (!ownsEscape) return;
|
||||
@@ -214,10 +222,27 @@ export function CoverDrawer({
|
||||
}, [escapeYieldsToContent, onClose, ownsEscape]);
|
||||
|
||||
React.useLayoutEffect(() => {
|
||||
previousFocusRef.current =
|
||||
document.activeElement instanceof HTMLElement
|
||||
? document.activeElement
|
||||
: null;
|
||||
// Capture the opener exactly once per drawer instance.
|
||||
//
|
||||
// `React.StrictMode` replays effects in development as setup → cleanup →
|
||||
// setup, and by that second setup this drawer has already focused itself. An
|
||||
// unconditional read of `document.activeElement` would therefore record the
|
||||
// drawer as its own opener, and a real close would focus a node React has
|
||||
// since detached — leaving focus on `<body>`, keyboard-stranded. Refs survive
|
||||
// the replay, so a one-shot flag is enough. The flag is deliberately not
|
||||
// reset in cleanup: the only cleanup it would see before a real close is the
|
||||
// simulated one, which is exactly what it exists to ignore.
|
||||
//
|
||||
// Re-claiming the focus slot on the replayed setup is correct and stays as
|
||||
// is — that new generation is what makes the first cleanup's deferred
|
||||
// restore stand down.
|
||||
if (!hasCapturedPreviousFocusRef.current) {
|
||||
hasCapturedPreviousFocusRef.current = true;
|
||||
previousFocusRef.current =
|
||||
document.activeElement instanceof HTMLElement
|
||||
? document.activeElement
|
||||
: null;
|
||||
}
|
||||
const focusClaim = claimCoverDrawerFocus();
|
||||
drawerRef.current?.focus({ preventScroll: true });
|
||||
|
||||
|
||||
@@ -143,6 +143,26 @@ test("only the last-opened drawer keeps focus across a chain of replacements", a
|
||||
third.unmount();
|
||||
});
|
||||
|
||||
test("a drawer restores focus to the opener even when StrictMode replays its capture", async () => {
|
||||
// The app root is wrapped in `React.StrictMode`, which in development runs
|
||||
// every effect setup → cleanup → setup. By that second setup the drawer has
|
||||
// already focused itself, so a capture that reads `document.activeElement`
|
||||
// unconditionally records the drawer as its own opener; closing then focuses a
|
||||
// node React has detached and focus falls to `<body>`. Capture has to survive
|
||||
// the replay, which is what the other tests here cannot see because they
|
||||
// render without StrictMode.
|
||||
const { act, drawer, opener, render } = await loadHarness();
|
||||
const view = render(drawer("strict-drawer"), { reactStrictMode: true });
|
||||
assert.equal(activeTestId(), "strict-drawer");
|
||||
|
||||
await act(async () => {
|
||||
view.unmount();
|
||||
});
|
||||
await flushDeferredFocusRestore(act);
|
||||
|
||||
assert.equal(dom.window.document.activeElement, opener);
|
||||
});
|
||||
|
||||
test("a released slot leaves focus where the caller put it", async () => {
|
||||
// The thread view-mode switch: nothing replaces the drawer, but the switch has
|
||||
// already decided where focus belongs, so the drawer's own restore must not
|
||||
|
||||
Reference in New Issue
Block a user