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

[ModalManager] Dialog in Shadow DOM creates scroll jump

Open arminbashizade opened this issue 2 years ago • 6 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the 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

arminbashizade avatar Oct 27 '23 15:10 arminbashizade

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

michaldudak avatar Nov 24 '23 13:11 michaldudak

@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.

radist2s avatar Apr 25 '24 23:04 radist2s

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.

michaldudak avatar May 02 '24 07:05 michaldudak

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

TamirCode avatar May 10 '24 13:05 TamirCode

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

michaldudak avatar May 13 '24 06:05 michaldudak

@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

TamirCode avatar May 13 '24 09:05 TamirCode