fix(react-combobox): preserve listbox scrollTop across parent re-renders caused by autoSize middleware#36039
fix(react-combobox): preserve listbox scrollTop across parent re-renders caused by autoSize middleware#36039
Conversation
Co-authored-by: tudorpopams <97875118+tudorpopams@users.noreply.github.com>
…lTop on re-render (#35731) Agent-Logs-Url: https://github.com/microsoft/fluentui/sessions/f1f92f2a-b1f9-42d9-a861-32b7b3d5f1d7 Co-authored-by: tudorpopams <97875118+tudorpopams@users.noreply.github.com>
…n story Agent-Logs-Url: https://github.com/microsoft/fluentui/sessions/f1f92f2a-b1f9-42d9-a861-32b7b3d5f1d7 Co-authored-by: tudorpopams <97875118+tudorpopams@users.noreply.github.com>
| import type * as React from 'react'; | ||
|
|
||
| // Stable module-level constants prevent new object/array references on every render. | ||
| // Without these constants, new references would cause usePositioningConfigFn's useCallback |
There was a problem hiding this comment.
Nit: the block references usePositioningConfigFn's useCallback but that's an internal implementation detail of @fluentui/react-positioning — reviewers unfamiliar with that hook have to go spelunking to verify the causal chain. Could you either:
- Trim the comment to the observable effect — e.g. "Module-level constants so
popperOptionshas stable references across renders, preventing the position manager from being torn down and rebuilt on each parent re-render (which would resetscrollTopvia autoSize'sresetMaxSize)." — and drop theuseCallbackreference, or - Keep the detail but point at the specific file (e.g.
@fluentui/react-positioning/src/usePositioningConfigFn.tsor wherever theuseCallbacklives) so readers can trace it without guessing.
Not blocking — the fix itself is correct and the cause is well-chosen. Just a readability thing for the next person who hits this code.
PR Review: #36039 — fix(react-combobox): preserve listbox scrollTop across parent re-rendersAuthor: app/copilot-swe-agent Confidence Score: 100/100Fix went deeper than the original hypothesis: instead of the save/restore-scrollTop pattern the Copilot brief suggested, this traces the actual root cause — FindingsBlockers (must fix before merge)none Warnings (should address)none Info (consider)
Category Breakdown
RecommendationAPPROVE The fix is smaller than I expected when I wrote the brief — and better. Shipping is safe; the inline comment-block nit is a polish pass that can land with the PR or in a follow-up. The new story plus the manual Storybook validation cover the regression surface that matters, and the root-cause analysis in the PR body is unusually precise for this kind of cross-render interaction bug. Posted via the |
📊 Bundle size reportUnchanged fixtures
|
|
Pull request demo site: URL |
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Charts-DonutChart 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Charts-DonutChart.Dynamic - Dark Mode.default.chromium.png | 7530 | Changed |
| vr-tests-react-components/Charts-DonutChart.Dynamic - RTL.default.chromium.png | 5570 | Changed |
vr-tests-react-components/Menu Converged - submenuIndicator slotted content 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Menu Converged - submenuIndicator slotted content.default.submenus open.chromium.png | 413 | Changed |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 500 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 614 | Changed |
vr-tests-react-components/ProgressBar converged 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness - Dark Mode.default.chromium.png | 34 | Changed |
| vr-tests-react-components/ProgressBar converged.Indeterminate + thickness.default.chromium.png | 40 | Changed |

Problem
When a parent component updates state in response to hovering a Combobox option (e.g. tracking the hovered item), every state update caused the positioning manager to be fully disposed and recreated. This triggered the
resetMaxSizeautoSize middleware, which temporarily removesheight/max-heightfrom the listbox — causing the browser to resetscrollTopto 0. The result: items near the bottom of a long list were unreachable because scrolling down and hovering any item immediately jumped the list back to the top.Root Cause
In
useComboboxPositioning,offsetandfallbackPositionswere created as new object/array literals on every render:These flow into
usePositioningConfigFn'suseCallbackas dependencies. New references → new callback →updatePositionManagerrecreated → position manager disposed and rebuilt →updatePositioncalled →resetMaxSizestrips height constraints →scrollTopresets.Fix
useComboboxPositioning.ts: PromoteoffsetandfallbackPositionsto module-level constants (DEFAULT_OFFSET,DEFAULT_FALLBACK_POSITIONS). Stable references mean the position manager is only recreated when positioning actually changes (mount, window resize/scroll, genuine container dimension change) — not on every parent re-render.ComboboxScrollStability: 50-option Combobox where a parent tracks the hovered option in state — the exact scenario from the bug report — to document the expected behavior.