Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Modal] Issue with overflow-y: scroll on <html> #34513

Open
nuanyang233 opened this issue Sep 29, 2022 · 12 comments
Open

[Modal] Issue with overflow-y: scroll on <html> #34513

nuanyang233 opened this issue Sep 29, 2022 · 12 comments
Assignees
@michaldudak
Labels
accessibility a11y component: modal This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@nuanyang233
Copy link

nuanyang233 commented Sep 29, 2022 •
edited by oliviertassinari
Loading

Summary 💡

Add scrollContainer to support custom scroll lock containers.

Examples 🌈

Our product is a browser plugin that enables some features on social platforms. We have implemented some features on Twitter and used dialog. But since twitter implements overflow on the html node.

image

Motivation 🔦

However, scrollLock currently only works on the body node. And developers cannot customize the nodes that require scroll lock.

So I implemented a simple patch.
@nuanyang233 nuanyang233 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 29, 2022
@mnajdova mnajdova added new feature New feature or request component: modal This is the name of the generic UI component, not the React module! labels Sep 29, 2022
@Jack-Works Jack-Works mentioned this issue Oct 10, 2022
Closed
1 task
@michaldudak
Copy link
Member

Could you please provide a simple codesandbox showing the problem so we can reproduce the issue and verify any potential solutions?

@michaldudak michaldudak added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 14, 2022
@nuanyang233
Copy link
Author

Well, I guess I don't know how to provide an example, it's really simple, I just want to use <html> node as a container, like the pull request submitted by Jack works that adds a scrollContainer prop to support custom container.

@github-actions
Copy link

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

@oliviertassinari
Copy link
Member

This should already be supported, I did the work in #17972.

@Jack-Works
Copy link
Contributor

This should already be supported, I did the work in #17972.

I think it didn't solve our's problem. We're already using the mui version that contains #17972 but it didn't help. I opened a PR in #34697

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 29, 2022 •
edited
Loading

A reproduction that demonstrates that it already works: https://codesandbox.io/s/nervous-sutherland-ism1ch?file=/demo.tsx

@oliviertassinari oliviertassinari changed the title Add custom scrollContainer for ModalManager [Modal] Add custom scrollContainer for ModalManager Oct 29, 2022
@oliviertassinari oliviertassinari changed the title [Modal] Add custom scrollContainer for ModalManager [Modal] Issue with overflow-y: scroll on <html> Oct 29, 2022
@github-actions
Copy link

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information and removed status: waiting for author Issue with insufficient information labels Oct 29, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 29, 2022 •
edited
Loading

I suspect that If you have an extension, the container of the portals/modals can't be the <body>, you have to set something that is a bit deeper in the DOM tree. However, the Modal relies on the container to be the <body> to handle the <html> overflow case, hence it breaks. If you can provide a reproduction, it would be great!

Having a new scrollLockContainer prop could make sense. We would only need to make sure that it still makes sense with the need to set the rest of the page inert:

Screenshot 2022-10-29 at 18 22 28

https://www.w3.org/WAI/ARIA/apg/patterns/dialogmodal/

and that it also makes sense to disable the scroll on something that isn't the <body>/<html>, because the alternative fix could be to traverse the tree up to find these elements, instead of using the container and needing scrollLockContainer.

Anyway, I'm leaving this discussion here. I have only engaged here because I was fixing the bot that handles
Screenshot 2022-10-29 at 18 27 07 with cfb7770. Handling this is no longer in my scope.

@github-actions
Copy link

github-actions bot commented Nov 5, 2022

Since the issue is missing key information and has been inactive for 7 days, it has been automatically closed. If you wish to see the issue reopened, please provide the missing information.

@github-actions github-actions bot closed this as completed Nov 5, 2022
@github-actions github-actions bot closed this as completed Nov 5, 2022
@mui mui deleted a comment from github-actions bot Nov 6, 2022
@Jack-Works
Copy link
Contributor

hello! @oliviertassinari please re-open this issue please! I have made a partially reproduction environment here:

https://codesandbox.io/s/blissful-bassi-0bjynp

There are two problems with the current implementation in our environment:

  • The padding-right is not added to body or html.
  • I need overflow: hidden; to be added on the HTML element, instead of the body element (note: you can try to open the Twitter timeline, scroll some pages, and then add overflow: hidden; on the body element, you will find the whole list disappears)

Adding a scrollContainer can solve this problem, thanks!

@oliviertassinari
Copy link
Member

I can reproduce the issue: https://codesandbox.io/s/vigilant-glitter-7i5plg?file=/src/index.tsx

@oliviertassinari oliviertassinari removed the status: waiting for author Issue with insufficient information label Nov 21, 2022
@triglian
Copy link

@oliviertassinari we also need specific styles in the root html that affect overflow behavior.

Our use case is to avoid jumping the content when the content is long enough to add a scrollbar. We have the content of the website to be center aligned with margin-left and -right set to auto.

Our css looks a bit like this:

/* ================== Scrollbars ================== */
/*
 * Always show the scrollbar to allocate the space.
 * Safari will respect this only if in the OS X settings the user
 * has selected "Show scroll bars: Always"
*/
html {
  overflow-y: scroll;
}

/*
 * Chrome, Edge and firefox will support this and when it overflows
 * the gutter will be stable
 * https://caniuse.com/mdn-css_properties_scrollbar-gutter
 */
@supports (scrollbar-gutter: stable) {
  html {
    overflow-y: auto;
    scrollbar-gutter: stable;
  }
}

/*
 * Chrome and Edge will support this and when it overflows
 * the gutter will be auto and the scroll bar will be an overlay
 * https://caniuse.com/css-overflow-overlay
 */
@supports (overflow-y: overlay) {
  html {
    overflow-y: overlay;
    scrollbar-gutter: auto;
  }
}

The issue with this is that when a <Menu /> component sets the body overflow to hidden, the scroll lock feature breaks. If the overflow: hidden was applied to the html element it would solve our issue.

Do you have any suggestions?

(We also have a mui-x license but I think it doesn't apply here)

@DiegoAndai DiegoAndai added this to the Material UI with Base UI milestone Mar 14, 2024
@oliviertassinari oliviertassinari removed this from the Material UI with Base UI milestone Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@michaldudak michaldudak

Labels
accessibility a11y component: modal This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[base-ui][material-ui][joy-ui][Modal] Add scrollLockContainer prop
7 participants
@triglian @oliviertassinari @mnajdova @michaldudak @Jack-Works @DiegoAndai @nuanyang233