[Lab][Masonry] Fix layout flicker and single column issue #43903
Conversation
Netlify deploy previewhttps://deploy-preview-43903--material-ui.netlify.app/ Bundle size report
|
zannager
added
package: lab
zannager
requested review from
DiegoAndai and
mnajdova
and removed request for
mnajdova
This commit addresses two long-standing issues with the Masonry component: 1. A flicker/layout shift on initial render, where items would briefly appear in a single column. 2. The layout breaking into a single column when the first item is hidden. The fix involves: - Removing `requestAnimationFrame` from the `ResizeObserver` to prevent rendering delays. - Using `ReactDOM.flushSync` to ensure DOM updates are painted synchronously. - Introducing a `MutationObserver` to dynamically track and measure children, making the layout updates more robust. - Modifying the layout logic to find the first *visible* child for width calculation, which correctly handles cases where initial items are hidden. Fixes mui#36673 Fixes mui#42611
Fanzzzd
changed the title
|
Hey @Fanzzzd, thanks for working on this! Removing Would you consider splitting this into two PRs, one for each issue, or is that complicated? @siriwatknp may I ask you to take a look when you have some time? |
The previous change using `flushSync` directly in the `handleResize` callback fixed a layout flicker but introduced a React warning: "flushSync was called from inside a lifecycle method". This commit wraps the `flushSync` call in a `Promise.resolve().then()` to move it to a microtask. This resolves the warning by ensuring React is not in the middle of a render cycle when `flushSync` is called, while still being fast enough to avoid the visual flicker that `requestAnimationFrame` could cause. An additional check for `masonryRef.current` is added inside the promise callback to guard against updates on an unmounted component. Refs mui#36673, mui#42611, mui#36909
@DiegoAndai , thanks for feedback! You were right to be cautious about the ResizeObserver loop. I'm pretty confident this change won't bring it back. My last attempt fixed the flicker but caused React warning in the console. This new version (4f83b82) wraps the flushSync in a Promise, which fixes the warning, all without reintroducing the visual flicker. As for splitting the PR, I'd prefer to keep it as one if that's okay. These two fixes are tied together in the handleResize function. It is easier refactor the layout logic as a whole to make it stable, so splitting it would mean merging a broken or incomplete state. I also think it's easier for you to review the complete solution all at once this way. Hope that makes sense. I also tested the case in #36909 36909.test.mov |
|
I am waiting for this merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical UX issues in the Masonry component by eliminating layout flicker on initial render and preventing single-column layout when the first child is hidden.
- Replaces asynchronous ResizeObserver handling with synchronous DOM updates using ReactDOM.flushSync
- Adds MutationObserver to detect child element changes and dynamically observe/unobserve them
- Updates layout calculation to use the first visible child instead of the first DOM child
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Promise.resolve().then(() => { | ||
| if (masonryRef.current) { | ||
| ReactDOM.flushSync(() => { | ||
| setMaxColumnHeight(Math.max(...columnHeights)); | ||
| setNumberOfLineBreaks(currentNumberOfColumns > 0 ? currentNumberOfColumns - 1 : 0); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Wrapping flushSync in Promise.resolve().then() defeats the purpose of synchronous updates mentioned in the PR description. This reintroduces asynchronous behavior and may cause the flicker issue to persist. Consider removing the Promise wrapper to ensure truly synchronous DOM updates.
| mutationObserver.disconnect(); | ||
| }; | ||
| }, [columns, spacing, children, handleResize]); | ||
| }, [handleResize]); |
There was a problem hiding this comment.
The useEnhancedEffect dependency array only includes handleResize, but the effect also depends on columns, spacing, and children for proper cleanup and re-initialization. When these props change, the observers should be recreated to reflect the new layout requirements.
| }, [handleResize]); | |
| }, [handleResize, columns, spacing, children]); |
|
Key Changes Made:
|
|
@Fanzzzd Thanks for the PR, I pushed the latest change as minor improvements. Can you provide/set up a repo that contain the demos from your description? I'd like to test it visually too. |
|
@siriwatknp Many thanks for the updates! I can spin up a repo with the demos if you prefer, but the quickest way to verify the fix is in the MUI playground. I’ve added a small demo in docs/pages/playground/index.tsx, dropping the same file into that folder on your side should work as well. If you’d still like a standalone repo, I’m very happy to set one up. import * as React from 'react';
import Masonry from '@mui/lab/Masonry';
import {
Box,
Paper,
Typography,
Switch,
FormControlLabel,
Button,
styled,
} from '@mui/material';
const Item = styled(Paper)(({ theme }) => ({
backgroundColor: theme.palette.mode === 'dark' ? '#1A2027' : '#fff',
...theme.typography.body2,
padding: theme.spacing(0.5),
textAlign: 'center',
color: theme.palette.text.secondary,
display: 'flex',
alignItems: 'center',
justifyContent: 'center',
}));
const heights = [150, 30, 90, 70, 110, 150, 130, 80, 50, 90, 100, 150, 30, 50, 80];
/**
* Delays rendering of its children to simulate items loading at different times.
*/
const DelayedItem = ({ children }: { children: React.ReactElement }) => {
const [isShown, setIsShown] = React.useState(false);
React.useEffect(() => {
const timer = setTimeout(() => {
setIsShown(true);
}, Math.random() * 800 + 200);
return () => clearTimeout(timer);
}, []);
return isShown ? children : null;
};
export default function Playground() {
const [showFirstItem, setShowFirstItem] = React.useState(false);
const [showMasonry, setShowMasonry] = React.useState(true);
const handleRemount = () => {
setShowMasonry(false);
setTimeout(() => {
setShowMasonry(true);
}, 400);
};
return (
<Box
sx={{
width: '100%',
p: 2,
boxSizing: 'border-box',
}}
>
<Typography variant="h4" gutterBottom>
Masonry Component Playground
</Typography>
<Box sx={{ my: 4 }}>
<Typography variant="h5" gutterBottom>
Issue #42611: Layout forms a single column when first item is hidden
</Typography>
<Typography mb={2}>
This section tests the fix for the Masonry layout collapsing into a single column when its
first child is hidden. Initially, the first item is hidden. With the fix, the layout
should render correctly across multiple columns. You can toggle the visibility of the
first item to see the effect.
</Typography>
<FormControlLabel
control={
<Switch checked={showFirstItem} onChange={(e) => setShowFirstItem(e.target.checked)} />
}
label="Show first item"
/>
<Masonry columns={{ xs: 2, sm: 3, md: 4 }} spacing={2}>
{heights.map((height, index) => (
<Item
key={index}
sx={{
height,
display: index === 0 && !showFirstItem ? 'none' : 'block',
}}
>
{index + 1}
</Item>
))}
</Masonry>
</Box>
<Box sx={{ my: 4 }}>
<Typography variant="h5" gutterBottom>
Issue #36673: Layout flicker/shift issue
</Typography>
<Typography mb={2}>
This section is for observing the layout flicker issue. On initial load or when
re-rendering, the old component would show a single column of items for a moment before
arranging them into a masonry layout. With the fix, this flicker should be gone. Use the
button to remount the component and observe the loading behavior. Items are loaded with a
random delay to simulate real-world conditions like network latency and make any
flickering more apparent.
</Typography>
<Button variant="contained" onClick={handleRemount}>
Remount Masonry
</Button>
<Box sx={{ mt: 2 }}>
{showMasonry && (
<Masonry columns={{ xs: 2, sm: 3, md: 4 }} spacing={2}>
{heights.map((height, index) => (
<DelayedItem key={index}>
<Item sx={{ height }}>{index + 1}</Item>
</DelayedItem>
))}
</Masonry>
)}
</Box>
</Box>
</Box>
);
} |
|
Hi @siriwatknp , do you still need some more detailed information regarding this PR? I can provide it at any time. I have some new projects that would like to use MUI's Masonry, and updating this PR would help me a lot. |
siriwatknp
left a comment
siriwatknp
left a comment
There was a problem hiding this comment.
👍 Well done! Thanks for working on this.
|
Great job! When is this going to be released? |
7 participants
Overview
This PR addresses two long-standing and critical issues in the Masonry component:
display: 'none').These fixes significantly improve the user experience and component stability, making it more reliable for production use.
Key Changes
The solution involves a few core improvements:
requestAnimationFramelogic withReactDOM.flushSyncinside the resize handler. This ensures that layout calculations and DOM updates are applied synchronously before the browser's next paint, eliminating the root cause of the flicker.MutationObserverto reliably detect when child elements are added or removed. This makes the component correctly recalculate its layout when the children change dynamically.Demonstration
Before:
before.mov
After:
after.mov
Fixes #36673
Fixes #42611