Repository navigation
[fix] Modal: trap focus and handle Escape from the commit that activates it - #2879
Open
ilazaridis wants to merge 1 commit into
Open
ilazaridis wants to merge 1 commit into
ilazaridis wants to merge 1 commit into
Conversation
…tes it A modal becomes active, and gets role="dialog", in a render after it shows. Its focus trap and its Escape listener were set up in passive effects, which run a task after that render commits. In between: - A click inside the modal can lose its focus. The previously focused element's blur handler may set state (Pressable does), which makes React flush the pending effects mid focus change. The trap then sees focus outside the modal, focuses its first focusable element, and the browser drops the click's own focus change. - The modal below still has its Escape listener with active=true, so an Escape closes the modal below instead of the new top one. Both effects are now layout effects (SSR-safe, via modules/useLayoutEffect), so they take effect in the same commit as role="dialog".
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c1045b1:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A modal becomes active (
role="dialog") in a render after it shows:onShow→setIsActive(true). That render commitsrole="dialog". But the focus trap (ModalFocusTrap's[active]effect) and the Escape listener (ModalContent) were set up in passive effects, which run a scheduler task after the commit. Two things can go wrong in that window:A click inside the modal loses its focus.
focusouton the element that had it.document.activeElementis<body>at that point.Pressabledoes, for its focus state), React flushes the pending passive effects first.The clicked control never gets focus. If the click opens a nested modal, that modal returns focus to the wrong element when it closes.
An Escape reaches the modal below. When a second modal becomes active, the first still has its Escape listener with
activetrue until the passive effects run. An Escapekeyupin that window closes the modal below, not the new top one.The window is one task, so it's rare by hand. We hit case 1 as an end-to-end test flake: a click on a button in a bottom sheet right after the sheet opened, in Chromium and WebKit, under CI load. Holding React's scheduler for 1 s once the modal gets
role="dialog"reproduces both cases every time. Case 2 needs a browser withoutCloseWatcher, such as shipping Safari.Change
Both effects run as layout effects, through
modules/useLayoutEffect(asTextInputandImagedo, so SSR still usesuseEffect). The trap and the Escape listener now take effect in the same commit that setsrole="dialog", and the modal below gives them up in that same commit. Nothing else changes:canUseDOM.Tests
Two new tests use a probe component rendered after the modals. Its layout effect runs in the commit that makes a modal active, after the modal's own effects and before passive effects, so it sees what an event arriving right after that commit would see:
focus is trapped in the same commit that makes the modal activeescape goes to the new top modal in the same commit that makes it activeBoth fail without the change (focus still outside the modal; the Escape goes to the modal below) and pass with it.
npm run flow: no errors.npm run formatandnpm run lintpass.I ran the unit suites with
jest --config ./configs/jest.config{,.node}.js --runInBand. npm 12 rejectsnpm run unit --runInBand("Unknown cli flag: --runInBand").We have run the same change in production as a patch on 0.21.3, with React 19.2. The repro above fails 10/10 without it and passes 10/10 with it, in Chromium and WebKit. Our full end-to-end suite, which covers focus trapping, Tab cycling, Escape, focus return and nested modals, also passes with it.
Related: #2862 also touches
ModalContent's Escape handling (one modal per key press). It's independent of this timing change.