[Select][base] Prevent unnecessary rerendering of Select options

[michaldudak]

This PR solves a performance issue with SelectUnstyled and dependent components (Joy's Select).

The root of the problem was that every mouseover event fired on an option caused rerendering of all options. The event correctly dispatches the optionHover action in the listbox reducer, but the surrounding logic caused some functions to be recreated, which caused the SelectUnstyledContext to be updated. This, in turn, caused all the options to detect the changes and rerender themselves.

A couple of optimizations were made:

1. Functions that don't need to be recreated every render were wrapped in useCallback
2. The SelectUnstyledContext value does not change when the selected or highlighted items change. Instead, a new way of notifying the options about changes was introduced. The options subscribe to updates in the select, and when an update event is fired, they can decide if the change concerns them and rerender accordingly.

The introduction of the lightweight event bus was necessary to update only the options that were changed. The Material UI implementation doesn't have this problem, as it uses cloneElement, so that the Select can set props directly on its MenuItems. This, however, can cause issues when grouping is used and prevents from using non-MenuItem components (or even Fragments) as Select's children. The discussion about removing cloneElement usage took place in #14943.

With these changes, the Option component has more to do. To prevent developers from having to reimplement this logic in their own design systems, the useOption hook was added.

Additionally, I refactored the code in a few places.

Breaking change

This PR changes what the useSelect and useMenu hooks return.

useSelect

The getOptionProps and getOptionState functions that were returned by useSelect are now members of the contextValue object returned by the hook.

-const { getOptionProps, getOptionState, value } = useSelect({ // ...
+const { contextValue, value } = useSelect({ // ...
+const { getOptionProps, getOptionState } = contextValue;

The whole contextValue object is intended to be passed into the SelectUnstyledContext.Provider that wraps all options.

useMenu

The getItemProps, getItemState, registerItem, and unregisterItem functions that were returned by useMenu are now members of the contextValue object returned by the hook.

-const { getOptionProps, getOptionState, registerItem, unregisterItem } = useMenu({ // ...
+const { contextValue } = useMenu({ // ...
+const { getOptionProps, getOptionState, registerItem, unregisterItem } = contextValue;

The whole contextValue object is intended to be passed into the MenuUnstyledContext.Provider that wraps all items.

---

Before: https://codesandbox.io/s/practical-oskar-ibmwvn?file=/demo.js
After: https://codesandbox.io/s/gifted-antonelli-srtk0u?file=/demo.js

This fixes #34112.

[mui-bot]

Netlify deploy preview

https://deploy-preview-35946--material-ui.netlify.app/

@material-ui/unstyled: parsed: +2.25% , gzip: +2.49%
@mui/joy: parsed: +0.63% , gzip: +0.77%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against bd46081

[oliviertassinari]

The root of the problem was that every mouseover event fired

Did we consider mouseenter? This event fires less often than the mouseover event. I don't recall why I didn't use mouseenter more in the past.

1. Functions that don't need to be recreated every render were wrapped in useCallback

Did you measure differences with this? I'm asking because I believe the default outcome with them is more CPU cycles (it takes more time to compare than to recreate the functions). However, it does make a difference when it prunes large branches of work (like not rendering 10 children).

After: https://codesandbox.io/s/gifted-antonelli-srtk0u?file=/demo.js

I think that the problem is present in this codesandbox, the hover effect lags +100ms behind. It seems fix, there is some JavaScript running on mouseover but it's a lot less work than before, and seems to be coming from React itself and not MUI Base 👍

[michaldudak]

Did we consider mouseenter? This event fires less often than the mouseover event. I don't recall why I didn't use mouseenter more in the past.

No, but in this case, I don't think it would make a difference. The problem was rerendering all options when hover moved from one to another.

the default outcome with them is more CPU cycles (it takes more time to compare than to recreate the functions)

Do you have any resources on this? I wonder how can comparing two references be slower than recreating a function. Anyway, in this case, recreating functions that are placed in a context causes a rerender of all listeners of the context (all options), which is definitely slower.

I think that the problem is present in this codesandbox, the hover effect lags +100ms behind

It could be perceived as that because of the transition of the background color. I disabled it so the difference is more noticable.

[oliviertassinari]

The problem was rerendering all options when hover moved from one to another.

@michaldudak Ok, it might have been because of bugs with SVGs or some element that wrongly trigger the mousenter event.

One thing I struggle to understand is the use case: why does the select need to know the hovered option? The Select in Material UI doesn't listen to it, the Autocomplete does but I think that it's a mistake I did per #23916. I shouldn't have mixed the hover and keyboard focus states, it breaks the UX and creates bugs like #20602.

Do you have any resources on this? I wonder how can comparing two references be slower than recreating a function.

The why: https://kentcdodds.com/blog/usememo-and-usecallback. It boils down to: useCallback is extra CPU work, the runtime still needs to recreate the function at each render.

It could be perceived as that because of the transition of the background color. I disabled it so the difference is more noticable.

Oh the transition, yes! @siriwatknp How about we remove all these transitions on hover (here and in other components when the delay to feedback is important)? It doesn't seem to help and doing a benchmark with other libraries, I can't find selects that have it that still feel great to use. It feels unresponsive (slow).

Off-topic. It looks like we have a bug with the scroll positioning when opening the select:

• Joy UI: https://codesandbox.io/s/frosty-diffie-h5cobm?file=/demo.js wrong position
• MUI Base: https://codesandbox.io/s/beautiful-haze-6p5o5w?file=/demo.tsx wrong position
• Material UI: https://codesandbox.io/s/pedantic-danny-euuyd2?file=/demo.js right position (super slow though)

It seems to be that the select is going beyond the viewport, we likely miss a max height of 100vh.

[michaldudak]

One thing I struggle to understand is the use case: why does the select need to know the hovered option?

This was a tough decision to make. Native <select> elements seem to use both mouse and keyboard to highlight an item. I am aware of the problems we have with Autocomplete. That's why I decided to make it configurable, so developers can choose what they like most. The code that runs on option hover calls a reducer with the optionHover action. By default, the built-in reducer just ignores the action, so nothing gets rerendered. However, a developer may choose to provide their own reducer that can highlight an option.

Oh the transition, yes! @siriwatknp How about we remove these transitions on hover (here and in other components when the delay to feedback is important)?

We could leave the "out" transition, but yes, I'd remove the "in" one.

Off-topic. It looks like we have a bug with the scroll positioning when opening the select

Thanks for pointing this out. The Select demo needs a max-height. The Joy Select may as well.

the runtime still needs to recreate the function at each render.

But thanks to this, we can prevent thousands of renders. I agree that, in some cases, the performance gain is not noticeable. Here, it definitely is. I'm not using useCallback here to minimize the number of function allocations but to prevent unnecessary rerenders.

[oliviertassinari]

By default, the built-in reducer just ignores the action, so nothing gets rerendered.

@michaldudak Ah ok, looks great 👍

We could leave the "out" transition, but yes, I'd remove the "in" one.

To experiment, I would expect no transition to feel better for hover. However, an "out" transition and no "in" sounds like a great idea to experiment with for the autocomplete, select, and date picker popups! Right now the autocomplete popup has no transitions, which I think feels great, but an out transition could feel even better. The date picker has an "in" transition which makes it feel too slow to be great (point 12 of mui/mui-x#7440): cc @gerdadesign.

[siriwatknp]

Oh the transition, yes! @siriwatknp How about we remove all these transitions on hover (here and in other components when the delay to feedback is important)?

Agree, #35952 will take care of that.

[siriwatknp]

@michaldudak Maybe this is off-topic but I wonder if the value prop should be required for the Option component?

Without the value prop, it looks like a bug https://codesandbox.io/s/joy-cra-typescript-forked-h6ybih?file=/src/App.tsx

[siriwatknp]

From Joy's perspective, this is a huge improvement! 👏 Great job @michaldudak.

[mnajdova]

Small nits here and there, apart from that I can't sport anything else. Great work @michaldudak!

[mnajdova]

I would do either:

1. Not add this at all

Suggested change

disabled: false,

2. Use the value from the props (we can initialize it above to false):

Suggested change

	disabled: false,
	disabled,

This is closer to what developers would actually need.

[mnajdova]

👍

[mnajdova]

Can we extract this above and define it only once?

[michaldudak]

I did this to make the tests more self-sufficient (it's easier to read when you have everything nearby). I don't think repetition is a problem is tests.

[mnajdova]

Suggested change

	const optionState = getOptionState(option);
	const index = options.findIndex((opt) => optionComparer(opt, option));
	const optionState = getOptionState(option);
	const { index } = optionState;

nit: we already are finding the index in getOptionState, we could return it as part of the state. Not sure if it's worth the change

[michaldudak]

Good point! This could make it even faster a bit.

[michaldudak]

Maybe this is off-topic but I wonder if the value prop should be required for the Option component?

I'll analyze it and create a separate PR.

[smellai]

I think some more breaking change was introduced in this release. Without changing the options array, the onChange event handler now gets object instead of string, and also there's a type mismatch there. Also highlighted state from optionState doesn't seem to be set correctly anymore.

https://codesandbox.io/s/heuristic-hypatia-brfyof?file=/demo.tsx

[michaldudak]

@smellai Could you please open a new issue so we can track this problem?

[enricoros]

Hi, starting with Joy 5.0.0-alpha.76 (.75 is working fine, .76+ are broken), Select > Options > text changes will not update the Select component.

The following code creates the options:

  <Select ...>
    {items.map((item: {key: string, label: string}, idx: number) => (
      <Option key={'opt-' + idx} value={item.key}>
        {symbol ? '-' : ''} {item.label}
      </Option>
    ))}
  </Select>

When toggling symbol:boolean, the Select will need a click to show the updated options, including the update in the "Select" itself.

The current workaround is to fix the version of Joy to alpha-75.

[siriwatknp]

Hi, starting with Joy 5.0.0-alpha.76 (.75 is working fine, .76+ are broken), Select > Options > text changes will not update the Select component.

The following code creates the options:

  <Select ...>
    {items.map((item: {key: string, label: string}, idx: number) => (
      <Option key={'opt-' + idx} value={item.key}>
        {symbol ? '-' : ''} {item.label}
      </Option>
    ))}
  </Select>

When toggling symbol:boolean, the Select will need a click to show the updated options, including the update in the "Select" itself.

The current workaround is to fix the version of Joy to alpha-75.

Thanks for reporting the issue! Can you open a new issue?