[dialog][alert dialog][popover] Fix focus management for children with positive tabindex#3846
[dialog][alert dialog][popover] Fix focus management for children with positive tabindex#3846michaldudak wants to merge 3 commits intomui:masterfrom
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
Greptile SummaryFixes focus trap breaking when modal dialogs or popovers contain elements with
Confidence Score: 5/5
Important Files Changed
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| return ( | ||
| <div> | ||
| <TestPopover | ||
| rootProps={{ modal: 'trap-focus' }} |
There was a problem hiding this comment.
Is it expected that for popover this doesn't work with modal={true} like dialog? https://stackblitz.com/edit/nht123vz?file=src%2FApp.tsx
They are documented the same: true: user interaction is limited to the popover
There was a problem hiding this comment.
Yes, it's deliberate.
2231a7a to
3d531d0
Compare
| // Positive tabIndex content can jump ahead of the focus guards in the browser's | ||
| // global tab order, so manually cycle within the floating element to preserve | ||
| // the focus trap. |
There was a problem hiding this comment.
Does Radix handle this? imo if it doesn't, let's leave it off. It adds extra cost to handle an edge case that's bad to begin with (you should never use positive tab indices)
There was a problem hiding this comment.
No, they explicitly ignore it:
https://github.com/radix-ui/primitives/blob/main/packages/react/focus-scope/src/focus-scope.tsx#L259
I suppose you're right, the use case is quite exotic anyway.
Fixed modal focus trapping when Dialogs or Popovers contain positive tabIndex elements by adding manual Tab cycling in FloatingFocusManager.
Fixes #3844