SuggestionList: use requestAnimationFrame instead of setTimeout when scrolling selected item into view

[ciampo]

What?

As originally flagged by @andrewserong in #44526 (review), currently SuggestionList (used in the ComboboxControl and FormTokenField components) uses a 100ms timeout when scrolling selected list items into view. This timeout causes the UI to be very unresponsive when hovering over the list of suggestions.

This PR swaps the setTimeout call with a requestAnimationFrame call, in order to minimize the delay while keeping the "scroll into view" logic unchanged.

Why?

Make out UI smoother and more pleasant to interact with

How?

Swap the setTimeout call with a requestAnimationFrame call, in order to minimize the delay while keeping the "scroll into view" logic unchanged.

Testing Instructions

On ComboboxControl:

• Click on the input, scroll down and pick an item that would normally render "below the fold" in the list of suggestions
  • Make sure that the suggestions are highlighted in a responsive manner when hovering (contrarily to what happens on trunk)
• Click outside the combobox to blur the input
• Click on the combobox's input again:
  • the suggestion list should open and the list of suggestions should be scrolled to the previously selected item
• As a bonus: while the list of suggestions is opened, move the cursor to the edges of the list
  • Make sure that when hovering on an item that is only partially visible, the list automatically scrolls that item fully into view
• Make sure that the same behaviors described above can be observed on trunk as well

Screenshots or screencast

trunk:

combobox-control-timeout-trunk.mp4

This PR:

combobox-control-timeout-PR.mp4

[ciampo]

I'm not sure there's a point in running cancelAnimationFrame in the effect's return function, and so for now I've avoided it.

[andrewserong]

I didn't get a chance to review properly today, unfortunately, but hopefully should get a bit more time next week once some things for 6.1 Beta 3 are ready.

From a quick skim of the failing unit test, though, it's reporting:

["Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in %s.%s", "a useEffect cleanup function", "

Is that warning showing up because the requestAnimationFrame is still firing? If so, I wonder if we might need to grab the id returned from requestAnimationFrame and call cancelAnimationFrame so that it doesn't attempt to update state after unmounting?

[ciampo]

Makes sense! Done in 566b233

[ciampo]

Hey @andrewserong , would you mind giving this another round of review? Thank you!

[andrewserong]

Thanks for your patience, and for the reminder ping @ciampo! This is all testing nicely for me in Chrome, FF, Safari, and Edge on Mac. I tested in Storybook, and using the parent page selector in a test site with a large number of pages.

✅ Suggestions are now highlighted in a responsive manner
✅ Hovering over an item, and then blurring the field, and then selecting the drop-down again correctly returns the scroll position to the previously hovered item
✅ Hovering over a partially visible item at the bottom of the list scrolls it into view
✅ Double-checked that the scrollView function from dom-scroll-into-view appears to attempt to scroll immediately (i.e. doesn't use smooth scroll) here, so using requestAnimationFrame seems safe to me 👍

Before	After

This LGTM! ✨

I believe this PR will need a rebase in order to land. I kicked off the failing e2e, but it looks like it needs this commit (3983fee) in order to pass, now that the 6.1 RC 1 has been released.

Thanks again for the follow-up!

[ciampo]

Thank you Andrew for the thorough review! I've just rebased, will merge once CI is ✅