Bug in isOverflowing?

[jasontrigg0]

• This is a v1.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Hello,

I was having an issue similar to #10000 and found the following line in isOverflowing.js

return marginLeft + doc.body.clientWidth + marginRight < win.innerWidth;

Is the sign backwards here? Flipping it solved my problem.

https://github.com/mui-org/material-ui/blob/5eee984cbe4f39f793573354ff170757f37d65cc/packages/material-ui/src/Modal/isOverflowing.js#L24

[oliviertassinari]

Hum, I not too familiar with this logic, I would need to test it out but what we currently have looks very wrong.

[nathanmarks]

@oliviertassinari

No, we don't have this wrong.

This is to check that the body is overflowing vertically. window.innerWidth returns the width of the viewport including the scroll bar, so this code is used to determine if we have a visible scrollbar or not.

[nathanmarks]

@jasontrigg0 Are you still having this issue? Could you provide a demo?

[jasontrigg0]

Thanks for the response, that makes sense. Looking into my issue a bit more I'm seeing the following when a modal is opened:

1. overflow:hidden is added to the modal's immediate parent div, not body
2. So the scrollbar doesn't disappear from body
3. the compensatory overflow padding is still added to various mui-fixed elements through the whole page (ie including some outside the modal's parent div), causing them to jump to the left on the page

Does this make sense? Do you know why (1) might be happening? Can work on pulling out a minimal example if this isn't clear.

[nathanmarks]

Can you tell me why it happens to you but not when I open a modal on the documentation site?

[oliviertassinari]

@jasontrigg0 I gave the logic a second look. It's all right. I have forgotten that I added the margin-left logic 9 months ago.

Could you please provide a full reproduction? On codesandbox would be great.

[jasontrigg0]

Example: https://codesandbox.io/s/3x00l11jpq
Note how clicking the modal causes the red div to jump to the left.

I think the key factor is that the modal has the container prop, and isOverflowing(container) is (incorrectly?) returning true here:
https://github.com/mui-org/material-ui/blob/382b7ce10ac7644a88b0e6575f4f3d38ded02583/packages/material-ui/src/Modal/ModalManager.js#L108

which is perhaps caused by this line evaluating as false?
https://github.com/mui-org/material-ui/blob/046fc982d25662fee7a960a79606a580bf2186d8/packages/material-ui/src/Modal/isOverflowing.js#L15

Let me know!

[oliviertassinari]

@jasontrigg0 I confirm the issue. The issue starts with this property:

<Modal container={document.getElementById("test")} />

What's your use case for this property?

[jasontrigg0]

We're using this toolbar component, which creates a Popover with a container:
https://github.com/gregnb/mui-datatables/blob/master/src/MUIDataTableToolbar.js

[jasontrigg0]

Any progress on this?

[oliviertassinari]

The logic was changed in: #14877. It should be better now.
https://github.com/mui-org/material-ui/blob/08eb5da07c2ebc51c8e543f85bc5b114924fab1b/packages/material-ui/src/Modal/isOverflowing.js#L8-L18