From fbc0cbf39db11bc065d65f9895d6e950bdf5a1e1 Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Mon, 24 Aug 2026 13:52:11 -0700 Subject: [PATCH] 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 `` -- 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 Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../src/features/channels/ui/CoverDrawer.tsx | 33 ++++++++++++++++--- .../ui/CoverDrawerFocusHandoff.test.mjs | 20 +++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/desktop/src/features/channels/ui/CoverDrawer.tsx b/desktop/src/features/channels/ui/CoverDrawer.tsx index 02c242d691..4f775f89bc 100644 --- a/desktop/src/features/channels/ui/CoverDrawer.tsx +++ b/desktop/src/features/channels/ui/CoverDrawer.tsx @@ -184,6 +184,14 @@ export function CoverDrawer({ const travelPx = prefersReducedMotion ? 0 : COVER_DRAWER_TRAVEL_PX; const drawerRef = React.useRef(null); const previousFocusRef = React.useRef(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 ``, 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 }); diff --git a/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs index 74b94f8a9d..ce07ddc7e0 100644 --- a/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs +++ b/desktop/src/features/channels/ui/CoverDrawerFocusHandoff.test.mjs @@ -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 ``. 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