-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: runaway auto-scroll loop in hover-highlighted menus - #6078
Conversation
When a scrollable menu's highlight follows the mouse, scrolling the
highlighted item into view moves the next item under the stationary
pointer, which re-highlights and scrolls again — an endless loop with
no user input.
Two instances of the same pattern:
- Chat/useTriggerMenu.tsx: hover-driven highlight changes now skip one
run of the scroll-into-view effect; keyboard navigation still scrolls.
- DropdownMenu/menuItemHover.ts: focusMenuItemOnHover now focuses with
{preventScroll: true}, covering DropdownMenu, ContextMenu, and
BreadcrumbMenu items via the shared helper.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Hi @faga295!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!
@cixzhang
cixzhang
left a comment
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.
Thanks, the changed DropdownMenu and Chat paths now keep hover stationary while keyboard navigation still scrolls in Chromium. People using Selector, MultiSelector, Typeahead, or DateTimeInput can still have hover move the list because those paths pair hover-updated highlights with unconditional scrollIntoView; Selector moved 376px in the same 25-item probe. Please either cover those paths through their shared highlight owner, or narrow #6077 and track the remaining instances separately. Also add the patch changeset required for this consumer-visible fix.
[Reviewed by Robohands]
Selector, MultiSelector, Typeahead, and DateTimeInput paired hover-updated highlights with an unconditional scrollIntoView effect, so a stationary pointer kept re-highlighting and scrolling -- the same runaway loop already fixed for DropdownMenu and Chat (facebook#6077). The scroll effect and its hover/keyboard split now live in useHighlightedOptionScroll, owned by each path's highlight owner: useCombobox and useMultiCombobox (also inherited by CommandPalette), BaseTypeahead, DateTimeInput, and Chat's useTriggerMenu (migrated from its inline copy). DropdownMenu keeps its focusMenuItemOnHover {preventScroll: true} fix; CommandPaletteItem's per-item hover scroll remains tracked in facebook#6077.
@cixzhang
cixzhang
left a comment
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.
Thanks—my prior review asked for the sibling option-list paths. This head fixes Selector, MultiSelector, Typeahead, and DateTimeInput while preserving keyboard scrolling, and adds the patch changeset.
CommandPalette still bypasses the hook’s hover callback: its items call the raw setter, then both the new useCombobox effect and CommandPaletteItem call scrollIntoView. In exact-head Chromium, one hover and one ArrowDown each produced two calls (main produces one), so the stationary-pointer runaway path remains and this head adds duplicate layout work. Please route CommandPalette hover through the hover-aware path and leave one keyboard scroll owner, with a regression test.
[Reviewed by Robohands]
CommandPaletteItem still called the raw setHighlightedIndex on hover and kept its own scrollIntoView effect, so hover highlights bypassed the hover-aware path and every keyboard scroll fired two scrollIntoView calls -- the stationary-pointer runaway loop (facebook#6077) remained and the head added duplicate layout work. The context now exposes useCombobox's onItemMouseEnter instead of the raw setter; CommandPaletteItem's scroll effect only runs for standalone use, leaving useHighlightedOptionScroll as the single keyboard scroll owner. Inline doc previews take the hover-aware path for the initial picker-mode highlight so opening still never scrolls the surrounding page. Adds a regression test covering hover-no-scroll and one scroll call per key.
@cixzhang
cixzhang
left a comment
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.
Thanks, this now closes the shared option-list paths. Chromium confirms hover stays still and keyboard navigation scrolls once, including CommandPalette. Could you credit @faga295 in the changeset?
[Reviewed by Robohands]
Approval removed at the author’s request.
@cixzhang
cixzhang
left a comment
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.
Thanks for helping to fix this. However the fix introduces multiple new API surfaces without clear intention. Could we fix the issue without adding API?
faga295
commented
Sep 6, 2026
Thanks for helping to fix this. However the fix introduces multiple new API surfaces without clear intention. Could we fix the issue without adding API?
ok
@cixzhang
cixzhang
left a comment
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.
Thanks—the auto-scroll fix is now solid. Chromium confirms hover stays put and keyboard navigation scrolls once across all seven menu paths.
My prior review asked to avoid new API. This head only changes the changeset credit, so CommandPaletteContextValue still replaces public setHighlightedIndex(index) with required onItemMouseEnter(item, index). The same consumer compiles on main and fails here. Could you keep the exported context shape compatible and route hover through an internal seam? The patch changeset is right once that API break is gone.
[Reviewed by Robohands]
...ist handler CommandPaletteContextValue keeps main's setHighlightedIndex instead of gaining onItemMouseEnter, so consumers that compile on main are unaffected. Hover highlighting moves to a single delegated onMouseEnter handler on CommandPaletteList, which resolves the hovered option and routes it through useCombobox's internal hover-aware path (highlight without scrolling, facebook#6077).
Fixes #6077
When a scrollable menu's highlight follows the mouse, scrolling the highlighted item into view moves the next item under the stationary pointer, which re-highlights and scrolls again — a runaway auto-scroll loop with no user input.
Two instances of the same pattern:
Chat/useTriggerMenu.tsx: hover-driven highlight changes now skip one run of the scroll-into-view effect (hoverHighlightRef); keyboard navigation still scrolls the highlighted option into view.DropdownMenu/menuItemHover.ts:focusMenuItemOnHovernow callsel.focus({preventScroll: true}). This is a one-line root-cause fix that covers DropdownMenu, ContextMenu, and BreadcrumbMenu items via the shared helper — the focus highlight does not require scrolling, and keyboard navigation has its own scroll-into-view handling.Tests
DropdownMenu.test.tsx: the existing hover-focus test now assertsfocuswas called with{preventScroll: true}(regression guard for the auto-scroll loop).ChatComposerInput.test.tsx: coverage for the hover-skip behavior inuseTriggerMenu.