fix: restore focus in FocusScope containment without scrolling the dialog into view#10339
fix: restore focus in FocusScope containment without scrolling the dialog into view#10339mvanhorn wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Changed to a test to verify that this fixed the issue. Removed the unit test, it was testing implementation which could change in the future.
I verified this in Chrome/Safari/FF with a copy of the stackblitz from the issue as well.
We'll need to discuss if this is always the behaviour we want. It's a pretty wide reaching change. Usually when something is focused, I think it should be scrolled into view. I'm giving an approval right now but it needs more testing across our other components in real browsers with real users.
|
Thanks for the approval and for reworking the test - agree that asserting the behavior over the implementation detail is the better call, and good to know it holds up in Chrome/Safari/FF. On the scope concern: this only touches the two re-focus paths in useFocusContainment - the restore when focus escapes the scope, and the containment correction when focus lands on a node inside the scope. Both are programmatic focus restoration, and they now route through the same focusElement helper the rest of the focus code uses. Initial autofocus and normal tab navigation are untouched and still scroll into view as before. The way I'd frame it: when the scope is correcting focus back to a node that was already focused a moment ago, the scroll-into-view is the surprising part (that's the #10324 jump). For genuinely new focus I fully agree it should scroll. If you'd rather gate this behind an opt-in or narrow it further, happy to - just wanted to flag it isn't changing scroll for all focus, only the containment restore paths. |
Closes #10324
FocusScopecontainment restored focus with a rawelement.focus()in two spots (the tab-out restore path and the underlay-triggered re-entry path), which scrolls the focused element into view. Every other restore path inFocusScopegoes through the scope-awarefocusElementhelper (which restores focus without scrolling and is owner-document safe), so these two calls were inconsistent. Dismissing a scrolled dialog via the underlay therefore jumped the page to the top. This routes both containment restore paths throughfocusElement, matching the rest of the file and preserving scroll position.✅ Pull Request Checklist:
📝 Test Instructions:
Render a Dialog/Modal on a page tall enough to scroll, scroll down, open the dialog, then dismiss it by clicking the underlay. Before this change the page jumped back to the top when the dialog closed (focus restoration scrolled the previously focused trigger into view); after this change the scroll position is preserved. The same applies when tabbing out of and back into a contained scope.
Automated:
yarn jest packages/react-aria/test/focus/FocusScope.test.js(59 tests pass), including new assertions that containment focus restoration goes through the scroll-safefocusSafelypath.🧢 Your Project:
Personal open-source contribution.