Skip to content

[popups] Apply data-base-ui-inert to highest level node#3955

Merged
atomiks merged 1 commit intomui:masterfrom
atomiks:codex/mark-others-simplify-tests
Mar 3, 2026
Merged

[popups] Apply data-base-ui-inert to highest level node#3955
atomiks merged 1 commit intomui:masterfrom
atomiks:codex/mark-others-simplify-tests

Conversation

@atomiks
Copy link
Copy Markdown
Contributor

@atomiks atomiks commented Feb 3, 2026 •
edited
Loading

Fixes #3950

This should avoid interfering with nodes inside editors while continuing to support 3rd party extension elements (as it's assumed they're portaled to <body>), and improves perf as well since a smaller number of elements are mutated (with the common portaled popup case)

@atomiks atomiks added the scope: all components Widespread work has an impact on almost all components. label Feb 3, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 3, 2026 •
edited
Loading

✅ Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit ac8a256
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69a764c4d35d450008b53bee
😎 Deploy Preview https://deploy-preview-3955--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.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Feb 3, 2026 •
edited
Loading

commit: 0c17f0f

@mui-bot
Copy link
Copy Markdown

mui-bot commented Feb 3, 2026 •
edited
Loading

Bundle size report

Bundle Parsed size Gzip size
@base-ui/react 🔺+386B(+0.08%) 🔺+127B(+0.09%)

Details of bundle changes

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

@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch from d971b13 to d3a5b09 Compare February 3, 2026 22:55
atomiks
atomiks commented Feb 3, 2026
View reviewed changes
Comment thread packages/react/src/floating-ui-react/utils/markOthers.test.ts
Comment on lines +1 to +2
import { markOthers } from './markOthers';

Copy link
Copy Markdown
Contributor Author

@atomiks atomiks Feb 3, 2026

Choose a reason for hiding this comment

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

This test is ported from @floating-ui/react

@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch 4 times, most recently from f29bef5 to c0597d6 Compare February 3, 2026 23:45
@atomiks atomiks mentioned this pull request Feb 3, 2026
Closed
@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch 3 times, most recently from 1aee985 to 7a630fc Compare February 4, 2026 00:01
@atomiks atomiks marked this pull request as ready for review February 4, 2026 00:05
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR optimizes the data-base-ui-inert marker application by applying it to the highest-level DOM nodes outside the popup instead of recursively marking all descendants. The key change separates the marker logic from the aria-hidden logic in FloatingFocusManager, allowing them to target different element sets.

Key improvements:

  • Performance: Fewer DOM mutations by marking parent nodes instead of all descendants
  • Compatibility: Prevents interference with nested elements inside editors (rich text editors, code editors, etc.) that may have their own complex DOM structures
  • Maintains support for 3rd party extension elements portaled to <body>

Technical changes:

  • Refactored markOthers.ts to separate marker and control attribute logic
  • Added mark and markerIgnoreElements options to MarkOthersOptions
  • Split markOthers call in FloatingFocusManager into two: ariaHiddenCleanup and markerCleanup
  • Added comprehensive test coverage for the new behavior

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is well-tested with comprehensive test coverage, addresses a real performance and compatibility issue, and the implementation is clean with proper separation of concerns between marker and aria-hidden logic
  • No files require special attention

Important Files Changed

Filename Overview
packages/react/src/floating-ui-react/utils/markOthers.ts Refactored to apply data-base-ui-inert to highest-level nodes instead of all descendants, improving performance and avoiding interference with nested editors
packages/react/src/floating-ui-react/components/FloatingFocusManager.tsx Split markOthers into two separate calls: one for aria-hidden with full inside elements, another for marker with common elements only

@atomiks atomiks mentioned this pull request Feb 4, 2026
Open
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 10, 2026
@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch from 7a630fc to da07bf6 Compare February 10, 2026 13:00
@github-actions github-actions Bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Feb 10, 2026
@mech
Copy link
Copy Markdown

mech commented Feb 27, 2026

Seems to have this on my AlertDialog (both 1.1.1 and 1.1.2) and Autocomplete (only 1.12) happening.

CleanShot 2026-02-27 at 09 11 20@2x

At a lost what to do.

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented Feb 27, 2026

@mech this is unrelated to this PR. It can happen in some scenarios like this: #4205. If you have a reproducible example, make an issue so we can look into it and fix 🙏

@mech
Copy link
Copy Markdown

mech commented Feb 27, 2026 •
edited
Loading

Wow, you are right. Once I remove my <ScrollArea> inside both my <AlertDialog> and <Autocomplete>, it is gone. Something's wrong with the <ScrollArea> I believe. Will investigate more on my side.

CleanShot 2026-02-27 at 10 49 34@2x

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented Feb 27, 2026

@mech it's an edge casey timing bug with focus trap and the scroll area tabIndex + measurements. It's on our side (see #4220). To fix it, just add tabIndex={0} to the Viewport part in the meantime as a workaround.

@mech
Copy link
Copy Markdown

mech commented Feb 27, 2026

Tks I have indeed add the tabIndex as a temp fix 👏

@atomiks
Copy link
Copy Markdown
Contributor Author

atomiks commented Mar 3, 2026 •
edited
Loading

Codex Review

Overview

This branch refactors markOthers to separate marker behavior from control-attribute behavior, updates FloatingFocusManager integration, and adds regression coverage for overlapping cleanup, shadow-root host unwrapping, and marker targeting around reference/focus-guard structures. The intended behavior remains: top-level marker application with preserved aria-hidden/inert cleanup correctness.

Findings (None)

No blocking issues found in this patch.

Confidence: 4/5

High confidence in functional correctness from full-branch review and test coverage; moderate uncertainty remains on end-to-end perf due normal benchmark variance.

Notes

  • Latest reviewed commit: f57808e0d.
  • Validation reruns:
    • pnpm test:jsdom markOthers FloatingFocusManager --no-watch
    • pnpm test:chromium markOthers --no-watch

Final stress review (Popover.Root open+close, modal=true, 3000 top-level outside nodes), two benchmark passes averaged:

Metric Master (94474b438) Branch (f57808e0d) Delta
marker nodes mean 3304.0 3002.0 -9.1%
aria-hidden nodes mean 1.0 1.0 0.0%
open+close mean (ms) 270.6882 252.0050 -6.9%
open+close median (ms) 267.2741 248.4421 -7.0%
open+close p95 (ms) 287.4924 276.5797 -3.8%

Regular hero-like Popover setup rerun (single docs-style hero popover + surrounding demo DOM), 5 alternating passes per side averaged:

Metric Master (94474b438) Branch (f57808e0d) Delta
marker nodes mean 12.0 2.0 -83.3%
aria-hidden nodes mean 2.0 2.0 0.0%
open+close mean (ms) 16.6264 15.5860 -6.3%
open+close median (ms) 16.2191 15.1946 -6.3%
open+close p95 (ms) 18.8188 17.4449 -7.3%

Rerun variance note (hero-like): branch mean stddev 0.42ms vs master 1.35ms; this run had larger variance on master.

@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch 5 times, most recently from 2096bd6 to f57808e Compare March 3, 2026 22:28
@atomiks atomiks force-pushed the codex/mark-others-simplify-tests branch from f57808e to ac8a256 Compare March 3, 2026 22:46
@atomiks atomiks merged commit d151b18 into mui:master Mar 3, 2026
21 of 22 checks passed
@atomiks atomiks deleted the codex/mark-others-simplify-tests branch March 3, 2026 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colmtuite colmtuite Awaiting requested review from colmtuite colmtuite is a code owner

@michaldudak michaldudak Awaiting requested review from michaldudak michaldudak is a code owner

Assignees

No one assigned

Labels

scope: all components Widespread work has an impact on almost all components.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adding data-base-ui-inert to DOM elements when floating elements open makes component usage restrictive and unpredictable

3 participants

@atomiks @mui-bot @mech