Skip to content

[dialog][alert dialog][popover] Fix focus management for children with positive tabindex#3846

Closed
michaldudak wants to merge 3 commits intomui:masterfrom
michaldudak:dialog-broken-tabindex
Closed

[dialog][alert dialog][popover] Fix focus management for children with positive tabindex#3846
michaldudak wants to merge 3 commits intomui:masterfrom
michaldudak:dialog-broken-tabindex

Conversation

@michaldudak
Copy link
Copy Markdown
Member

Fixed modal focus trapping when Dialogs or Popovers contain positive tabIndex elements by adding manual Tab cycling in FloatingFocusManager.

Fixes #3844

@michaldudak michaldudak requested a review from atomiks as a code owner January 23, 2026 10:29
@michaldudak michaldudak added the component: alert dialog Changes related to the alert dialog component. label Jan 23, 2026
@michaldudak michaldudak added component: dialog Changes related to the dialog component. component: popover Changes related to the popover component. labels Jan 23, 2026
@michaldudak michaldudak changed the title [popups] Fix focus management for children with positive tabindex [dialog][alert dialog][popover] Fix focus management for children with positive tabindex Jan 23, 2026
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jan 23, 2026

commit: 3d531d0

@mui-bot
Copy link
Copy Markdown

mui-bot commented Jan 23, 2026

Bundle size report

Bundle Parsed size Gzip size
@base-ui/react 🔺+206B(+0.05%) 🔺+104B(+0.08%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jan 23, 2026

Greptile Summary

Fixes focus trap breaking when modal dialogs or popovers contain elements with tabIndex greater than 0. The fix adds manual Tab key cycling in FloatingFocusManager to prevent positive tabIndex elements from jumping ahead of focus guards in the browser's global tab order.

  • Detects when tabbable content includes positive tabIndex values
  • Manually cycles focus forward/backward through tabbable elements
  • Preserves the focus trap by preventing default Tab behavior
  • Comprehensive test coverage for both Dialog and FloatingFocusManager

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is clean, well-tested, and solves a specific focus management bug. The logic correctly handles both forward and backward Tab navigation with proper edge case handling. Tests verify the fix works for both Dialog and FloatingFocusManager components.
  • No files require special attention

Important Files Changed

Filename Overview
packages/react/src/floating-ui-react/components/FloatingFocusManager.tsx Added manual Tab cycling logic to trap focus when positive tabIndex elements are present
packages/react/src/floating-ui-react/components/FloatingFocusManager.test.tsx Added test coverage for focus trapping with positive tabIndex elements
packages/react/src/dialog/root/DialogRoot.test.tsx Added Dialog-specific test for focus trap with positive tabIndex elements

@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 23, 2026

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 3d531d0
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/698a03e2cc700c00085453bc
😎 Deploy Preview https://deploy-preview-3846--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

return (
<div>
<TestPopover
rootProps={{ modal: 'trap-focus' }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's deliberate.

@michaldudak michaldudak force-pushed the dialog-broken-tabindex branch from 2231a7a to 3d531d0 Compare February 9, 2026 15:57
Comment on lines +378 to +380
// 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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: alert dialog Changes related to the alert dialog component. component: dialog Changes related to the dialog component. component: popover Changes related to the popover component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Dialogs] focus trap breaks when dialog contains an element with tabIndex greater than 0

4 participants