mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
93.05k stars 32.05k forks source link

[Select][material-ui] Don't lock the body scroll and make it non-modal select #17353

Open ghost opened 4 years ago

ghost commented 4 years ago

Current Behavior 😯

When I open a Select component vertical scroll bar of the page disappears. See here scrn

I don't have a demo to reproduce sorry, but I suppose this is a known issue: https://github.com/mui-org/material-ui/issues/8710, https://github.com/mui-org/material-ui/pull/7239, so maybe someone can help.

The select component is located on an app bar on the screenshot.

I tried setting box-sizing: border-box on App bar, no help.

Expected Behavior 🤔

The scrollbar should not disappear.

Your Environment 🌎

Tech Version
Material-UI v3.1.1
React 16.5
Browser Chrome
sjsakib commented 4 years ago

Any update on this issue? I have a select element inside a hero/landing page. When the select menu opens, scroll gets locked. And that causes body to shrink by about .2px. And that causes the background image to jump a bit. I find this very annoying. Anyone have any suggestions to work around this?

sjsakib commented 4 years ago

I've found out that material ui is apply a padding-right of 17px to handle the hidden scrollbar. But the problem is that the scrollbar is actually 17.2px. I've tested on chrome, firefox, and opera

jeronimomora commented 4 years ago

@sjsakib

Quote from eplumecocq for a Select fix

<Select MenuProps={{ disableScrollLock: true }} />

Which means that if you have this problem for a menu, just add disableScrollLock={true} as a prop to Menu

oliviertassinari commented 4 years ago

@jeronimomora Unfortunately, this isn't enough to solve the problem, the position won't be correct. I would also expect the backdrop to be removed, and the aria inert not to be applied.

WholemealDrop commented 4 years ago

Following, also experiencing this with Select component.

| Material-UI | v3.9.2 | | React | v16.9.0 | | Browser | chrome |

tactycHQ commented 4 years ago

@sjsakib

Quote from eplumecocq for a Select fix

<Select MenuProps={{ disableScrollLock: true }} />

Which means that if you have this problem for a menu, just add disableScrollLock={true} as a prop to Menu

While this seems to works for the Select component, React throws the following warning. I suspect this is because there is no child Menu compnent under Select. Select only has the Menuiitem components as children.

"Warning: React does not recognize thedisableScrollLockprop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercasedisablescrolllockinstead. If you accidentally passed it from a parent component, remove it from the DOM element."

TomPradat commented 4 years ago

Any updates ? I also have the problem with version 4.8.2 when displaying drawers, menus. The disableScrollLock works for me on the Menus but this is not a satisfying solution IMO.

BrentGrammer commented 4 years ago

I'm having a scroll bar issue with the Select component as well - the scroll bar appears when hovering over it and disappears on mouse leave. It causes a janky visual effect. Setting overflow-y to hidden on all sorts of elements doesn't fix it. Anyone else run into this and solve it?

TomPradat commented 4 years ago

Personnaly i've locked the version on 4.6.x, you can try the disableScrollLock prop trick else

Dito-Orkodashvili commented 4 years ago

In my case this problem is caused by Tooltip component. It is a bit strange issue.

if I use it like this: `const ActionButton = ({ tooltip }) => (

defkev commented 4 years ago

Seeing the same behavior as @Dito-Orkodashvili with a <Button> wrapped by a <Tooltip> toggling a <Menu>

If the box model is close to showing the vertical scrollbar, opening the menu will add the padding-right:17px scrollbar to the body though there is no scrollbar moving the body 17px to the left.

As a workaround set the tooltip's title to nothing when the <Menu> is opened:

<Tooltip title={this.state.menuAnchor ? '' : 'The Tooltip'}>

@material-ui/core 4.9.5

thenderson55 commented 3 years ago

This is still an issue with @material-ui/core 4.11.0. When I open a nav menu or popover it adds: padding-right: 10px

disableScrollLock={true} fixes it but is a terrible solution since people can then scroll which seems very unnatural

benpryke commented 3 years ago

I added box-sizing: border-box to body so that at least the added padding-right no longer causes the page content to stretch when the scrollbar is hidden, though there is now an empty space down the side of the page when the menu is open. Most users probably won't notice, but some will.

peterlu-hinter commented 3 years ago

The whole Modal -> Popover -> Menu -> Select approach is a mistake (excuse my French). Select shouldn't be a modal. Please check Microsoft Fluent UI https://developer.microsoft.com/en-us/fluentui#/controls/web/dropdown I really don't like everything is based on Modal and the hack to body overflow: hidden or padding-right. It is a patch on top of another patch. Guys, can we properly implement this basic component "select dropdown"? Coz we are going to build more complex components on top of this. The whole JSS approach in MUI is great. However the basic component is not done properly, which will scare dev team away. No matter how I can fix or add properties into this Select Dropdown, the client is not happy. The dropdown should be position: absolute to the relative parent select input. Do not inherit Angular Material design, their approach is wrong. Google is redesigning its material anyway :).

Just imaging how I can explain to my junior dev about <Select MenuProps={{ disableScrollLock: true }} /> Juniors will ask, why do we have to add this MenuProps to every single select dropdown. It is not DRY. And what is this anyway?

Trust me, I have tried this Material UI Select dropdown on 3 projects. They all fail.

About padding-right on the body. We are trying to build next.js + MUI with straps on pages. The strap is like a full-screen div. Every website has that. Imagine you put a padding-right on the body. It will destroy all my straps. Now all straps will have a blank padding right. Don't tell me I have to add padding-right fix on every straps or put a "mui-fixed" class:) This is just polluting the code base. I have read the modal manager class of MUI, and the overflow:hidden and padding-right bit does not sell well to me :).

TomPradat commented 3 years ago

Just in case this helps, in my team we've fixed this by adding the following css :

html {
    overflow: hidden;
}

See https://github.com/mui-org/material-ui/issues/19984

artyom-88 commented 3 years ago

The issue is still actual for me

artyom-88 commented 3 years ago

Just in case this helps, in my team we've fixed this by adding the following css :

html {
    overflow: hidden;
}

See #19984

In this case you won't have any scrolls at all

TomPradat commented 3 years ago

Just in case this helps, in my team we've fixed this by adding the following css :

html {
    overflow: hidden;
}

See #19984

In this case you won't have any scrolls at all

Well you can let the body overflow but I agree this is not ideal

artyom-88 commented 3 years ago

@oliviertassinari do you have any advice how to fix that? If I use MenuProps={{ disableScrollLock: true }}, the menu will have improper position after scroll image https://codesandbox.io/s/suspicious-wozniak-847u9?file=/src/App.js:594-633

sanderderks commented 3 years ago

In my case padding-right 17px is added to the MenuList component, not to the right of the page (where the scrollbar would appear) and I can't find any way to work around this. image

Edit I solved this problem by adding this https://github.com/mui-org/material-ui/issues/10000#issuecomment-394837390 to the root class of the MenuList component.

maksimgm commented 3 years ago

@oliviertassinari I'm using "@material-ui/core": "4.9.10", and am still seeing this issue. Per @Artyom-Ganev comment, setting {{ disableScrollLock: true }} does allow scrolling to be enabled, however, the Menu component's position is not fixed. Any advice on the best approach to this issue?

Dev-Tarek commented 3 years ago

My solution to this problem is:

  1. With CSS use a custom scrollbar for the body with a given $scrollbar-width value
  2. Set overflow: overlay; for the body
  3. Add padding-right: $scrollbar-width; to the body

This way you will have a fixed padding to the body so that when the scrollbar is hidden the padding won't be affected.

However, you will have an extra padding problem with small height views that don't render the scrollbar, you can do a work around like applying the style in [3] padding-right: $scrollbar-width; conditionally.

I implemented it using ResizeObserver:

const resizeObserver = new ResizeObserver(elements => {
  const element = elements[0].target
  element.style.paddingRight = element.clientHeight > window.innerHeight
    ? element.style.paddingRight = scrollbarWidth
    : element.style.paddingRight = 0
})

// React
function App () {
  useEffect(() => {
    resizeObserver.observe(document.body)
    return () => resizeObserver.disconnect()
  }, [])
  ...
}
DanielRuf commented 3 years ago

I think the logic should be (if disableScrollLock is set to false, which is the default):

Update: we've tested this approach and the closing animation from the Select component still jumps to the top and then our scroll restore logic jumps to the correct position. So this is not ideal and has to be fixed in materialui.

With disableScrollLock the position: fixed is problematic. I guess this was not the case in the past?

an-zaz commented 2 years ago

I guess, one more option is to close menu on scrolling. I omitted all logic except opening and closing

const SelectWrapper = () => {
  const [isOpen, setIsOpen] = useState(false);

  useEffect(() => {
    const handler = () => {
      setIsOpen(false);
    };
    window.addEventListener('scroll', handler);
    return () => {
      window.removeEventListener('scroll', handler);
    };
  }, []);

  const menuProps: Partial<MenuProps> = {
    variant: 'menu',
    disableScrollLock: true,
  };

  return (
      <Select
        open={isOpen}
        onOpen={() => {
          setIsOpen(true);
        }}
        onClose={() => {
          setIsOpen(false);
        }}
        MenuProps={menuProps}
      >
      </Select>
  );
};
enflam3 commented 2 years ago

I guess, one more option is to close menu on scrolling. I omitted all logic except opening and closing

const SelectWrapper = () => {
  const [isOpen, setIsOpen] = useState(false);

  useEffect(() => {
    const handler = () => {
      setIsOpen(false);
    };
    window.addEventListener('scroll', handler);
    return () => {
      window.removeEventListener('scroll', handler);
    };
  }, []);

  const menuProps: Partial<MenuProps> = {
    variant: 'menu',
    disableScrollLock: true,
  };

  return (
      <Select
        open={isOpen}
        onOpen={() => {
          setIsOpen(true);
        }}
        onClose={() => {
          setIsOpen(false);
        }}
        MenuProps={menuProps}
      >
      </Select>
  );
};

This seems to bee good solution for me , but another problem is that if you have more then one [Select] this triggers open all of [Select] . How would I modify it for multiple where only one [Select] can be open at the same time?

VeikkoLehmuskorpi commented 2 years ago

I guess, one more option is to close menu on scrolling. I omitted all logic except opening and closing

const SelectWrapper = () => {
  const [isOpen, setIsOpen] = useState(false);

  useEffect(() => {
    const handler = () => {
      setIsOpen(false);
    };
    window.addEventListener('scroll', handler);
    return () => {
      window.removeEventListener('scroll', handler);
    };
  }, []);

  const menuProps: Partial<MenuProps> = {
    variant: 'menu',
    disableScrollLock: true,
  };

  return (
      <Select
        open={isOpen}
        onOpen={() => {
          setIsOpen(true);
        }}
        onClose={() => {
          setIsOpen(false);
        }}
        MenuProps={menuProps}
      >
      </Select>
  );
};

I guess it's not possible to use this solution with components that utilize the Select component, such as the TablePagination component.

akgupta0777 commented 2 years ago

I encountered this issue due to this my react app got frozen and scrolls got disabled from page. I think select component don't unbind the scroll listeners properly building a memory leak and blocking UI thread.

grantmr commented 2 years ago

I encountered this issue due to this my react app got frozen and scrolls got disabled from page. I think select component don't unbind the scroll listeners properly building a memory leak and blocking UI thread.

I'm also getting this. scrollbars are hiding and not coming back when the select closes. I'm getting it with the Select used in TablePagination and can't pass MenuProps to it. Also happening with Dialog.

akgupta0777 commented 2 years ago

I encountered this issue due to this my react app got frozen and scrolls got disabled from page. I think select component don't unbind the scroll listeners properly building a memory leak and blocking UI thread.

I'm also getting this. scrollbars are hiding and not coming back when the select closes. I'm getting it with the Select used in TablePagination and can't pass MenuProps to it. Also happening with Dialog.

disableScrollLock:true solved my problem.

grantmr commented 2 years ago

disableScrollLock:true works find for Select but I was also getting it on the TablePagination component which has a Select that you can't pass disableScrollLock to. I ended up just overriding the padding and overflow on the body with important, as it didn't affect the app

noblessem commented 2 years ago

Guys i am just a junior programmer but ive found one solution, u can just pass MenuProps={{ style: { maxWidth: 0, maxHeight: 300, position: 'absolute', }, disableScrollLock: true, }}

And that will hold menu items under select, hope it will help)

andrelmchaves commented 2 years ago

Should this issue be closed? It seems this is the normal behavior of the Select component.

withflaxen commented 2 years ago

Guys i am just a junior programmer but ive found one solution, u can just pass MenuProps={{ style: { maxWidth: 0, maxHeight: 300, position: 'absolute', }, disableScrollLock: true, }}

And that will hold menu items under select, hope it will help)

Suggested solution doesn't work for me.Page scrolls to the top, when I don't pass autoFocus={false} and disableAutoFocus={true}, and if I do then menu appears in different places depending on the current page scroll. How can I attach menu to the bottom of my input and keep page scrolling at the same time?

karthick-cs commented 1 year ago

@noblessem Thanks! your solution worked

satellite-xyz commented 1 year ago

Note that this example is also present on the official doc page: https://mui.com/material-ui/react-menu/

sedatbasar commented 1 year ago

This is because of the calculation in MUI . So the issue that MUI try to fix is to disable the scrolling MUI adds "overflow: hidden" to your body element and if your browser shows scrollbar, then it will have a flickering effect. Because while adding the overflow:hidden, the scrollbar will also get hidden and ur page will shift to right. So to prevent this MUI is adding the padding to body element to not have that flickering effect. For me following workaround is working well.

html {
  overflow: hidden
}

body {
  overflow: auto !important;
  max-height: 100vh;
}

So with this we are forcing our window.innerWidth and document.documentElement.clientWidth to have same value so that MUI dont add that padding while keeping our scrolling behavior. But now scrolling will work on body element so you should use it carefully since it might have unexpected behaviors. (For example if you are using react-router with ScrollRestoration it might broke that)

AChangXD commented 1 year ago

Why can't <Select/> replicate the behavior of <Autocomplete/>? <Autocomplete/>'s dropdown works pefectly fine while scrolling either inside of the dropdown or outside of the page, the dropdown also doesn't stay fixed in place. @oliviertassinari

Edit: Nevermind, saw a different issue about re-writing this. Is there an ETA on this? '' is nice for multiselect but it does not work well on Mobile (The keyboard always pop up no matter what you do).

krnl0138 commented 1 year ago

I've spent 4 hours on this weird behaviour. This is deeply messed up as @peterlu-hinter correctly summarised. The issue is being opened for 4 years and its priority is set to 'important'. Is there any progress? Maybe you could at least point out this quirkiness in the docs? I believe this may be an expected behaviour for MUI, but not for devs and users.

@sedatbasar 's solution works fine, though. Don't know if there is any major implications out of it. This way, however, disableScrollLock: true isn't working anymore for me in MenuProps in a TextField component. Definitely can be misleading in large projects, since I suppose it won't work for anything that is popover-based.

jovicheng commented 1 year ago

If you want the disableScrollLock: true props and don't want the body misalignment with layout, you can try setting box-sizing: content-box; with body tag, it works for me!

DiegoAndai commented 12 months ago

Hi everyone! Thanks for your patience.

We've added a workaround for this use case in https://github.com/mui/material-ui/pull/37773, released in version 5.14.7. You can now turn off the Select component's scroll lock and margin threshold. To do so, set the MenuProps properties disableScrollLock to true and marginThreshold to null:

<Select
  MenuProps={{
    // disable scroll lock
    disableScrollLock: true,
    // allow the menu to go outside the window
    marginThreshold: null
  }}
/>

Here's a CodeSandbox example of this.

Doing so on a TextField component with the select prop enabled is also possible. To do so, provide the same MenuProps properties through the SelectProps prop. Here's a CodeSandbox example of this.

I'll close this issue as this workaround enables the scroll behavior when the select is open. If you have any questions, don't hesitate to leave a comment. Feel free to open a new issue if you encounter any problems with the workaround.

oliviertassinari commented 12 months ago

@DiegoAndai your demo looks great, but I think this issue is about having a non-modal select, per https://github.com/mui/material-ui/issues/17353#issuecomment-727710338.

In https://codesandbox.io/s/issue-17353-scrollable-text-field-select-forked-8hq5xr, there is still aria-hidden=true on all the elements of the page, and most importantly, can't focus on the input in one click it takes two.

DiegoAndai commented 12 months ago

@oliviertassinari The original intent of this issue is that when Select opens, it shouldn't lock the page's scroll. We won't make that change in v5 as it's a breaking change, so adding a workaround to achieve it is the next best thing we can do.

Keeping this issue open is not useful IMO. It has a lot of comments and opinions, so it's hard to read and understand the scope. I propose closing it.

I agree that there is value in this issue's comments, like https://github.com/mui/material-ui/issues/17353#issuecomment-727710338. The Select component is next in line to be migrated to use Base UI for v6. For that work we should keep these in mind cc: @mj12albert

mnajdova commented 12 months ago

@DiegoAndai your demo looks great, but I think this issue is about having a non-modal select, per https://github.com/mui/material-ui/issues/17353#issuecomment-727710338.

In https://codesandbox.io/s/issue-17353-scrollable-text-field-select-forked-8hq5xr, there is still aria-hidden=true on all the elements of the page, and most importantly, can't focus on the input in one click it takes two.

Let's create a new issue about this, seems like it is a bit different than the original issue. @DiegoAndai please include the original comment too. We can generalize the issue to all components that use modals, to allow a modal and non-modal options. I have partially covered this in https://github.com/mui/material-ui/issues/38630

DiegoAndai commented 12 months ago

I created a new issue to track the non-modal select option for v6: https://github.com/mui/material-ui/issues/38756.

Do you think we can close this one now, @oliviertassinari?

oliviertassinari commented 11 months ago

I think that this issue is a subset of #38756 (cover more components). I mean, I fail to understand why people would land here to only ask to allow the scrollbar and not to allow focus on the page too, or keeping any modal behavior. If we close this one, we would lose the 👍, I think it's better open for visibility.

samueltesfayegari commented 4 days ago

added this

body { overflow: auto !important; }

To overcome any CSS or JavaScript preventing the body from scrolling or locking the scroll bar.

But I ended up with the MUI offset of the right margin of the screen, like it fluctuates to the opposite.

I made that to stop the fluctuation from the scrollbar hiding, but MUI still got another offset.