material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[Lab][Masonry] Fix layout flicker and single column issue

Open Fanzzzd opened this issue 1 year ago • 7 comments

Overview

This PR addresses two long-standing and critical issues in the Masonry component:

  1. A noticeable flicker and layout shift on initial render, where items briefly stack in a single column before arranging correctly.
  2. The layout breaking and collapsing into a single column when the first child item is hidden (e.g., with 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:

  • Synchronous DOM Updates: Replaced the requestAnimationFrame logic with ReactDOM.flushSync inside 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.
  • Robust Child Observation: Introduced a MutationObserver to reliably detect when child elements are added or removed. This makes the component correctly recalculate its layout when the children change dynamically.
  • Smarter Layout Calculation: The logic now correctly identifies the first visible child for measurements, instead of just the first child in the DOM. This resolves the issue where a hidden first item would break the column calculation.

Demonstration

Before:

https://github.com/user-attachments/assets/299e3716-e315-45fa-970a-9d51a4174219

After:

https://github.com/user-attachments/assets/ac2a0d43-daa5-442c-82cd-3e20ca9366d2


Fixes #36673 Fixes #42611

Fanzzzd avatar Sep 27 '24 16:09 Fanzzzd

Netlify deploy preview

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

Bundle size report

Bundle Parsed size Gzip size
@mui/material 0B(0.00%) 0B(0.00%)
@mui/lab 🔺+597B(+1.87%) 🔺+152B(+1.89%)
@mui/system 0B(0.00%) 0B(0.00%)
@mui/utils 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by :no_entry_sign: dangerJS against e7d8c9196a8bcc1fc4c412cf164f3a745d31491c

mui-bot avatar Sep 27 '24 16:09 mui-bot

Hey @Fanzzzd, thanks for working on this!

Removing requestAnimationFrame might bring back https://github.com/mui/material-ui/issues/36909 (see related comment). Have you checked if this regression is not reintroduced?

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?

DiegoAndai avatar Jul 23 '25 18:07 DiegoAndai

Hey @Fanzzzd, thanks for working on this!

Removing requestAnimationFrame might bring back #36909 (see related comment). Have you checked if this regression is not reintroduced?

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?

@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 https://github.com/mui/material-ui/issues/36909 No error showed. Everything looks good to me.

https://github.com/user-attachments/assets/714d2ac8-ddb2-44f6-a9f6-a6133337ab69

Fanzzzd avatar Jul 24 '25 02:07 Fanzzzd

I am waiting for this merge.

wwz223 avatar Aug 29 '25 02:08 wwz223

Key Changes Made:

  1. Replaced Promise wrapper with intelligent flushSync detection - Detects if we're in an observer callback where flushSync is safe - Falls back to regular state updates in other contexts - Eliminates flicker while avoiding React 18 warnings
  2. Added missing effect dependencies - Updated useEnhancedEffect to include columns, spacing, children - Ensures observers recreate properly when props change
  3. Performance optimization - Added 16ms debouncing to resize events (~60fps) - Prevents excessive recalculations during rapid resizes
  4. Enhanced test coverage - Added comprehensive tests for hidden children scenarios - Validates the fix works with hidden first child, dynamic hiding, and all hidden

siriwatknp avatar Sep 09 '25 04:09 siriwatknp

@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 avatar Sep 10 '25 02:09 siriwatknp

@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>
  );
}

Fanzzzd avatar Sep 18 '25 02:09 Fanzzzd

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.

Fanzzzd avatar Nov 25 '25 06:11 Fanzzzd

Great job! When is this going to be released?

Mikilll94 avatar Dec 01 '25 11:12 Mikilll94