[FocusTrap] Fix tab order when children have positive tabIndex#48500
[FocusTrap] Fix tab order when children have positive tabIndex#48500starboyvarun wants to merge 2 commits into
Conversation
…6479) Set sentinelStart tabIndex to 1 instead of 0 so it takes priority over positive-tabIndex children inside the trap. Previously, tabbing past the last element with tabIndex > 0 would reach the sentinel only after all other tabIndex=0 elements — including elements outside the trap — allowing focus to escape.
Deploy previewhttps://deploy-preview-48500--material-ui.netlify.app/ Bundle size
Check out the code infra dashboard for more information about this PR. |
| screen.getByTestId('outside-input').focus(); | ||
| }); |
There was a problem hiding this comment.
I don't think the fix works. The test is wrong since the input is focus()ed directly.
The sentinel approach alone is not enough when children have positive tabIndex. After the last positive-tabIndex element inside the trap, the browser's natural Tab target is the first tabIndex=0 element in document order — which can be outside the trap (e.g. elements rendered before the modal in the DOM). Fix: In loopFocus, intercept forward Tab from any positive-tabIndex element inside the trap and redirect to the next entry in getTabbable() order instead, preventing the brief focus escape. Also fix the test: the previous version called .focus() directly on outside-input, which contain() already handles regardless of the fix. The new test fires a keydown Tab event, which exercises the loopFocus path that the fix adds.
|
You're right on both counts — I've pushed a corrected fix. Why the original test was wrong Calling Why the original fix was wrong Setting The real fix The problem is in the The fix: in The corrected test fires a |
|
Have you manually tested your changes? https://github.com/mui/material-ui/blob/master/CONTRIBUTING.md#trying-changes-on-the-documentation-site |
3 participants
Summary
Fixes #36479.
When children inside
FocusTraphave positivetabIndexvalues (e.g.tabIndex={2}), the browser processes them beforetabIndex={0}elements. ThesentinelStartdiv hadtabIndex={0}, so after tabbing through all positive-tabIndex children the browser would reachtabIndex={0}elements outside the trap before looping back to the sentinel — allowing focus to escape.Fix: Set
sentinelStarttotabIndex={1}so it is reached in tab order immediately after the highest positive-tabIndex child and before anytabIndex={0}elements outside the trap.sentinelEndkeepstabIndex={0}— it is at the end of the DOM so the browser reaches it naturally when tabbing forward throughtabIndex={0}elements.Test plan