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.96k stars 32.27k forks source link

[ModalManager] Dialog in Shadow DOM creates scroll jump #39636

Open arminbashizade opened 1 year ago

arminbashizade commented 1 year ago

Duplicates

Latest version

Steps to reproduce πŸ•Ή

Link to live example: https://stackblitz.com/edit/react-aj5fcw?file=index.tsx

Steps:

  1. add a dialog using Dialog demo
  2. add the same dialog in a Shadow DOM following steps from the docs
  3. add lots of divs to create overflow
  4. open the two dialogs and see the difference in handling the scrollbar

Current behavior 😯

when the dialog is opened in Shadow DOM the scrollbar is removed (overflow: hidden) however the space is not filled with a padding-right, as it is done for a dialog outside of shadow DOM, see the column of text on the right on the example:

image

Expected behavior πŸ€”

the scrollbar's width must be replaced with a padding-right

Context πŸ”¦

We use Shadow DOM to isolate styles for components we inject into other pages. We add a component on a long page that opens a dialog, but because its added in a Shadow DOM it doesn't replace the scrollbar width with padding so there's a jump on the page.

here's where I think the issue is happening in MUI: In ModalManager isOverflowin function returns false because container is not the same as body and both its clientHeight and scrollHeight are 0

https://github.com/mui/material-ui/blob/64c48b50612fec38d29606868de45ce4ebb09019/packages/mui-base/src/unstable_useModal/ModalManager.ts#L12-L20

so handleContainer will not add paddingRight https://github.com/mui/material-ui/blob/64c48b50612fec38d29606868de45ce4ebb09019/packages/mui-base/src/unstable_useModal/ModalManager.ts#L101-L123

Your environment 🌎

npx @mui/envinfo ``` System: OS: Windows 10 10.0.22621 Binaries: Node: 18.18.0 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: 118.0.5993.117 Edge: Chromium (118.0.2088.69) npmPackages: @emotion/react: 11.11.1 @emotion/styled: 11.11.0 @mui/base: 5.0.0-beta.21 @mui/codemod: 5.14.15 @mui/core-downloads-tracker: 5.14.15 @mui/docs: 5.14.15 @mui/envinfo: 2.0.12 @mui/icons-material: 5.14.15 @mui/internal-waterfall: 1.0.0 @mui/joy: 5.0.0-beta.12 @mui/lab: 5.0.0-alpha.150 @mui/markdown: 5.0.0 @mui/material: 5.14.15 @mui/material-next: 6.0.0-alpha.107 @mui/private-theming: 5.14.15 @mui/styled-engine: 5.14.15 @mui/styled-engine-sc: 6.0.0-alpha.3 @mui/styles: 5.14.15 @mui/system: 5.14.15 @mui/types: 7.2.7 @mui/utils: 5.14.15 @mui/x-charts: 6.0.0-alpha.15 @mui/x-data-grid: 6.16.2 @mui/x-data-grid-generator: 6.16.2 @mui/x-data-grid-premium: 6.16.2 @mui/x-data-grid-pro: 6.16.2 @mui/x-date-pickers: 6.16.2 @mui/x-date-pickers-pro: 6.16.2 @mui/x-license-pro: 6.10.2 @mui/x-tree-view: 6.0.0-alpha.1 @mui/zero-jest: 0.0.1-alpha.0 @mui/zero-next-plugin: 0.0.1-alpha.0 @mui/zero-runtime: 0.0.1-alpha.0 @mui/zero-tag-processor: 0.0.1-alpha.0 @mui/zero-vite-plugin: 0.0.1-alpha.0 @types/react: ^18.2.32 => 18.2.33 react: 18.2.0 react-dom: 18.2.0 styled-components: 6.0.8 typescript: ^5.1.6 => 5.1.6 ``` this was tested in Chrome
michaldudak commented 11 months ago

@arminbashizade, thanks for the report. Would you like to create a PR with a fix?

radist2s commented 6 months ago

@michaldudak, I have encountered a similar problem as @arminbashizade. According to documentation, container is the way to set <Dialog/>'s root when using Shadow DOM. The problem, as I see it, is the impossibility of controlling which scrollbar to lock when using a custom container:

https://github.com/mui/material-ui/blob/c9bef2cac914722a1ceb60894f2a104a4c209d88/packages/mui-base/src/unstable_useModal/ModalManager.ts#L125-L139

The only way to solve the problem is to use disableScrollLock and try to implement custom scrollbar locking logic, but this is not reliable.

@michaldudak, if <Modal/> would accept something like scrollContainer as a property, that would solve the problem. However, I realize that extending the API without a dire need is unwise. Maybe you can suggest how to solve the problem differently? I could prepare a PR.

michaldudak commented 6 months ago

We're not adding any new features to this version of @mui/base. We moved the development of the library to the new repo, and we're working on changing the components' API there. When we get to the Modal, we'll consider this issue.

TamirCode commented 6 months ago

I have an issue when using LightGallery's carousel component along with MUI Dialogs on the same page, opening the MUI dialog causes scroll to jump to bottom of page When using modal component instead, the scroll jumps to the Carousel component

michaldudak commented 6 months ago

@TamirCode, please open a new issue and provide a reproduction.

TamirCode commented 6 months ago

@TamirCode, please open a new issue and provide a reproduction.

it seems to be an issue with lightgallery package rather than mui, my mistake. thanks