Conversation
snowystinger
left a comment
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); |
There was a problem hiding this comment.
why are we asserting this? what does it tell us?
There was a problem hiding this comment.
It tells us we are looking at slop. This test already passes on main without changes.
There was a problem hiding this comment.
Thanks for checking that, I had a feeling that was the case but hadn't pulled it down yet. I appreciate the help, hopefully they'll look back over the PR and get it fixed up.
I'm not actually sure there is a bug yet, I'm unclear why they are using FocusScope directly, and not using Modal. #10593 (comment)
There was a problem hiding this comment.
You're right that the previous unit test did not establish the regression independently: it checked Shift+Tab only after forward Tab and also asserted the tree size. I removed that assertion and gave each direction a fresh fixture. The exiting sibling now has contain={false} while it remains mounted, matching the reporter's clarification and the exiting-overlay behavior you pointed out.
I checked both variants against the source without the production change. Forward Tab passes, but the independent reverse case moves focus to Outside in Jest and Chromium, Firefox, and WebKit. The existing fix makes both directions pass (63 unit tests and 6 browser cases). I narrowed the PR summary accordingly: this repro demonstrates reverse containment loss, not an independent reproduction of the reported forward skipping.
I also reran the full suites and recorded their local failures in the PR body. The TokenField shortcut and stale-Browserslist SSR failures persist without the production change; I have not claimed a full-suite all-green result.
|
The updated unit, browser, lint, and type jobs passed. All four SSR jobs fail for the same reason: the pinned Browserslist/caniuse-lite data is reported as seven months old, and the test setup converts that console warning into a thrown error. For example, https://circleci.com/gh/adobe/react-spectrum/700986 reports 60 failed suites and 60 failed / 14 passed tests; the same warning appears in jobs 700984, 700991, and 700978. I reproduced the same warning failure in a local Flex SSR case with the production change removed, and restored the source afterwards. I have left the warning checks and test thresholds intact. This appears to need a shared browser-data dependency refresh rather than another FocusScope change; could a maintainer confirm the preferred way to handle that dependency update? |
|
Merged main ( The full Jest run passes the FocusScope suite, and all six FocusScope browser cases pass in Chromium, Firefox and WebKit. Full Jest reports 8,074 passed / 16 existing Windows failures / 16 skipped; the complete browser suite reports 417 passed / 6 current-main TokenField/Chat failures / 69 skipped. Type-check, oxlint, package checks and constraints pass; full checkout formatting remains a local failure. The PR test plan records these current results without expanding the earlier reproduction claim: the independent baseline failure is reverse containment loss, not reported forward skipping. |
Closes #10593
Summary
When an exiting portaled overlay opens a sibling overlay, removing the old scope can discard the containment owner of the surviving scope. The independent reverse-Tab regression reproduces focus moving to Outside after the closing scope stops containing focus and unmounts.
The cleanup resets the active scope only when the scope being removed is itself active. A still-mounted active descendant is preserved while tree removal reparents it; whole-subtree unmount behavior stays covered.
The unit regressions now use separate fresh fixtures for forward and reverse navigation, remove the non-behavioral tree-size assertion, and release containment on the exiting scope. Against the source without the production change, forward navigation passes but Shift+Tab escapes in Jest and Chromium, Firefox, and WebKit. With the fix, both directions pass. This minimal repro establishes reverse containment loss; it does not independently reproduce the reported forward skipping.
✅ 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.Current local validation after merging main
4cea3b10without rewriting history:BROWSERSLIST_IGNORE_OLD_DATAunset. Main includes upstream fix: browser tests failing due to browsers list data old #10688's warning-matcher correction; this PR adds no suppression or duplicate fix.Commands:
yarn test --maxWorkers=2,yarn test:ssr,yarn lint, andyarn exec vitest run --config=vitest.browser.config.ts --maxWorkers=1using existing browser binaries. No unrelated assertions or rules were disabled. Chromatic is reserved for maintainers and was not run. Screen readers, mobile devices, themes and zoom were not tested locally; this does not establish complete accessibility conformance.🧢 Your Project:
Open-source contribution by
dvd233; no company project.