Fix Chrome window.innerWidth priority for correct canvas resizing#8122
Fix Chrome window.innerWidth priority for correct canvas resizing#8122ayushman1210 wants to merge 9 commits into
Conversation
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
|
@ayushman1210 Thanks! I think |
|
Hi @eyaler , Thanks for the feedback! I’ve updated the code so that window.innerWidth and window.innerHeight are checked first, before document.body.clientWidth and document.documentElement.clientWidth/Height. This ensures the viewport dimensions are captured correctly and also handles scrollbars properly in Chrome. Thanks |
perminder-17
left a comment
There was a problem hiding this comment.
Can you explain what's the reason behind removing all the docs? I think you need to fix the PR with the minimal changes whatever needed.
|
@ayushman1210 i don't get it. this was the state of things before with the issue that innerWidth may be wrong, no? my suggestion was that document.documentElement.clientWidth will be used first (but i think we need more feedback before making this change) |
|
@eyaler
About your suggestion (use [document.documentElement.clientWidth] first )
The tradeoffs:
Because timing of which value updates when can vary across browsers, I left the resize handler deferred to rAF so whichever source we pick will be read after layout has updated. Proposed conservative change (what I can implement now) Use the following order in [getWindowWidth()] / [getWindowHeight()] : Questions / next steps Is that ordering acceptable to you (visualViewport -> documentElement -> innerWidth -> body)? Thanks |
|
thanks @ayushman1210
I coundn't find the added requestAnimationFrame in your PR, and in my own testing requestAnimationFrame did not solve the issue. Can you point me to the commit/line?
In my testing visualViewport does not include scrollbars just as documentElement.clientWidth, and also is wrong when rotating back just as innerWidth. I did find viewport.segments[0].width that seems to both include scrollbars and be correct after orientation change but that is still experimental and has partial support (and i don't have a good enough understanding of it)
On the contrary, I actully think scrollbar should be included if possible |
…able Prefer experimental visualViewport.segments[0] and visualViewport before window.inner* so measurements include scrollbars and handle orientation changes when available. Guard experimental access and fall back to documentElement.client* for compatibility.
|
@eyaler Hi maintainer !! Thanks |
|
@ayushman1210 sorry but i am not a maintainer... btw, i still couldn't find where you used |
|
@eyaler Updated implementation:
The segments API is only used when explicitly available (for foldable devices), then we fall back directly to Regarding
These functions are NOT called in the animation loop, so there's no per-frame performance impact. Thanks |
|
Hi @perminder-17 |
perminder-17
left a comment
There was a problem hiding this comment.
Sorry for the delay in review, I'll be testing the changes once.
perminder-17
left a comment
There was a problem hiding this comment.
Hi @ayushman1210 @eyaler , really sorry for the delay in reviewing this one.
Btw, are you still able to reproduce this issue with the latest chrome. Looks like I am unable to reproduce this issue on my android chrome.
If the issue is still reproduceable, @eyaler gave a very nice finding;
the issue seems to be with chrome window.innerWidth, so maybe in https://github.com/processing/p5.js/blob/main/src/core/environment.js#L787 the order should be changes so that innerWidth gets lower priority?
The main thing the issue needs is just changing the order in which we check the window size, checking document.documentElement.clientWidth first, before window.innerWidth, since innerWidth is the one that gives the wrong value on Chrome Android after rotating.
| // Prefer the visual viewport when available (better for mobile: pinch/keyboard/zoom), | ||
| // then the layout viewport, then the window inner size, then the body content size. | ||
| // Order chosen intentionally: | ||
| // 1) window.visualViewport.* -> visual viewport (mobile-friendly; adapts to on-screen keyboard) | ||
| // 2) document.documentElement.client* -> layout viewport (avoids including scrollbars on some platforms) | ||
| // 3) window.inner* -> includes scrollbars on some platforms (Chrome) | ||
| // 4) document.body.client* -> content-box fallback if the above are unavailable | ||
| // Caveat: different browsers expose subtly different viewport measurements (scrollbars, zoom, | ||
| // and visual viewport). This ordering prefers mobile-friendly measurements while retaining | ||
| // sensible fallbacks for desktop browsers. | ||
| function getWindowWidth() { | ||
| // Prefer including scrollbars when possible per contributor guidance. | ||
| // Order: experimental visualViewport.segments (may include scrollbars/handle rotation), | ||
| // visualViewport.width, window.innerWidth (includes scrollbars), | ||
| // documentElement.clientWidth (excludes scrollbars), document.body.clientWidth. | ||
| try { | ||
| if (window.visualViewport) { | ||
| const segs = window.visualViewport.segments; | ||
| if (Array.isArray(segs) && segs.length > 0) { | ||
| const w = segs[0] && typeof segs[0].width === 'number' ? segs[0].width : null; | ||
| if (w && w > 0) { | ||
| return w; | ||
| } | ||
| } | ||
|
|
||
| // if (typeof window.visualViewport.width === 'number' && window.visualViewport.width > 0) { | ||
| // return window.visualViewport.width; | ||
| // } | ||
| } | ||
| } catch (e) { | ||
| // experimental access may throw; ignore and continue with fallbacks | ||
| } | ||
|
|
||
| return ( | ||
| window.innerWidth || |
There was a problem hiding this comment.
The extra visualViewport.segments part and the try/catch around it aren't really needed for this, which isn't what this bug is about. There are also a few commented-out lines left in getWindowWidth/getWindowHeight that we can remove to keep things tidy.
So it could come down to just this one line:
(document.documentElement && document.documentElement.clientWidth) || window.innerWidth || (document.body && document.body.clientWidth) || 0
perminder-17
left a comment
There was a problem hiding this comment.
For now, I am closing this PR since I am unable to reproduce the issue. Will be reopening if we again face the same issue, but looks like the latest chrome doesn't have that issue.
Resolves #8121
Changes:
getWindowWidth()andgetWindowHeight()functions inenvironment.jsto prioritizedocument.documentElement.clientWidthanddocument.documentElement.clientHeightoverwindow.innerWidthandwindow.innerHeight.windowWidthandwindowHeightvalues do not update correctly when the device is rotated between portrait and landscape orientation.PR Checklist
npm run lintpasses