UI: Fix focus-trap broken by ThemeProvider's display:contents#77381
Merged
UI: Fix focus-trap broken by ThemeProvider's display:contents#77381
Conversation
ciampo
added
[Type] Bug
Base UI ≥1.4.0 switched to an internal tabbable implementation that uses checkVisibility() for visibility detection. Per spec, checkVisibility() returns false for display:contents elements (no generated box). Since ThemeProvider wraps popup elements and uses display:contents, every element inside the popup was classified as non-tabbable, completely breaking focus-trap cycling. Override display:contents to display:block on the ThemeProvider div when it directly wraps a popup. The popup is position:fixed, so an extra block wrapper is layout-inert. The rule is unlayered to win over ThemeProvider's unlayered display:contents. Affects: Dialog, AlertDialog, Popover, Tooltip, Select. Made-with: Cursor
ciampo
force-pushed
the
fix/ui-theme-provider-focus-trap
branch
from
512c101 to
5ede7e0
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @atomiks. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 7.75 MB ℹ️ View Unchanged
|
|
Flaky tests detected in c718bb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24522131923
|
Add a ThemeProvider hasLayoutBox escape hatch so overlay popups can opt out of display: contents without duplicating CSS overrides across each popup stylesheet. Made-with: Cursor
Member
Is this a correct / expected behavior on BaseUI's part? 🤔 I guess I'm wondering if this is something that seems reasonably expected behavior for box layout semantics that we'd want to expand the props API to cover this, especially when we didn't need it before. I don't know that either of these alternatives are great, but wondering:
|
Member
|
Like we see in #77191, If we agree that we should try getting it addressed upstream, then any temporary fix here should perhaps not involve adding a new public prop to |
Contributor
Author
|
If you look at the first commit in this PR, would that be a better temporary fix while we bring the concern to Base UI? |
Member
Yes, I would think so. It's good enough for a temporary fix, and has no obvious API implications. We can reassess the approach depending on what the Base UI folks say. |
Revert the ThemeProvider hasLayoutBox follow-up and keep the temporary unlayered popup stylesheet override while we reassess the longer-term fix upstream. Made-with: Cursor
Contributor
Author
|
Updates:
@aduth I would prefer to avoid exposing |
Reference mui/base-ui#4622 and keep note that rules stay outside @layer to override ThemeProvider’s unlayered display: contents. Made-with: Cursor
mirka
approved these changes
Apr 16, 2026
| * This must stay outside the CSS layers to override ThemeProvider's | ||
| * unlayered display: contents. | ||
| */ | ||
| [data-wpds-theme-provider-id]:has(> .popup) { |
Member
There was a problem hiding this comment.
Popover.Popup doesn't receive the .popup class in the unstyled variant. We probably want coverage there too?
Contributor
Author
There was a problem hiding this comment.
Good catch, fixed in c718bb0.
Rather than switching the selector to a data attribute (which would inline the workaround concern into the popup markup), I kept .popup as a structural marker class that is always applied, and moved the visual surface styles to a new .default class. This way the selector still matches regardless of variant, and the visual/structural concerns stay separated in CSS.
|
Sorry for the regression here. I didn't realize We will harden this logic so |
Contributor
Author
|
@atomiks Re. popup tabbability, I think we should be able to hold off with this workaround until 1.5. @mirka re. mui/base-ui#4622, do you think we can hold too? |
Member
|
Yes, I think we can live with the workaround until the next Base UI release. |
Splits Popover's popup class into a structural marker (always applied) and a visual `default` class (applied for non-unstyled variants). This ensures the ThemeProvider display workaround targeting `.popup` matches unstyled popovers too. Addresses review feedback on #77381. Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 21, 2026
Since PR #77381's workaround was Unreleased and is reverted by this PR, the net user-facing change between 0.11.0 and the next release is just the 1.4.0 -> 1.4.1 bump. Drop the standalone Bug Fix line and fold the focus-trap restoration into the existing Internal entry, matching the convention used for prior base-ui bumps (0.9.0, 0.11.0). Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 21, 2026
Since 0.11.0 shipped with the Base UI 1.4.0 regressions, users do see two bug fixes between 0.11.0 and the next release. Document them explicitly: - Bug Fix: restore focus-trap tabbability (cites #77381 + this PR). - Bug Fix: remove transitive date-fns / @date-fns/tz peer dep (resolves #77395). - Internal: keep the @base-ui/react 1.4.0 -> 1.4.1 bump as a generic line. Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 22, 2026
* UI: Update @base-ui/react from 1.4.0 to 1.4.1 This upstream release contains two fixes relevant to @wordpress/ui: - "Fix display: contents tabbability" (mui/base-ui#4642) fixes the focus-trap regression in overlay popups that was previously patched with a CSS workaround in #77381. That workaround is now removed from Dialog, AlertDialog, Popover, Tooltip, and Select styles. - "Mark date-fns peer dependencies as optional" (mui/base-ui#4639) addresses #77395 -- consumers of @wordpress/ui no longer need to install date-fns / @date-fns/tz to satisfy Base UI's peer deps. Made-with: Cursor * UI: Consolidate CHANGELOG into a single Internal entry Since PR #77381's workaround was Unreleased and is reverted by this PR, the net user-facing change between 0.11.0 and the next release is just the 1.4.0 -> 1.4.1 bump. Drop the standalone Bug Fix line and fold the focus-trap restoration into the existing Internal entry, matching the convention used for prior base-ui bumps (0.9.0, 0.11.0). Made-with: Cursor * UI: Split CHANGELOG into user-facing bug fixes + generic Internal bump Since 0.11.0 shipped with the Base UI 1.4.0 regressions, users do see two bug fixes between 0.11.0 and the next release. Document them explicitly: - Bug Fix: restore focus-trap tabbability (cites #77381 + this PR). - Bug Fix: remove transitive date-fns / @date-fns/tz peer dep (resolves #77395). - Internal: keep the @base-ui/react 1.4.0 -> 1.4.1 bump as a generic line. Made-with: Cursor --- Unlinked contributors: Joseph-Mutua. Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org> Co-authored-by: anomiex <bjorsch@git.wordpress.org>
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
4 participants
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.
What?
Fix the focus-trap regression in
@wordpress/uioverlay popups after the Base UI 1.4 update.Why?
PR #77308 updated
@base-ui/reactfrom 1.3.0 to 1.4.0. Base UI 1.4 replaced the externaltabbabledependency with an internal implementation that usescheckVisibility()semantics for visibility detection.ThemeProviderwraps popup elements and usesdisplay: contents, which means it does not generate a layout box. That causes popup descendants to be treated as non-tabbable during the ancestor walk, breaking focus-trap cycling.How?
Temporarily override
display: contentstodisplay: blockon theThemeProviderwrapper when it directly wraps a popup element, using the CSS rule:The rule is unlayered so it wins over the unlayered
display: contentsfromThemeProvider. The popup isposition: fixedor positioned by a positioner, so the extra block wrapper is layout-inert for these overlay cases.Applied to:
DialogAlertDialogPopoverTooltipSelectThis keeps the fix local to the affected overlay styles without expanding the
ThemeProviderAPI, while we reassess whether the underlying visibility detection should be addressed upstream in Base UI.Testing Instructions
Note: this change is tricky to reproduce in JSDOM because it depends on CSS box generation and browser visibility and focusability behavior.
Dialog,AlertDialog,Popover, andSelect.Tabcycles focus through all interactive elements.Shift+Tabcycles focus in reverse.Verification
npm run lint:js -- packages/ui/src/alert-dialog/popup.tsx packages/ui/src/dialog/popup.tsx packages/ui/src/form/primitives/select/popup.tsx packages/ui/src/popover/popup.tsx packages/ui/src/tooltip/popup.tsxnpm run lint:css -- packages/ui/src/dialog/style.module.css packages/ui/src/popover/style.module.css packages/ui/src/tooltip/style.module.css packages/ui/src/utils/css/item-popup.module.cssnpm run test:unit -- packages/ui/src/dialog/test/index.test.tsx packages/ui/src/popover/test/index.test.tsx packages/ui/src/utils/test/use-deprioritized-initial-focus.test.tsx