fix: preserve active FocusScope when sibling unmounts - #10596
Open
dvd233 wants to merge 1 commit into
Open
Conversation
snowystinger
left a comment
Member
There was a problem hiding this comment.
This left the surviving overlay without the correct containment owner, so keyboard navigation could skip a control or escape the overlay
Can you explain this in a little more detail? why would this result in the skip or escape? what was the flow of logic that the incorrect containment owner resulted in?
| rerender(<Test showSecond />); | ||
| expect(document.activeElement).toBe(getByTestId('second1')); | ||
|
|
||
| expect(focusScopeTree.size).toBe(3); |
Member
There was a problem hiding this comment.
why are we asserting this? what does it tell us?
Contributor
There was a problem hiding this comment.
It tells us we are looking at slop. This test already passes on main without changes.
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.
Closes #10593
Summary
When one portaled overlay opens another before the first finishes closing, the active
FocusScopetree can temporarily reparent the new scope beneath the old one. The old cleanup treated an active descendant as a reason to resetactiveScope, even when that descendant remained mounted. This left the surviving overlay without the correct containment owner, so keyboard navigation could skip a control or escape the overlay.This change resets
activeScopeonly when the scope being removed is itself active. Removing an ancestor now preserves a still-mounted active descendant whileremoveTreeNodereparents it. The existing nested whole-subtree unmount behavior remains covered and unchanged.Regression coverage includes:
✅ Pull Request Checklist:
AGENTS.md,CLAUDE.md, and the relevant files underdocs/contributing/.📝 Test Instructions:
yarn jest packages/react-aria/test/focus/FocusScope.test.js --runInBand.yarn vitest run --config=vitest.browser.config.ts packages/react-aria/test/focus/FocusScope.browser.test.tsx.Additional local validation:
yarn test:ssr: 60 suites / 74 tests passed.yarn test: 373 suites and 7,989 tests passed; four unrelated Windows/locale/codemod suites failed (locale fixture path separator mismatch, empty generated locale sets, and a codemod package-manager e2e expectation).yarn test:browser: the target test passed in Chromium, Firefox, and WebKit; the full run later ended on an unrelated Firefox browser-session connection timeout while runningListBox.browser.test.tsx.yarn lint: type-check, oxlint, package lint, and Yarn constraints passed. The repository-wide format check reports checkout-wide CRLF differences on Windows;yarn format:checkpasses for all three changed files.🧢 Your Project:
Open-source contribution by
dvd233; no company project.