-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A change of the browser zoom change the scrollbar size. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Credit https://github.com/twbs/bootstrap/blob/488fd8afc535ca3a6ad4dc581f5e89217b6a36ac/js/src/util/scrollbar.js#L14-L18 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2
to
+3
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export default function getScrollbarSize(win: Window = window): number { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // https://developer.mozilla.org/en-US/docs/Web/API/Window/innerWidth#usage_notes | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const documentWidth = win.document.documentElement.clientWidth; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return win.innerWidth - documentWidth; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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
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.
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?
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?
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.
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?
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.
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.
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.
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)
Uh oh!
There was an error while loading. Please reload this page.