[docs][material-ui] Fix Material Icon search lag and other improvements

[anle9650]

• I have followed (at least) the PR section of the contributing guide.

Fixes #41126

Screen.Recording.2024-03-01.at.8.32.19.PM.mov

Preview: https://deploy-preview-41330--material-ui.netlify.app/material-ui/material-icons/

[mui-bot]

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against c39b78d

[DiegoAndai]

Hey @anle9650, thanks for working on this.

I see a problem with this implementation though:

Screen.Recording.2024-03-06.at.10.56.22.mov

It's missing keystrokes because the query value is not updated immediately. For example if I press a``s``d quick, only d is inputed. This is probably because when the d useTransition is dispatched, the a and s haven't made their way to the query variable yet, so we get setQuery('d') instead of setQuery('asd'). Does that make sense?

I would imagine the solution to be to modify useQueryParameterState, but I'm not sure how without digging deeper.

Let me know if you can think of alternative solutions having this in mind.

cc: @oliviertassinari

[anle9650]

@DiegoAndai good catch! That makes sense. I'll try to come up with a solution.

[anle9650]

I think I figured it out. setUrlValue(newValue) is the heavy UI task causing the bottle neck. setState(newValue) is the task that updates the value of the input field. By using the transition hook on setUrlValue(newValue), we prevent it from blocking setState(newValue), which allows for the input field to be quickly updated.

Lmk what you guys think!

[DiegoAndai]

By using the transition hook on setUrlValue(newValue), we prevent it from blocking setState(newValue), which allows for the input field to be quickly updated.

This makes sense to me. The query param seems only to be read on the initial render, so I don't think we'll have synchronization issues between the state and the query param.

@Janpot, I noticed you initially implemented the feature. What do you think of this change? I want to ensure I'm not missing context on this feature's history.

[Janpot]

I may be wrong, but I don't believe it's setUrlValue that's blocking the main thread, it's a debounced function, all it does is schedule a function call for later execution. It rather seems to be that it's caused by rendering the large amount of icons in the Icons component:

I believe the right approach is to defer the derived state that is used to render the icons, this will allow the UI to rerender in the background and be interruptible when the input value updates:

diff --git a/docs/data/material/components/material-icons/SearchIcons.js b/docs/data/material/components/material-icons/SearchIcons.js
index fc2d1f34e1..558606fca7 100644
--- a/docs/data/material/components/material-icons/SearchIcons.js
+++ b/docs/data/material/components/material-icons/SearchIcons.js
@@ -532,6 +532,8 @@ export default function SearchIcons() {
     [theme, keys],
   );
 
+  const deferredIcons = React.useDeferredValue(icons);
+
   const dialogSelectedIcon = useLatest(
     selectedIcon ? allIconsMap[selectedIcon] : null,
   );
@@ -579,9 +581,9 @@ export default function SearchIcons() {
           />
         </Paper>
         <Typography sx={{ mb: 1 }}>{`${formatNumber(
-          icons.length,
+          deferredIcons.length,
         )} matching results`}</Typography>
-        <Icons icons={icons} handleOpenClick={handleOpenClick} />
+        <Icons icons={deferredIcons} handleOpenClick={handleOpenClick} />
       </Grid>
       <DialogDetails
         open={!!selectedIcon}

For me this seems to make the input a lot more responsive while the UI is rendering. However, this doesn't solve the lag between typing and rendering the filtered icons. If we want to solve this, I believe virtualization of the icons list is in order, to reduce the amount of elements that are being rendered.

In hindsight, the whole thing can probably be simplified after this change. As the render becomes interruptible, we can likely remove the debounce of the search function altogether.

Updated this PR with the suggestions and a few other tweaks:

• Fix flexsearch import to get correct typing
• Show feedback when the UI is rendering in the background
• Fix RadioGroup controlled props
• Remove index search debounce.

Preview: https://deploy-preview-41330--material-ui.netlify.app/material-ui/material-icons/

Screen.Recording.2024-03-14.at.16.51.10.mov

To be clear: this solution solves the search input responsiveness. The INP as reported in #41126 may also be caused by long hydration. I believe this can not be solved without reducing the amount of rendered icons. cc @oliviertassinari

[danilo-leal]

This PR → #41415 seems to be also touching on some of these aspects regarding the number of displayed icons, input responsiveness, etc. Just want to make sure we don't discuss the same thing in two places 😃 Maybe this one should be prioritized as it's a "narrower" scope than the other?!

[DiegoAndai]

Thanks for taking a deep look into this @Janpot 🙌🏼 your changes make sense to me

It seems that there's room to improve the solution further by optimizing the icon's rendering, for example, by reducing or limiting the amount, and also maybe adding pagination. This was also touched upon in #41415.

Also, If the goal is to improve INP, is there a way for us to measure if a change is improving it? We should figure that out before continuing to work on the solution. With that, we can test if reducing the amount of icons rendered improves the score.

Besides that, I think we should point this PR to the next branch when that's created next week.

[Janpot]

Also, If the goal is to improve INP, is there a way for us to measure if a change is improving it?

I suppose we could use the react profiler. e.g. comparing typing "foo" in the empty box:

• https://mui.com/material-ui/material-icons/ ~250ms render
  

• https://deploy-preview-41330--material-ui.netlify.app/material-ui/material-icons/ all renders stay below ~60ms
  

[ZeeshanTamboli]

@Janpot @DiegoAndai @anle9650

I’ve virtualized the icons using react-virtuoso, resulting in a noticeable performance improvement (see the screen recording below):

PR-41330.mp4

I’ve also changed the scrolling to be within the Search Icons container, which I think looks better than scrolling the entire page.

Additionally, in commits ba2e45c and e083e60, I fixed an intermittent bug where the page becomes unclickable after closing the Icon Dialog. The issue is linked to react-transition-group, as mentioned in #32286. I used a workaround to resolve it, though it doesn’t occur consistently and wasn’t captured in the recording. You can reproduce it with these steps:

1. Visit the https://next.mui.com/material-ui/material-icons/ or https://mui.com/material-ui/material-icons/.
2. Click an icon to open the Dialog.
3. Close the Dialog by clicking outside or on the close button.
4. The page may become unresponsive to clicks or scrolling, with the Dialog element still present in the DOM and the Backdrop having opacity: 0.

This issue isn’t directly related to this PR. I can revert the change and address it separately if needed.

URLs to test:

• Before: https://next.mui.com/material-ui/material-icons/
• After: https://deploy-preview-41330--material-ui.netlify.app/material-ui/material-icons/

Please also test all changes on a mobile device.

[Janpot]

Running lighthouse on these show barely any improvement. We should further profile the page load. I expect the inclusion of @mui/icons-material in this page bundle to be heavy on the page loading. Perhaps we should try how it feels when we use code splitting and load this module through dynamic import?

edit:This shouldn't be part of this PR. Just some thoughts I had when I was exploring the performance of this page. We can keep the scope of this PR to what it already is.

[ZeeshanTamboli]

I expect the inclusion of @mui/icons-material in this page bundle to be heavy on the page loading. Perhaps we should try how it feels when we use code splitting and load this module through dynamic import?

Yes, I also noticed this. It's a good idea. This is especially important because, according to google analytics, this is the most visited page consistently.

As for the search performance improvements in this PR, it's ready for review.

[Janpot]

Looks ok to me. I can't reproduce the problem with the dialog though.

[ZeeshanTamboli]

I won’t cherry-pick this to master since we're likely releasing stable v6 soon, and the docs will be merged into mui.com.

[oliviertassinari]

If the goal is to improve INP, is there a way for us to measure if a change is improving it?

@DiegoAndai We can use https://chromewebstore.google.com/detail/web-vitals/ahfhijdlegdabablpippeagghigmibma?hl=en.

I got introduced to it at https://www.youtube.com/watch?v=KZ1kxzsJZ5g&t=223s. After 28 days, we can also use the field data from https://pagespeed.web.dev/ (aggregated data by Chrome sent anonymously).

---

Before: https://deploy-preview-41329--material-ui.netlify.app/material-ui/material-icons/?query=ale INP 1,000ms without much struggle to trigger

Screen.Recording.2024-09-01.at.23.20.04.mov

After: https://deploy-preview-41330--material-ui.netlify.app/material-ui/material-icons/

Screen.Recording.2024-09-01.at.23.21.35.mov

The INP problems seem solved. Pretty cool!

A related issue on this: mui/mui-x#9657.

---

Now, the problems:

1. Icons are no longer available when searching with Algolia the initial DOM output. Similar outcome with Google search. https://v4.mui.com/components/material-icons/#material-icons

"burger" / "facebook", people searching for those icons: https://dashboard.algolia.com/apps/TZGZ85B9TB/analytics/no-results/material-ui?from=2024-08-01&to=2024-08-31

We might want to restore this feature. It could be simply about rendering a hidden list of <ul><li> for Algolia.

2. I'm not sure about the virtualization UX. I'm not so convinced about the double scrollbar. When I have a large screen and have a bunch of results, I want to be able to scan, I can't do this anymore. It's kind of OK now, so fair, but I can't help myself thinking how is SvgIcon so slow that it doesn't work? When I look at the Chrome performance timeline, I mostly see MUI System that are slow. Removing the virtualization would allow us to better measure progress toward making it fast. It would also fix 1. It makes sense to me to do it.

3. We can't do useMemo to use the query, it hydration mismatch:

To fix this.

4. We use useDeferredValue but we don't memo the children, so it harms the performance since it means we render x2. If we want to improve them, we need to:

diff --git a/docs/data/material/components/material-icons/SearchIcons.js b/docs/data/material/components/material-icons/SearchIcons.js
index d8378f881f..00f470e74e 100644
--- a/docs/data/material/components/material-icons/SearchIcons.js
+++ b/docs/data/material/components/material-icons/SearchIcons.js
@@ -130,6 +130,7 @@ const StyledSvgIcon = styled(SvgIcon)(({ theme }) => ({
 }));

 const ListWrapper = React.forwardRef(({ style, children, ...props }, ref) => {
+  console.log('ListWrapper');
   return (
     <div
       ref={ref}
@@ -141,6 +142,9 @@ const ListWrapper = React.forwardRef(({ style, children, ...props }, ref) => {
   );
 });

+const virtuosoComponents = { List: ListWrapper };
+const virtuosoStyle = { height: 500 };
+
 function Icon(handleOpenClick) {
   return function itemContent(_, icon) {
     const handleIconClick = () => {
@@ -528,6 +532,9 @@ export default function SearchIcons() {
   const deferredQuery = React.useDeferredValue(query);
   const deferredTheme = React.useDeferredValue(theme);

+  console.log('deferredQuery', deferredQuery, query)
+
+  const itemContent = React.useMemo(() => Icon(handleOpenClick), [handleOpenClick]);
   const isPending = query !== deferredQuery || theme !== deferredTheme;

   const icons = React.useMemo(() => {
@@ -604,10 +611,10 @@ export default function SearchIcons() {
           icons.length,
         )} matching results`}</Typography>
         <VirtuosoGrid
-          style={{ height: 500 }}
+          style={virtuosoStyle}
           data={icons}
-          components={{ List: ListWrapper }}
-          itemContent={Icon(handleOpenClick)}
+          components={virtuosoComponents}
+          itemContent={itemContent}
         />
       </Grid>
       {/* Temporary fix for Dialog not closing sometimes and Backdrop stuck at opacity 0 (see issue https://github.com/mui/material-ui/issues/32286). One disadvanta
ge is that the closing animation is not applied. */}

It's documented in https://react.dev/reference/react/useDeferredValue#deferring-re-rendering-for-a-part-of-the-ui. So we solved INP with react-virtuoso, not by using suspense features.

---

How about we revert this and instead only go with deferred value?

We can fix the slow render another time but still get a search input that responds quickly to events. We are nowhere near having a performance issue on https://v4.mui.com/components/material-icons/, even if we x2 the time because there are x2 more icons now. So to me, the red flag is Material UI v5, we have to fix this regression we introduced 3 years ago, we sacrificed it for dynamic props, and now CSS variables mean we don't need this anymore.

@romgrk has been looking a bit at it lately.

[Janpot]

How about we revert this and instead only go with deferred value?

Agreed, approving this was poor judgement on my side. Reverting back to the changes proposed in #41330 (comment)

[ZeeshanTamboli]

@Janpot Thanks for handling it. I didn't realize it would affect global search, even on Google.

I'm not so convinced about the double scrollbar. When I have a large screen and have a bunch of results, I want to be able to scan, I can't do this anymore.

I think you're aiming for something like this: https://fonts.google.com/icons. However, we need the double scrollbar due to other content on the page, like the API section. IMO, It's better than the old version - https://v5.mui.com/material-ui/material-icons/, where you had to scroll all the way down to reach the API section. Maybe rendering a large container would be better. I'll leave it to you guys to decide.

[Janpot]

Did a quick experiment, I think there could be a few low hanging fruits here:

• using a single handler for the handleIconClick instead of recreating it for every icon and removing the extra div
• using an intersection observer to conditionally replace the svg with a placeholder div. Virtualization, but it retains the texts for SEO.

These improved Icons rendertime from about 450ms down to 150ms (locally, dev mode). I'm sure we can optimize more.

Edit: Looks like building the searchindex takes about 170ms synchronously while the module initializes. We can think about deferring this as well to improve initial load time