-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[material-ui] Take browser zoom into account when calculating scrollbar size #47408
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
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-47408--material-ui.netlify.app/ Bundle size report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zache Thanks for the PR. The rule is to separate out into two different PRs when doing two changes, even if they're minor.
|
Ok, I figured since they both resolve issues that occur with the scroll lock that they could go in together. |
|
I split out the padding-right change to #47420 |
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zache Can you provide a screen-recording showing the issue? I tried your steps on Windows 10 and Chrome browser but I was unable to reproduce it.
|
Here is a screen recording of the problem: https://sendvid.com/ifpkx74b |
Thanks for the recording. I don't see any AppBar icons shifting when clicking the ⋮ icon in the demo. I do notice a slight page shift when the scrollbar is hidden, but that seems like expected behavior and should be fine. |
|
The problem is caused by Here is a table I made of the actual
As you can see the actual devicePixelRatio is a float, which is inexact, and this means that the current way of calculating the scrollbar width is sometimes slightly off. This leads to the page shifting, something the scroll lock is meant to counter. |
|
@Zache Sounds good. I would let @siriwatknp review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to fix scrollbar size calculation issues that occur at certain browser zoom levels and window widths, which causes horizontal shifts when scroll lock is applied (e.g., when opening modals). The implementation replaces a simple calculation based on window dimensions with a more complex DOM manipulation approach that tries to account for browser zoom using devicePixelRatio.
Key changes:
- Replaces
window.innerWidth - documentElement.clientWidthcalculation with a DOM element creation/measurement approach - Introduces
devicePixelRatio-based calculations to compensate for browser zoom - References older Bootstrap implementation patterns
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const scrollDiv = win.document.createElement('div'); | ||
|
|
||
| scrollDiv.style.setProperty('position', 'absolute'); | ||
| scrollDiv.style.setProperty('top', '-9999px'); | ||
| scrollDiv.style.setProperty('width', '50px'); | ||
| scrollDiv.style.setProperty('height', '50px'); | ||
| scrollDiv.style.setProperty('overflow', 'scroll'); | ||
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | ||
|
|
||
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); | ||
|
|
||
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; | ||
|
|
||
| return zoomedScrollbarWidth; |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation divides by devicePixelRatio, which is incorrect. As devicePixelRatio reflects display density (not browser zoom), this will produce wrong results on high-DPI displays. For instance, on a 2x Retina display with no browser zoom, this would incorrectly halve the scrollbar width. The previous implementation using window.innerWidth - documentElement.clientWidth was the correct approach for measuring scrollbar size and already accounts for zoom naturally.
| const scrollDiv = win.document.createElement('div'); | |
| scrollDiv.style.setProperty('position', 'absolute'); | |
| scrollDiv.style.setProperty('top', '-9999px'); | |
| scrollDiv.style.setProperty('width', '50px'); | |
| scrollDiv.style.setProperty('height', '50px'); | |
| scrollDiv.style.setProperty('overflow', 'scroll'); | |
| // Invert the zoom level to get 100% sized scrollbars for the element | |
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | |
| win.document.body.append(scrollDiv); | |
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | |
| win.document.body.removeChild(scrollDiv); | |
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; | |
| return zoomedScrollbarWidth; | |
| const documentElement = win.document.documentElement; | |
| return win.innerWidth - documentElement.clientWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZeeshanTamboli can you verify this, this sounds like a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, I've posted other comments that refute the claims about devicePixelRatio. While the previous approach does account for zoom, it fails to do so correctly at the zoom levels that are tricky floats that causes problems with rounding.
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code appends a div to document.body without checking if body exists. In edge cases where this function is called during early page initialization or in unusual document contexts, this could throw an error. Consider adding a check to ensure win.document.body is available before attempting to append the element.
| win.document.body.append(scrollDiv); | |
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | |
| win.document.body.removeChild(scrollDiv); | |
| const body = win.document.body; | |
| if (!body) { | |
| return 0; | |
| } | |
| body.append(scrollDiv); | |
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | |
| body.removeChild(scrollDiv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function getScrollbarSize is called when we need to apply a scroll lock. That happens on user interaction.
| const scrollDiv = win.document.createElement('div'); | ||
|
|
||
| scrollDiv.style.setProperty('position', 'absolute'); | ||
| scrollDiv.style.setProperty('top', '-9999px'); | ||
| scrollDiv.style.setProperty('width', '50px'); | ||
| scrollDiv.style.setProperty('height', '50px'); | ||
| scrollDiv.style.setProperty('overflow', 'scroll'); | ||
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | ||
|
|
||
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); | ||
|
|
||
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; | ||
|
|
||
| return zoomedScrollbarWidth; |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new implementation creates a DOM element, appends it to the document body, measures it, and then removes it on every call. This is significantly more expensive than the previous approach which only queried existing DOM properties (window.innerWidth and documentElement.clientWidth). If this function is called frequently (for example, during window resize events or when multiple modals open), this could create performance issues. Consider implementing a caching mechanism or using the previous simpler approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance concern here. It runs everytime getScrollbarSize is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not something that will happen frequently, it should happen once every time there is the need to apply a scroll lock. Because users are free to change zoom or drag a window to another display whenever they like we cannot simply calculate the value once and use it every time.
Consider what we use the value for, we use it to set padding-right on one or more elements, forcing the browser to reflow the layout. I would be very surprised if that didn't have a higher performance cost.
But there is the option to use the same trick that is used in the devicePixelRatio example I have linked previously and monitor for changes to devicePixelRatio using window.matchMedia() and use it to signal if we need to remeasure the scrollbar width. But this would add more complexity for very little gain.
| scrollDiv.style.setProperty('height', '50px'); | ||
| scrollDiv.style.setProperty('overflow', 'scroll'); | ||
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS zoom property is non-standard and not supported in Firefox. According to MDN, zoom is deprecated and considered non-standard. Using it will cause this functionality to not work properly in Firefox browsers. Consider using CSS transforms with scale() instead, or investigate alternative approaches that work across all major browsers.
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | |
| scrollDiv.style.setProperty('transform-origin', '0 0'); | |
| scrollDiv.style.setProperty('transform', `scale(${1 / win.devicePixelRatio})`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zoom is not deprecated, and it is supported in Firefox https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/zoom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is deprecated is the reset value
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | ||
|
|
||
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); | ||
|
|
||
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new zoom-aware scrollbar calculation logic lacks test coverage. The existing test only covers custom scrollbar widths but doesn't test the devicePixelRatio-based zoom calculations. Consider adding tests that verify the behavior at different devicePixelRatio values (e.g., 1, 1.5, 2) to ensure the calculations work as expected.
| // Credit https://github.com/twbs/bootstrap/blob/0907244256d923807c3a4e55f4ea606b9558d0ca/js/modal.js#L214-L221 | ||
| // and https://github.com/twbs/bootstrap/blob/0907244256d923807c3a4e55f4ea606b9558d0ca/less/modals.less#L122-L128 |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The referenced Bootstrap commit (0907244) is very old (from 2013) and predates modern approaches to handling scrollbar measurements. The linked code uses a different technique than what's implemented here. The Bootstrap reference creates a scrollable div to measure scrollbar width but does not use devicePixelRatio or zoom properties. Consider updating the comment to accurately reflect the actual source of this implementation or remove the misleading citation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from 2014, and that is the latest commit to the referenced file. I kept the citation because there was one in the original and I started from it when developing my solution. The devicePixelRatio and zoom parts are mine and are the things that are required for this to work correctly with browser zoom (and very unorthodox dpi/scaling combinations)
It felt best to keep the citation to bootstrap, but I can absolutely take full credit if that's what you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what is "modern approaches to handling scrollbar measurements" supposed to mean? It's more recent than the commit that was referenced before by mui.
While developing my solution I looked at other libraries, like Bootstrap, Chakra, and FluentUI but they all have the same problem. As far as I can tell my solution to this is unique.
After completing it I did find Vuetify but they take another approach that doesn't hide the scrollbar. Instead they apply position: fixed to the html element and some positioning to keep the scrollbar but prevent scrolling. But implementing this would require much more changes to MUI and I generally prefer smaller changes to large ones.
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | ||
|
|
||
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); | ||
|
|
||
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; | ||
|
|
||
| return zoomedScrollbarWidth; |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using devicePixelRatio to account for browser zoom is incorrect. The devicePixelRatio property represents the ratio between physical pixels and CSS pixels (related to display density/DPI), not browser zoom level. For example, a Retina display has a devicePixelRatio of 2 or 3 regardless of browser zoom. When a user zooms to 90%, devicePixelRatio remains unchanged on the same display. This approach conflates two different concepts and will produce incorrect results on high-DPI displays or when browser zoom doesn't match the device pixel ratio.
| // Invert the zoom level to get 100% sized scrollbars for the element | |
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); | |
| win.document.body.append(scrollDiv); | |
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | |
| win.document.body.removeChild(scrollDiv); | |
| const zoomedScrollbarWidth = unZoomedScrollbarWidth / win.devicePixelRatio; | |
| return zoomedScrollbarWidth; | |
| win.document.body.append(scrollDiv); | |
| const scrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | |
| win.document.body.removeChild(scrollDiv); | |
| return scrollbarWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, you can try it out yourself in the demo that I linked earlier: https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio#examples devicePixelRatio changes with browser zoom.
The table I made earlier is from data gathered using the two different displays I have access to:
- Desktop monitor 27" 2560x1440
- Laptop screen 16" 1920x1200 (which I'm not sure if it qualifies as "High-DPI" but as you can see in my table it has a
devicePixelRatioof 1.25 at 100% zoom)
|
Other than the comment stating the obvious that I did not add test cases for different values of I have now spent about 1 hour of my time refuting the slop generated by Copilot. It has been 100% negative productivity |
|
@Zache In the screen-recording below at https://mui.com/material-ui/react-modal/, I am at 1200px wide and at 90% browser zoom level. I don't see any noticeable shift in icons: Screen-Recording.2.mp4 |
|
@ZeeshanTamboli I'm not sure that I understand you. When you write: "I don't see any noticeable shift" does that mean that you don't notice the horizontal shift of:
Because I very much notice it and so do colleagues which I showed your screen recording. Perhaps when you write "don't see any noticeable" you mean don't think the shift is significant enough to warrant fixing? If that is the case, then I must say that I disagree. I think that the horizontal shift that can happen when zoomed in is both noticeable and significant, even if it is very small. If such a shift would be expected then I don't understand why MUI tries to counteract it when applying the scroll lock. The application where we use MUI is very rich and contains a lot of content. For accessibility reasons we have to keep that content at a certain size but many of our users habitually work zoomed out (especially those who work from laptops), and we would like them to have the same crisp and professional experience as every other user. |
I don't notice shift in this.
Yes, I notice a slight shift there if I look carefully.
Not here.
Yes, I thought is it that significant and worth it?
I agree with you. |
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this in https://deploy-preview-47408--material-ui.netlify.app/material-ui/react-modal/ and it fixes it. There is no horizontal shift.
| const scrollDiv = win.document.createElement('div'); | ||
|
|
||
| scrollDiv.style.setProperty('position', 'absolute'); | ||
| scrollDiv.style.setProperty('top', '-9999px'); | ||
| scrollDiv.style.setProperty('width', '50px'); | ||
| scrollDiv.style.setProperty('height', '50px'); | ||
| scrollDiv.style.setProperty('overflow', 'scroll'); | ||
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to understand the code here, why is there a need to create this dummy scroll div? I see it's added in Bootstrap, but can't understand why.
| const scrollDiv = win.document.createElement('div'); | ||
|
|
||
| scrollDiv.style.setProperty('position', 'absolute'); | ||
| scrollDiv.style.setProperty('top', '-9999px'); | ||
| scrollDiv.style.setProperty('width', '50px'); | ||
| scrollDiv.style.setProperty('height', '50px'); | ||
| scrollDiv.style.setProperty('overflow', 'scroll'); | ||
| // Invert the zoom level to get 100% sized scrollbars for the element | ||
| scrollDiv.style.setProperty('zoom', `${1 / win.devicePixelRatio}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why to add this zoom on the dummy div?
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; | ||
|
|
||
| win.document.body.removeChild(scrollDiv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of adding and removing the div immediately?
|
|
||
| win.document.body.append(scrollDiv); | ||
|
|
||
| const unZoomedScrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this unZoomedScrollbarWidth has value in float?
This PR resolves an issue with to how the scrollbar is measured to prevent a horizontal shift that can happen to elements when applying the scroll lock for certain window sizes and zoom levels.
How to reproduce the scrollbar zoom issue?
(I'm using Windows 11 and Edge with 100% UI scaling in Windows.)
We can use the mui docs (for example Modal)
I have not been able to reproduce the issue for all zoom levels, but for various widths the problem occurs for the following zoom levels: 67%, 80%, 90%, 110%, 175%