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

[Popper] Position shift when the scroll is blocked #37207

Closed
2 tasks done
cherniavskii opened this issue May 8, 2023 · 8 comments
Closed
2 tasks done

[Popper] Position shift when the scroll is blocked #37207

cherniavskii opened this issue May 8, 2023 · 8 comments
Assignees
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! component: Popper The React component. See <Popup> for the latest version. docs Improvements or additions to the documentation

Comments

@cherniavskii
Copy link
Member

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/loving-fast-7scfjf?file=/demo.tsx

Steps:

  1. Open the Popper
  2. Open the Select

Current behavior 😯

When Popper has *-end placement, its position shifts when the scroll is blocked:

Screen.Recording.2023-05-08.at.12.09.01.mov

Expected behavior 🤔

Popper's position should not shift (it doesn't when the placement is changed to *-start):

Screen.Recording.2023-05-08.at.12.09.14.mov

Context 🔦

We noticed this in MUI X mui/mui-x#6619 (comment)

The issue can be solved with the mui-fixed className applied to the Popper:

<Popper className="mui-fixed" />

Demo: https://codesandbox.io/s/hidden-breeze-92jzki?file=/demo.tsx

But we can make this fix unnecessary by applying mui-fixed automatically when the placement is set to *-end.

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.
@cherniavskii cherniavskii added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 8, 2023
@zannager zannager added the component: Popper The React component. See <Popup> for the latest version. label May 8, 2023
@michaldudak michaldudak added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 18, 2023
@michaldudak
Copy link
Member

Thanks for reporting this. I'll look into it.

@michaldudak
Copy link
Member

The logic would have to be more complicated than just adding mui-fixed class when placement is *-end, especially if you consider RTL rendering. Also, the auto placement caused some issues when I was testing it, so I think leaving it as is, with the recommendation in https://mui.com/material-ui/getting-started/faq/#why-do-the-fixed-positioned-elements-move-when-a-modal-is-opened won't accidentally break anything.

@cherniavskii
Copy link
Member Author

I think leaving it as is, with the recommendation in mui.com/material-ui/getting-started/faq/#why-do-the-fixed-positioned-elements-move-when-a-modal-is-opened won't accidentally break anything.

Yeah, my main concern here is the fix is not obvious at all 🙂

Also, the auto placement caused some issues when I was testing it

Out of curiosity, what issues did it cause?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 31, 2023

The issue can be solved with the mui-fixed className applied to the Popper:

This sounds to me like the correct way to fix this. I'm not aware of anything else needed. But yes, maybe the Popper should be smart about it and be able to reposition itself automatically, so a bug?

I think that there could be a docs issue as well, maybe we would want to remove as much as possible from https://mui.com/material-ui/react-modal/ and link back to https://mui.com/base/react-modal/#overflow-layout-shift starts to explain the solution. A demo there would be nice as well.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: modal This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. and removed component: Popper The React component. See <Popup> for the latest version. bug 🐛 Something doesn't work labels May 31, 2023
@michaldudak
Copy link
Member

@cherniavskii, the new Popup from Base UI does not suffer from this issue (unless I missed something): https://codesandbox.io/s/friendly-noyce-dd3r2d?file=/demo.tsx
Could you please verify?

@cherniavskii
Copy link
Member Author

@michaldudak Yes, I confirm that the Popup component does not suffer from this issue: https://codesandbox.io/s/bold-lamarr-zvh75k?file=/demo.tsx
(I had to update the import statement to make the demo work)

@michaldudak
Copy link
Member

Would using the Popup instead of Popper be a good workaround in your use case? We're planning to deprecate the Popper in the future, so I'm not keen on investing time to fix its bugs.

@cherniavskii
Copy link
Member Author

We're currently using the className="mui-fixed" workaround, but in the future, we can migrate to the Popup as they seem to be easily replaceable 👍🏻
Feel free to close this issue 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! component: Popper The React component. See <Popup> for the latest version. docs Improvements or additions to the documentation
Projects
None yet
Development

No branches or pull requests

4 participants