mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.36k stars 32.13k forks source link

Select popover is displayed in wrong position while using React.lazy #14782

Open codewithanugraha opened 5 years ago

codewithanugraha commented 5 years ago

Expected Behavior 🤔

Select popup should need to be displayed on right position.

Current Behavior 😯

Not displaying select popup in right position at first click while using React.lazy. It can only be reproduce in first click.

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/rrjr735myn

  1. Use react lazy import @material-ui/core/Select
  2. Click on select box (You will see popup on wrong side)
  3. If want to reproduce this again than refresh you page.

Your Environment 🌎

Tech Version
Material-UI 3.9.2
React 16.8.3
oliviertassinari commented 5 years ago

The reproduction can be further simplified into:

import React, { Suspense } from "react";
import ReactDOM from "react-dom";
import Select from "@material-ui/core/Select";

const MenuItem = React.lazy(() => import("@material-ui/core/MenuItem"));

function App() {
  return (
    <Suspense fallback="Loading...">
      <div style={{ float: "right" }}>
        <p>lazy loading</p>
        <Select onChange={() => {}}>
          <MenuItem>I am not in right position at first click</MenuItem>
        </Select>
      </div>
    </Suspense>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);
jsheetzati commented 5 years ago

Positioning is also incorrect when using Popover with suspense/lazy. Is it the same underlying issue or should I create a new open issue?

import React, { Suspense, useState } from "react";
import ReactDOM from "react-dom";

const Popover = React.lazy(() => import("@material-ui/core/Popover"));
const MenuItem = React.lazy(() => import("@material-ui/core/MenuItem"));

function App() {
  const [open, setOpen] = useState(false);
  const [target, setTarget] = useState(null);
  return (
    <>
      <Suspense fallback="Loading...">
        <div style={{ float: "right" }}>
          <p>lazy loading</p>
          <button ref={node => setTarget(node)} onClick={() => setOpen(true)}>
            click
          </button>
          <Popover open={open} anchorEl={target} onClose={() => setOpen(false)}>
            <MenuItem>I am not in right position at first click</MenuItem>
          </Popover>
        </div>
      </Suspense>
    </>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

https://codesandbox.io/s/4kq3mk2mx

oliviertassinari commented 5 years ago

@jsheetzati The Select use the Popover component internally, it might be related to: https://github.com/facebook/react/issues/14536.

gaearon commented 3 years ago

I've tried testing this with React 18 alpha which should fix some Suspense quirks: https://codesandbox.io/s/exciting-feather-s3j2l?file=/src/index.js. But it seems like something in JSS is throwing. I haven't looked closer but if someone wants to, you're welcome!

eps1lon commented 3 years ago

But it seems like something in JSS is throwing. I haven't looked closer but if someone wants to, you're welcome!

JSS had some bugs with concurrent rendering last time we tried. We've switched to emotion in v5 (alpha) and it seems like this is fixed: https://codesandbox.io/s/lively-wind-6gdqr?file=/src/index.js:595-754

The prop-type warning is a bit unfortunate though expected. Once we open the Popover, React suspends thus cleaning up the ref again. Once it unsuspends we don't have the anchorEl set (yet) so we trigger the warning if open && anchorEl === null. Can't set open={open && anchorEl !== null because then React constantly ping-pongs between suspended and unsuspended state.

rpggio commented 2 years ago

I found that (non-wrapped) suspense state in popover content causes popover in wrong position. It also throws warning: MUI: The anchorEl prop provided to the component is invalid. The fix was to wrap popover content with <Suspense>. I suppose MUI could provide a better warning, or automatically provide suspense component in the modal tree...