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] When placement is set to "right" or "left", placement doesn't update based on viewport position #22708

Closed
2 tasks done
caitecoll opened this issue Sep 23, 2020 · 4 comments · Fixed by #22748
Closed
2 tasks done
Labels
component: Popper The React component. See <Popup> for the latest version. external dependency Blocked by external dependency, we can’t do anything about it ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@caitecoll
Copy link

When placement is set to any value beginning with top or bottom, the placement will update dynamically if there isn't sufficient vertical space on the screen to place the Popper in that position. This is not true for any right or left placements. Ideally, right-start could dynamically update to right-end, etc.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

If placement value begins with right or left, vertical placement does not update based on screen position.

Expected Behavior 🤔

Placement updates dynamically to attempt to keep the popper visible to the user. For instance, when placement is set to right-start, but the anchor is at the bottom of the screen, placement will update to right-end.

Steps to Reproduce 🕹

codesandbox

Steps:

  1. Scroll until "Toggle Popper" button is at the bottom of the screen.
  2. Click the button to open the popper.
  3. Note that the popper is not fully visible.

Context 🔦

We use this popper (with placement="right-start") to allow users to select from a list of actions. The popper can be up to 340px tall, and in many cases most of it is rendering offscreen. Dynamic vertical placement would help considerably.

Tech Version
Material-UI 4.11.0
React 16.10.0
Browser Chrome
TypeScript 3.8.3
@caitecoll caitecoll added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 23, 2020
@oliviertassinari oliviertassinari added component: Popper The React component. See <Popup> for the latest version. external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 23, 2020
@oliviertassinari
Copy link
Member

@caitecoll Have a look at https://github.com/popperjs/popper-core, it's the positioning engine we use.

@caitecoll
Copy link
Author

caitecoll commented Sep 24, 2020

@oliviertassinari Thanks for the link. I'm looking at the second demo ("Overflow prevention") on that library's demo site, and it seems like placement="right" is working correctly there. Is there something extra that I need to configure or override on MaterialUI's Popper in order for it to work like that demo?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 24, 2020

The following fixes the issue:

<Popper
  id={id}
  open={open}
  anchorEl={anchorEl}
  placement="right-start"
  modifiers={{
    preventOverflow: {
      boundariesElement: "viewport"
    }
  }}
>

However, the implementation doesn't match the comment:

https://github.com/mui-org/material-ui/blob/1c2055a95c85b025720f2fe8f2322852d2ed4835/packages/material-ui/src/Popper/Popper.js#L130-L133

I think that we can try:

diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index f5e70dce68..67db3cf431 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -129,7 +129,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
           : {
               // It's using scrollParent by default, we can use the viewport when using a portal.
               preventOverflow: {
-                boundariesElement: 'window',
+                boundariesElement: 'viewport',
               },
             }),
         ...modifiers,

We changed it in #12168 but it was coupled with something else. I can't reproduce the issue.

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Sep 24, 2020
@caitecoll
Copy link
Author

Thanks so much for the fix! I overlooked that modifiers prop in the API documentation. It fixed our issue, and I really appreciate the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Popper The React component. See <Popup> for the latest version. external dependency Blocked by external dependency, we can’t do anything about it ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants