[popups] Prevent unwanted flip with capped scrollable content - #5120
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,069.71 ms -35.81 ms(-3.2%) | Renders: 78 (+0)
13 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
403917a to
c633b60
Compare
When a popup's scrollable content is taller than the viewport and capped with the documented max-height: min(<x>, var(--available-height)) pattern, the popup could open on the wrong side: it flipped away from its preferred side even when the capped popup fit there comfortably. flip() runs before size() sets --available-height, so on the first positioning pass that variable is undefined, the min() is invalid, and the whole max-height declaration is dropped. The list is then measured at its full, uncapped content height while flip() decides the side, so it picks the side with more raw space rather than the side where the capped popup actually fits. Seed --available-width: 100vw and --available-height: 100vh on the positioner before size() writes the real values, so the consumer min() stays valid on the first flip() pass and the side is chosen against the capped height. Closes mui#5118
c633b60 to
e269f2b
Compare
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
This is a tight, well-reasoned fix for an unwanted-flip bug: it seeds --available-width: 100vw / --available-height: 100vh in the positioner's React style object so consumer min(<x>, var(--available-height)) rules resolve to a valid length on the first flip() pass, before size() writes the real px values. The mechanism is correct — React's per-property style diff never rewrites a key whose value is constant across renders, so size()'s imperative px values survive — and the change only ever makes the flip pass measure a smaller (capped) popup, which strictly reduces unwanted flips rather than introducing new ones. Nothing is merge-blocking.
Bugs (0)
No findings.
Tests (1)
1. ℹ️ Regression test covers only the min(...) consumer pattern, not the bare var(--available-height) pattern
Location: packages/react/src/combobox/positioner/ComboboxPositioner.test.tsx:90
style={{ maxHeight: 'min(80px, var(--available-height))', overflowY: 'auto' }}The new test exercises the documented min(<x>, var(--available-height)) shape. But the default Select/Menu dropdowns (and several demos) use the bare form max-height: var(--available-height) — see docs/src/components/Select.css:11 and Menu.css:11. The first-pass behavior change differs between the two: for the bare form, the declaration previously dropped entirely (uncapped) and now resolves to 100vh. Since the fix lives in shared useAnchorPositioning, the Combobox test does cover the shared code path, so this is informational rather than a real gap — but a Select/Menu case asserting the bare pattern no longer over-flips would pin the behavior these components actually ship with.
Failure scenario: A future change to how the bare var(--available-height) first-pass value resolves could regress Select/Menu flip behavior without any test failing.
Fix: Optionally add an analogous it.skipIf(isJSDOM) flip test to SelectPositioner (or Menu) using max-height: var(--available-height).
Simplifications (0)
No findings.
Docs (0)
No findings.
Verdict
Approve — correct, minimal fix; the explanatory comment accurately documents the React-reconciliation invariant the fix depends on, and the only note is an optional test-breadth suggestion.
🤖 Review generated with Claude Code
Summary
Fixes #5118.
When a popup's scrollable content is taller than the viewport and capped with the documented
max-height: min(<x>, var(--available-height))pattern, the popup could open on the wrong side: it flipped away from its preferred side even when the capped popup fit there comfortably (e.g. a combobox in the lower half of the viewport opening upward despite plenty of room below).Root cause:
flip()runs beforesize()sets--available-height. On the first positioning pass that variable is undefined, somin(<x>, var(--available-height))resolves to an invalid value and the wholemax-heightdeclaration is dropped. The list is therefore measured at its full, uncapped content height whileflip()decides the side, so it picks the side with more raw space rather than the side where the capped popup actually fits.Fix
Seed
--available-width: 100vwand--available-height: 100vhon the positioner beforesize()writes the real values. This keeps the consumer'smin(<x>, var(--available-height))valid on the firstflip()pass, so the popup is measured at its capped height and the side is chosen correctly.size()overwrites the vars with the real px values immediately, and React's style reconciliation does not clobber those imperative values (the seeded value string is constant across renders, so React never rewrites the property after mount).No changes to the docs demos are required — the existing patterns now behave as intended.
Testing
ComboboxPositioner.test.tsx(fails without the fix: the popup flips totop; passes with it).