Repository navigation
[fix] Modal: wrap focus backwards to the last element, not its container - #2908
Open
ilazaridis wants to merge 1 commit into
Open
ilazaridis wants to merge 1 commit into
ilazaridis wants to merge 1 commit into
Conversation
|
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 1159ad7:
|
ilazaridis
force-pushed
the
fix/modal-focus-wrap-backwards
branch
from
October 15, 2026 07:06
e5059f5 to
6115229
Compare
Shift+Tab from a modal's first element wraps focus with focusLastDescendant, which tried each node before its children. Chrome and Firefox can focus a scroll container, so the wrap stopped on a ScrollView around the modal's last elements instead of the last one. Shift+Tab from there moved focus back to the first element, and the elements in the ScrollView were never reached backwards. focusLastDescendant now tries a node's children before the node, which is reverse document order.
ilazaridis
force-pushed
the
fix/modal-focus-wrap-backwards
branch
from
October 15, 2026 07:07
6115229 to
1159ad7
Compare
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.
When focus leaves a modal's first focusable element backwards (Shift+Tab onto the leading focus bracket), the focus trap wraps it with
focusLastDescendant. That function tried each node before its children, so it didn't find the last focusable element in document order: any focusable ancestor of that element came first.Chrome and Firefox let
focus()land on a scroll container. So in a modal whose last elements sit in aScrollView, the wrap focuses theScrollView's<div>rather than the last element in it. Shift+Tab from that<div>moves to the previous tab stop, the modal's first element, and the trap's next wrap lands on the<div>again. Focus bounces between the two, and the elements inside theScrollVieware never reached backwards. Tab forwards is fine:focusFirstDescendanttries a node before its children, which is already document order.We hit this with a bottom sheet (
Modalwith a backdropPressablefirst, then aScrollViewholding a text field and a Done button). In Chrome (Playwright, Pixel 7 emulation) and Firefox 157, Shift+Tab from the backdrop alternated between the backdrop and theScrollView.Change
focusLastDescendanttries a node's descendants before the node itself: reverse document order. The set of nodes it tries is the same; only the order changes. A modal without a focusable container around its last element wraps to the same element as before.Tests
New test:
focus wraps backwards to the last element inside a focusable container. A<div tabIndex={-1}>stands in for the scroll container (jsdom can't focus a scroll container without atabIndex). It fails on master (focus goes to the<div>) and passes with the change. The existing Modal tests pass unchanged.npm run flow: no errors.npm run formatandnpm run lintpass.jest --config ./configs/jest.config{,.node}.js --runInBand).We run the same change as a patch on 0.21.3, with React 19.2, together with #2879 and #2904. In our end-to-end suite, a Chromium test that Tabs and Shift+Tabs through a sheet's controls fails without the change and passes with it, and the rest of the suite passes in Chromium and WebKit. In Firefox 157, Shift+Tab reaches every control too (Firefox also stops on the scroll container itself, both ways, as a tab stop of its own).
This is independent of #2879 and #2904, and any of them can land first.