search

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.87k stars 32.26k forks source link

[Popover] Wrong positioning #25204

Open mark-night opened 3 years ago

mark-night commented 3 years ago

https://codesandbox.io/s/simplepopover-material-demo-forked-hj680 Notice how popover is shifted from expected position. This seems to be caused by the 'incorrect' top left CSS property values.

Those position values are traced back to getBoundingClientRect() call in referenced codes below. https://github.com/mui-org/material-ui/blob/7855f2b967b7edaae012f35e18480fa72374fcfd/packages/material-ui/src/Popover/Popover.js#L140 https://github.com/mui-org/material-ui/blob/7855f2b967b7edaae012f35e18480fa72374fcfd/packages/material-ui/src/Popover/Popover.js#L219

Certain CSS properties combinations change containing block calculation for child DOM elements. Details explained here. When container is set to some DOM element happens to be the situation, position values would need an extra calculation based on the value from getBoundingClientRect(), e.g., subtract position value of the container.

All components internally use popover (e.g. Menu) should suffer the same problem.

oliviertassinari commented 3 years ago

@mark-night Out of curiosity, why change the container?

In #17636. We will refactor the Popover to combine Modal + Popper instead of having its own position logic (removing it). It should solve this issue.

mark-night commented 3 years ago

@oliviertassinari Just to make DOM structure more managed, I personally prefer not to render modal-like nodes outside the structure. It looks like this is right what container attribute is there for. Besides, sometime it's necessary to limit modals within certain body area, e.g. to render a mobile UI in a webpage for desktop... Container is surely not the only way to achieve this, but it definitely makes it easier and makes more sense.

oliviertassinari commented 3 years ago

The primary use case for the container prop is to render inside different windows (popup, iframe, shadow dom). Inside nested DOM elements, it messed up the a11y hidden of the DOM tree.

mark-night commented 3 years ago

Ah. So it's not recommended unless I have to?