-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(react-combobox): preserve listbox scrollTop across parent re-renders caused by autoSize middleware #36039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
6eabf58
c309dd6
9f3906c
2081a5b
2f27dae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "fix: preserve Combobox listbox scrollTop by stabilizing positioning options to prevent manager recreation on every render (#35731)", | ||
| "packageName": "@fluentui/react-combobox", | ||
| "email": "198982749+Copilot@users.noreply.github.com", | ||
| "dependentChangeType": "patch" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,14 @@ import { resolvePositioningShorthand, usePositioning } from '@fluentui/react-pos | |
| import type { ComboboxBaseProps } from './ComboboxBase.types'; | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the block references
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. |
||
| // to recreate on every render, which disposes and recreates the position manager, triggering | ||
| // autoSize middleware (resetMaxSize) that temporarily removes height constraints from the listbox | ||
| // and resets scrollTop to 0. See: https://github.com/microsoft/fluentui/issues/35731 | ||
| const DEFAULT_FALLBACK_POSITIONS: PositioningShorthandValue[] = ['above', 'after', 'after-top', 'before', 'before-top']; | ||
| const DEFAULT_OFFSET = { crossAxis: 0, mainAxis: 2 } as const; | ||
|
|
||
| export function useComboboxPositioning(props: ComboboxBaseProps): [ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-deprecated | ||
| listboxRef: React.MutableRefObject<any>, | ||
|
|
@@ -13,15 +21,12 @@ export function useComboboxPositioning(props: ComboboxBaseProps): [ | |
| ] { | ||
| const { positioning } = props; | ||
|
|
||
| // Set a default set of fallback positions to try if the dropdown does not fit on screen | ||
| const fallbackPositions: PositioningShorthandValue[] = ['above', 'after', 'after-top', 'before', 'before-top']; | ||
|
|
||
| // popper options | ||
| const popperOptions = { | ||
| position: 'below' as const, | ||
| align: 'start' as const, | ||
| offset: { crossAxis: 0, mainAxis: 2 }, | ||
| fallbackPositions, | ||
| offset: DEFAULT_OFFSET, | ||
| fallbackPositions: DEFAULT_FALLBACK_POSITIONS, | ||
| matchTargetSize: 'width' as const, | ||
| autoSize: true, | ||
| ...resolvePositioningShorthand(positioning), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import * as React from 'react'; | ||
| import type { JSXElement } from '@fluentui/react-components'; | ||
| import { Combobox, makeStyles, Option, useId } from '@fluentui/react-components'; | ||
| import type { ComboboxProps } from '@fluentui/react-components'; | ||
|
|
||
| const useStyles = makeStyles({ | ||
| root: { | ||
| // Stack the label above the field with a gap | ||
| display: 'grid', | ||
| gridTemplateRows: 'repeat(1fr)', | ||
| justifyItems: 'start', | ||
| gap: '2px', | ||
| maxWidth: '400px', | ||
| }, | ||
| }); | ||
|
|
||
| const options = Array.from({ length: 50 }, (_, i) => `Option ${i + 1}`); | ||
|
|
||
| /** | ||
| * Demonstrates that the listbox scrollTop is preserved when a parent component updates | ||
| * state in response to hovering an option (e.g. tracking the currently-hovered item). | ||
| * | ||
| * Previously, each state update caused the Combobox to re-render, which recreated the | ||
| * positioning manager and triggered the autoSize middleware to reset the listbox scrollTop | ||
| * to 0, making the last options in a long list unreachable. | ||
| * See: https://github.com/microsoft/fluentui/issues/35731 | ||
| */ | ||
| export const ScrollStability = (props: Partial<ComboboxProps>): JSXElement => { | ||
| const comboId = useId('combo-scroll-stability'); | ||
| const styles = useStyles(); | ||
| const [hoveredOption, setHoveredOption] = React.useState<string | null>(null); | ||
|
|
||
| const onMouseEnter = React.useCallback((e: React.MouseEvent<HTMLElement>) => { | ||
| const value = (e.currentTarget as HTMLElement).dataset.value ?? null; | ||
| setHoveredOption(value); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <div className={styles.root}> | ||
| <label id={comboId}>Scroll-stable Combobox (50 options)</label> | ||
| <span>Hovered: {hoveredOption ?? 'none'}</span> | ||
| <Combobox aria-labelledby={comboId} placeholder="Select an option" {...props}> | ||
| {options.map(option => ( | ||
| <Option key={option} data-value={option} onMouseEnter={onMouseEnter}> | ||
| {option} | ||
| </Option> | ||
| ))} | ||
| </Combobox> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ScrollStability.parameters = { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'A Combobox with many options where a parent component tracks the hovered option in state. ' + | ||
| 'Scrolling to the bottom of the list and hovering a low option should not cause the listbox ' + | ||
| 'to jump back to the top. This validates the fix for https://github.com/microsoft/fluentui/issues/35731.', | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Charts-DonutChart 2 screenshots
vr-tests-react-components/Menu Converged - submenuIndicator slotted content 1 screenshots
vr-tests-react-components/Positioning 2 screenshots
vr-tests-react-components/ProgressBar converged 2 screenshots