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.

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

[material-ui][Popover] The disableScrollLock prop is causing issues with CSS and additional scrolling functionality #42320

Open
loizosv opened this issue May 21, 2024 · 7 comments
Assignees
Labels
component: Popover The React component. package: material-ui Specific to @mui/material support: docs-feedback Feedback from documentation page

Comments

@loizosv
Copy link

loizosv commented May 21, 2024

See Live example to reproduce

Link to live example: CodeSandBox Demo

ReactApp-GoogleChrome2024-05-2119-25-59-ezgif.com-crop-video.mp4

Current behavior

When the Popover is open without the disableScrollLock prop, is not allowing me to scroll through the page. In addition is adding padding-right and overflow:hidden to the body tag.

To prevent additional CSS from being apply to the body tag, I used disableScrollLock and also allows me to scroll on the page.
However this solution creates a second issue: **When the popover is active and the user still scrolls at a point where the button is not visible anymore, the popup remains open which i don't want **

I tried adding a useEffect hook to detect when the user scrolls more that 50 px and force the popover to close. Although it works it creates a 3rd issue where the popover is translated (moved) to the bottom left corner of the page for less than a second and then disappears.

Expected behavior

The required functionality is as follows:

  • The user clicks on the button to open the popover.
  • The user will be able to scroll while popover is open.
  • An open popover should not apply any css to the body tag.
  • When user scrolls more than 50px the popover should close (on spot, not at left bottom corner)
  • When user clicks somewhere else on the screen the popover should close

Context

I checked all ANCIENT previous questions but cannot find solution. Most questions below are more than 6-7 years old. There should be a solution by now. Don't say is duplicate if the duplicate question doesn't even have a reply with a valid structured answer.

Your environment

No response

Search keywords: popover, scroll, disableScrollLock

@loizosv loizosv added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 21, 2024
@zannager zannager added the component: Popover The React component. label May 22, 2024
@mnajdova
Copy link
Member

It would work if you use the Popper component, e.g. https://codesandbox.io/p/sandbox/popover-issue-forked-7fxkvy?file=%2Fsrc%2FMyPopover.jsx%3A10%2C24. Is there a specific reason for using Popover vs Popper? Check the docs page for more details: https://mui.com/material-ui/react-popper/

@mnajdova mnajdova 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 May 23, 2024
@loizosv
Copy link
Author

loizosv commented May 23, 2024

SOLUTION 1: Add Transition Duration on the Popover

One solution is to add transitionDuration={0} on the Popover so the animation duration is 0 which is preventing the popover from moving to the bottom corner.
Solution 1 Demo

SOLUTION 2: Replace Popover with Popper

Popover also works and does not require the transitionDuration. However it requires additional code to implement onClickAway listener, and implement additional CSS to make it look better
Solution 2 Demo

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels May 23, 2024
@mnajdova
Copy link
Member

Considering the number of issues you linked, it seems like this is common problem for people. It could be valuable to add a demo to teach people what are the options when facing these kinds of issues. cc @samuelsycamore @danilo-leal what do you think?

@mnajdova mnajdova added support: docs-feedback Feedback from documentation page and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 24, 2024
@danilo-leal danilo-leal changed the title Popover prop disableScrollLock is causing issues with css and additional scrolling functionality. [material-ui][Popover] The disableScrollLock prop is causing issues with CSS and additional scrolling functionality May 24, 2024
@danilo-leal danilo-leal added the package: material-ui Specific to @mui/material label May 24, 2024
@danilo-leal
Copy link
Contributor

Yeah, agreed! I think we have some improvement opportunities as far as documentation for both of these components:

  • We don't seem to address the use case for Popover vs. Popper; what's the difference? When to use one vs. the other?
  • There's no demo for the prop in question here (disableScrollLock)
  • There's no demo/information about the Popover when in a scroll context (but there's a section/very thorough—and maybe too complex—Scroll demo playground for the Popper instead) which could address the desired behavior here

I also wonder if there's room to change how the disableScrollLock affects the Popover positioning or if we need another prop for that altogether. Like, if the prop is on, I can understand the expectation of the popover getting out of the viewport as you scroll past the trigger (it seems like this is Radix's default behavior, and we can change that with their hideWhenDetached prop, if I'm getting it right). But I can also understand the opposite (current behavior). In any case, the moving to the left corner behavior does seem like a bug and these are definitely things we'll need to address on Base UI 👍

@loizosv
Copy link
Author

loizosv commented May 24, 2024

Hi thanks @mnajdova and @danilo-leal for recognizing this issue.

In my hamble opinion i think is unnecessary to have these components separate. They are both kind of popup windows which allow you to display information, of which their behavior changes based on the props passed to them. The difference i can find is based mainly on aesthetics.

Eg, popper is simple with no animation and popover is animated and more interactive.
popper doesn't handle click away events while popper has
popover prevents page scroll while popper does.
...

So there is a kind of true-false relationship of these functionalities on which you can address by adding more props to their APIs, or in my opinion one final component/API

@mnajdova
Copy link
Member

So there is a kind of true-false relationship of these functionalities on which you can address by adding more props to their APIs, or in my opinion one final component/API

It's very likely that we will consolidate the API in the future, especially after we migrate to Floating UI (Base UI has already done it, so once we depend on it, this change will happen).

@Holybasil
Copy link

It’s weird that the disableScrollLock of Popover isn’t working as expected as the default value is false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popover The React component. package: material-ui Specific to @mui/material support: docs-feedback Feedback from documentation page
Projects
None yet
Development

No branches or pull requests

5 participants