[autocomplete] Fix popper rendering issues#48327
Conversation
Bundle size
Deploy previewhttps://deploy-preview-48327--material-ui.netlify.app/ Check out the code infra dashboard for more information about this PR. |
eab775a to
502e987
Compare
502e987 to
65b298a
Compare
65b298a to
eb2b456
Compare
michelengelen
left a comment
There was a problem hiding this comment.
PR #48327 — [Autocomplete] Fix popper rendering issues
Author: mj12albert | Requested reviewers: LukasTy, ZeeshanTamboli, michelengelen, silviuaavram, siriwatknp
Overview
Fixes three independent but related Autocomplete Popper bugs:
- #27670 — Popper width doesn't follow anchor resize → ResizeObserver + force re-render
- #36304 — "No options" flash during exit transition → stale options ref preserved while closing
- #40843 — Empty Popper in freeSolo mode when no matches → hasPopupContent guard
New Utility: useForcedRerendering
Clean, minimal implementation. setState({}) pattern is idiomatic for forced re-renders. 'use client' directive and unstable_ export prefix both follow conventions.
Fix 1: ResizeObserver for width tracking
React.useEffect(() => {
if (!popupOpen || !anchorEl || typeof ResizeObserver === 'undefined') { ... }
let lastWidth = anchorEl.clientWidth;
const observer = new ResizeObserver(() => {
const newWidth = anchorEl.clientWidth;
if (lastWidth !== newWidth) { lastWidth = newWidth; forceRenderOnResize(); }
});
...
}, [popupOpen, anchorEl, forceRenderOnResize]);- Observer only active when popupOpen && anchorEl — no unnecessary overhead when closed
- The lastWidth guard prevents re-renders for height-only changes — good optimization
- SSR-safe (typeof ResizeObserver === 'undefined' check)
- Proper cleanup
Minor concern: Width is read from anchorEl.clientWidth during render (a synchronous layout read). This is fine since re-renders are only triggered on actual width changes, but worth
noting.
Fix 2: Exit transition options preservation
const previousGroupedOptionsRef = React.useRef([]);
const prevPopupOpenRef = React.useRef(false);
if (popupOpen && !prevPopupOpenRef.current) {
previousGroupedOptionsRef.current = []; // reset on new open
}
prevPopupOpenRef.current = popupOpen;
if (popupOpen && groupedOptions.length > 0) {
previousGroupedOptionsRef.current = groupedOptions; // cache while open+populated
}
const renderedOptions = popupOpen ? groupedOptions : previousGroupedOptionsRef.current;The logic is correct and well-commented. The pointerEvents: 'none' guard preventing interaction with stale options is a necessary safety net.
Potential concern with concurrent mode: Ref mutations during render are technically unsafe in concurrent React (renders can be discarded before committing). However, this is an
established pattern in MUI's own usePreviousProps, so it's an accepted trade-off. The logic is also idempotent enough that extra renders don't corrupt state.
Edge case handled well: The test "should not show stale options from a prior session" explicitly covers the scenario where options change between open/close cycles — good catch.
Fix 3: hasPopupContent guard
const hasPopupContent =
renderedOptions.length > 0 || loading || !freeSolo || popperProps.keepMounted === true;Correctly handles all four cases. The comment explains the keepMounted contract clearly. Using renderedOptions (not groupedOptions) here ensures the Popper stays mounted during exit
transitions.
One question: What happens with keepMounted during the exit transition when freeSolo=true and no matches? hasPopupContent is true (due to keepMounted), the Popper renders,
pointerEvents: 'none' is set, but the Paper content is empty. Is that intentional? The Popper would be in the DOM but visually empty — which seems fine since keepMounted is an
explicit consumer opt-in.
Test Coverage
Strong coverage across all three fixes:
- freeSolo no-match → no Popper render
- freeSolo + loading → loading text shown
- keepMounted both object and callback slotProps forms
- Exit transition DOM preservation (browser-only, correctly uses it.skipIf(isJsdom))
- Stale options cleared on re-open with changed options prop
- pointerEvents: 'none' applied when closing
- ResizeObserver observe/disconnect lifecycle
Gaps:
- No test for ResizeObserver actually causing a measurable width update on the Popper element (but this would require a real layout engine)
- No test covering anchorEl changing while popup is open (observer should re-attach)
Summary
This is a well-scoped, clearly motivated fix. The code is thoroughly commented, the three approaches are the right ones for their respective problems, and test coverage is
comprehensive. The most complex part (exit transition options) is well-documented and handles edge cases.
The concurrent-mode ref mutation is a minor theoretical concern but consistent with existing MUI patterns. Ready to approve with no blocking issues.
Oops fixed the refs |
|
Cherry-pick PRs will be created targeting branches: v7.x |
Fixes #27670
Fixes #36304
Fixes #40843
Before: https://stackblitz.com/edit/mgl7u76l?file=src%2FDemo.tsx
After: https://stackblitz.com/edit/mgl7u76l-mtyiy8wd?file=src%2FDemo.tsx
1. Popper width tracks anchor resize (#27670)
Current(master): popper doesn't resize
Fixed: popper resizes with the anchor (the textbox)
2. Options visibility during exit transition (#36304)
Current(master): shows "no options" during the exit transition
Fixed: options remain visible/mounted during the exit transition
3. Empty popper when
freeSolo(#40843)xyzCurrent(master): shows an empty popper, red border still visible
Fixed: popper doesn't render at all when no matches