[Masonry][lab] Fix layout flicker issue and ResizeObserver loop error

[DiegoAndai]

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

Closes: #36673

Context

The Masonry component initially renders all the items (in one row) to measure their computed heights. Then, it places them in the corresponding columns, which results in a flicker between the initial render and the ordered render.

This flicker was made worse by #37208, which fixed #36909. The issue was that a ResizeObserver loop error would show up, and the fix was to handle the loops by using requestAnimationFrame. The usage of requestAnimationFrame delays the placement, making the flicker more noticeable.

Here's a good explanation for the ResizeObserver loop error: https://webkit.org/blog/9997/resizeobserver-in-webkit/#:~:text=observation.-,How%20Observations%20are%20Delivered.

Changes

• Revert #37208. There were three different reasons the ResizeObserver loop error would occur, which we can fix:
  • The first occurrence is due to line break elements being observed, which is unnecessary. This was a 2-level loop: add an item > resize triggers > recalculate layout > position item > line break elements height is set > resize triggers again, causing the error to appear. We don't need to watch the line break elements as we set the height for them. Not observing these removes the loop.
  • The second occurrence is due to new items being shrunk to the previous max height. For example, imagine a 3-column Masonry with 8px spacing and two items of 100px height each. The max height of the Masonry in that state is 116px, as we added spacing as margin on top and bottom of each item. When adding a 200px item, it would shrunk to 116px, the previous max-height, but because we added spacing on top and bottom to the new item, its actual height was 132px. The new item triggers the observer, and the max height would be set to 132px, and the item's new height to 148px. It would loop until the actual item's height was 200px. The loop is avoided by not allowing the items to shrink with flex-shrink: 0. A video of this example is added below. You'll think that this would cause the added item never to reach its height if spacing is 0, and indeed that's the case: https://codesandbox.io/s/masonry-shrinking-loop-k5hhqr
  • The third occurrence was when using React's Strict Mode. The initial render happens twice, which caused the child elements to be observed twice in quick succession, causing the error. Using requestFrameAnimation for the observing setup fixes this issue. The difference from #37208 is that the delay is on the observing setup, which is invisible to the user, instead of the placement calculation.

Video of the shrinking loop

Adding a debugger call between renders allows us to appreciate the shrinking loop. The partially calculated column heights appear at the bottom of the recording.

Screen.Recording.2023-08-21.at.12.52.08.mov

[mui-bot]

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8e7f111

[tangye1234]

do not use map children and clone element. The underneath element may not accept style prop.

[tangye1234]

I basically have resolved the flicker issues in my own infinite photo gallery masonry component. I can provide some more important steps below, but the resize observer loop issue sometimes happened but rarely.

1. Flicker issue happened due to line breaks observed, which you have already point out.

2. Flicker issue happened due to new children coming in without style order calculated at the first time added, while you point out and add flex-basis with clone element. This is not suitable enough. It is still too late, and clone element narrows down the children component kinds comparing to the original design(minimize the changing to the children). I use another way: pre-style &>*:not(.line-breaks) {...} every elements(not line breaks) in component parent style, and recalculate/restore it in the observer callbacks. This makes new items coming in with styles already.

3. The most important should be this, avoid set any react state including setState in flushSync. Many flicker issues happen because the calculation is not in the right/synchronized state, one case is the maxHeight state, when the container is resized to a larger ones, the height not sync with the calculation, while every children is bigger, and there is not enough room for them to place in the right column, unless height state is synced. I have already resolve the two inner react state by using css property. A height property in the parent, and the quantity of line breaks can be statically large enough, such as 10.

4. Flicker issues happened due to the precision of calculation. Example: the height of the cards is 5.76 and 5.77. Which card should be placed next. The order changes too frequently when resizing. We should debounce the order variation.

If you need the code, I will write a minimal demo out of our project in my bare time.

[siriwatknp]

Off-topic: I think Masonry should start using gap which could make the implementation cleaner?

[DiegoAndai]

Thanks for the feedback both!

---

@tangye1234 regarding some of your points:

2. Flicker issue happened due to new children coming in without style order calculated at the first time added

I don't understand this point. Some code might help to understand better 😅

3. The most important should be this, avoid set any react state including setState in flushSync

This is not possible as we use flushSync to avoid flickering: #33163. I don't think it's causing any problems right now. Do you have a repro case for flushSync causing flickering?

4. Flicker issues happened due to the precision of calculation

Do you have a repro case for this?

---

@siriwatknp I refactored with your suggestion, so it's ready for re-review 😊

I think Masonry should start using gap which could make the implementation cleaner?

Yeah, I think whenever we wish to refactor the Masonry, we should use gap

[tangye1234]

Thanks for the feedback both!

@tangye1234 regarding some of your points:

2. Flicker issue happened due to new children coming in without style order calculated at the first time added

I don't understand this point. Some code might help to understand better 😅

3. The most important should be this, avoid set any react state including setState in flushSync

This is not possible as we use flushSync to avoid flickering: #33163. I don't think it's causing any problems right now. Do you have a repro case for flushSync causing flickering?

4. Flicker issues happened due to the precision of calculation

Do you have a repro case for this?

@siriwatknp I refactored with your suggestion, so it's ready for re-review 😊

I think Masonry should start using gap which could make the implementation cleaner?

Yeah, I think whenever we wish to refactor the Masonry, we should use gap

Hi, here is my demo: https://stackblitz.com/edit/stackblitz-starters-ktbdbe?file=components%2Fmasonry.tsx

I use the styled-components to make the masonry component.

[siriwatknp]

👍 Looks good to me. @mnajdova Do you have some context about the Masonry? would be great to have another eye on this.

[DiegoAndai]

Thanks for sharing your code @tangye1234! I replied to you in #36673 (comment)

[mnajdova]

Fix layout flicker issue

I would rephrase this to "Improved layout flicker", so that we don't give wrong expectations - there is still flickering when changing the opacity value.

Hide the initial rendering using opacity: 0 before the initial calculation is done. Before this initial calculation, the component is not ready to be shown, so hiding it makes sense.

This is the only thing I am not 100% certain about, but I don't have a better proposal. The main reason is that, usually the Masonry component will wrap almost the whole page - imagine dashboard, image gallery etc. This will mean that the response from the server will always be fully hidden.

[DiegoAndai]

@mnajdova I reverted the opacity, default height, and !important changes as discussed.

We should only merge this once we find a better solution for the flickering on the initial rendering. It is still not acceptable. I had the following idea:

Add a new effect (see commit). This effect should run handleResize before the initial paint, as useEnhancedEffect becomes useLayoutEffect in the client. This way, we could set the layout before the initial paint without flickering.

But, for some reason, I still get the initial flickering (see attached video). The handleResize seems to be still running after the initial paint. It's weird because useLayoutEffect is used for this case exactly 😅, but there must be some reason why it is not working.

I'm stuck here. Would you happen to know why this is not working? I would appreciate it if you could take a look 😊.

Masonry-flicker.mov

[mnajdova]

Add a new effect (see commit). This effect should run handleResize before the initial paint, as useEnhancedEffect becomes useLayoutEffect in the client. This way, we could set the layout before the initial paint without flickering.

We already have a useEnhanchedEffect that calls the resize observer handler, how is the new one different?

[DiegoAndai]

@mnajdova

We already have a useEnhanchedEffect that calls the resize observer handler, how is the new one different?

The idea for the new useEnhancedEffect is that it triggers handleResize instantly on mount. This way, it should run before the initial paint, avoiding the flicker.

The existing one sets up the observer for subsequent resizes. On that one, we need requestAnimationFrame, so the first resize will always be called a frame after the initial paint, causing the flicker.

That's the theory. It's not working though 😅

[luisjavierbautista]

Hi there, any news about this?? Just 1 test is pending! :)

[DiegoAndai]

Hey @luisjavierbautista, thanks for the interest!

I haven't found a better solution for my comment here. I think it might be better to remove the requestAnimationFrame altogether. This would bring back the Resize loop warning for developers using strict mode. I will discuss it with the core team tomorrow.

[DiegoAndai]

Converting this to a draft as it's not ready yet. The thing missing is to solve the initial render flicker. All my thoughts are in the PR's description and comments.

I cannot allocate more time to keep working on this right now. I can provide guidance if anyone wishes to continue it or propose another solution.