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

Bad scrollbar replacement on non-integer width windows #29681

Closed
kremerd opened this issue Nov 15, 2019 · 10 comments · Fixed by #30690 or #30772
Closed

Bad scrollbar replacement on non-integer width windows #29681

kremerd opened this issue Nov 15, 2019 · 10 comments · Fixed by #30690 or #30772

Comments

@kremerd
Copy link

kremerd commented Nov 15, 2019

Bug description:

When a modal is shown on a scrollable page, the scrollbar is hidden and the page body is padded to compensate for the lost width. Depending on display settings and the browsers zoom level, this padding is sometimes applied, even when the page body is not scrollable and there is no scrollbar to begin with.

So far as I can tell this happens, when the value right = document.body.getBoundingClientRect().right rounds up, i.e. if right - Math.floor(right) > 0.5. You can check it yourself on this demo site (full Stackblitz).

Note that in order for the value to be non-integer, the page must be rendered in a non-native resolution. This can be done by zooming in or out of the page, or by scaling the display in system settings like these in Windows.

Tested browsers:

I tested and reproduced this bug on Windows 10 with the following browsers:

  • Firefox 70.0.1
  • Chrome 78.0.3904.97
  • Edge 44.18362.449.0
  • Internet Explorer 11.476.18362.0

Related issues:

As #28101 this issue regards the padding introduced to compensate for scrollbars. While in that issue padding is applied when the scrollbar cannot be hidden, this issue is about padding that is applied when no scrollbar was shown to begin with.

I originally posted this at ng-bootstrap/ng-bootstrap#3448, but now realised that the same issue occurs in the plain Bootstrap implementation as well (and for the same reason).

Suggested fix

Some rounding must be applied in the method _checkScrollbar to accomodate for non-integer values:

  _checkScrollbar() {
    const rect = document.body.getBoundingClientRect()
    this._isBodyOverflowing = Math.round(rect.left + rect.right) < window.innerWidth
    this._scrollbarWidth = this._getScrollbarWidth()
  }

This works for the scenarios I tested. Still, I don't understand why rect.left is added here, so maybe I'm missing something...

@XhmikosR
Copy link
Member

/CC @Johann-S

@kremerd
Copy link
Author

kremerd commented May 3, 2020

@Johann-S @XhmikosR Thanks a lot for working on this issue! Unfortunately I don't believe the implementation in #30690 solved it just yet. In fact when I debugged the original StackBlitz and injected the rounding from the PR, padding was still applied, although no scrollbar was present to begin with.

Concretely, the first setup I came up with had left === 0, right === 1045.800048828125, window.innerWidth === 1046, and no scrollbars (Firefox 75 on Windows). In this case

this._isBodyOverflowing = Math.floor(left + right) < window.innerWidth

is true, and therefore padding was applied.

I'm not sure if Math.round would really work in all cases, in particular I don't understand your concerns about equal values being rounded up in #30017. In this setup it would work, though. A more flexible approach might be to explicitly handle that rounding threshold:

const overflowingWidth = window.innerWidth - left - right;
this._isBodyOverflowing = overflowingWidth > 0.5; // or 1?

@XhmikosR
Copy link
Member

XhmikosR commented May 4, 2020

@kremerd can you please make a patch against master and ideally add a test case too? @Johann-S did try the solution before we landed it, but we might missed something. Thanks!

@XhmikosR XhmikosR reopened this May 4, 2020
@kremerd
Copy link
Author

kremerd commented May 5, 2020

@XhmikosR: Of course, thanks for asking! I'll look into it on the weekend.

Meanwhile, maybe you can help me understand some of the other edge cases I'm not seeing here. In #30017 you were concerned that rounding up might lead to a wrong result. Do you have a concrete use case in mind for that?

Also, I'm not sure for which case substracting left is helpful. The only way I can get it non-zero at all is by adding left padding to the html element. And even then the scrollbar width is still just window.innerWidth - right.

@kremerd
Copy link
Author

kremerd commented May 6, 2020

@Johann-S In principal, yes:

  • On the two modal documentation pages I first have to delete a lot of content to make the page small enough that it doesn't require a scrollbar (using devtools). When I open a modal then, padding is inserted to replace the absent scrollbar.
  • On the Stackblitz I cannot reproduce it natively. That's due to left padding on the html element. (Apparently I inserted that yesterday in the original Stackblitz. Sorry for that, it's reverted now.) If I remove the padding, though, I can reproduce it again.

Do I understand you correctly, that you're unable to reproduce the issue yourself? If so, are you able to set "Good size!" widths in the Stackblitz demo? What happens if you open the modal then?

@XhmikosR
Copy link
Member

XhmikosR commented May 6, 2020

I can actually reproduce now on https://stackblitz.com/edit/bootstrap-scrollbar-handling?file=index.html

2020-05-06_16-22-24

We need a test case for sure in our unit tests.

@XhmikosR
Copy link
Member

XhmikosR commented May 6, 2020

So, the question is, can this cause rounding issues? From my tests it can't. Not sure why I thought it could.

Math.round(rect.left + rect.right) < window.innerWidth

That being said, we need some help with adding a test case. I will revert the merged patch and reopen the original PR.

@XhmikosR
Copy link
Member

XhmikosR commented May 7, 2020

For what is worth, I also reproduced without using zoom. My DPI is 175% so maybe that affects things.

@kremerd
Copy link
Author

kremerd commented May 10, 2020

Added another PR for the fix with a test case. (I don't think I could have reused #30017, could I?) Let me know if it needs further revision.

@XhmikosR: Yes, the DPI also affects this. I actually first encountered this without any zoom, but on a screen with 125% display scaling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment