-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: root rects #1099
fix: root rects #1099
Conversation
// Math.abs(win.innerWidth / visualViewport.scale - visualViewport.width) < | ||
// 0.001 | ||
// Fallback here: "Not Safari" userAgent | ||
if (!/^((?!chrome|android).)*safari/i.test(navigator.userAgent)) { |
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.
If we end up using this check we may want to move it outside the function so that we don't run it more than once per session
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 think it's too impractical to use since it could change in the future, have you had a look at this? Maybe you can find a way to feature detect this that works for all edge cases? There might be something fundamentally wrong with the approach. Not being able to write a test for it doesn't exactly help either.
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.
Puppetter should allow us to enable the mobile emulation mode, if we want to write a test for this we can. I'll try to add one now.
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.
To summarize the issue:
Safari's getBoundingClientRect()
returns "visual viewport" values by default (including pinch-zoom), but Chrome's returns "layout viewport" values (not including pinch-zoom). This means it already uses visual viewport by default, so the offsetLeft/Top
values to determine the offset relative to getBoundingClientRect()
always need to be 0
, but in Chrome, we need to consider them.
So we need to be able to feature detect if getBoundingClientRect()
is returning visual or layout viewport values, and if it's visual, don't add the offsets.
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 just pushed a commit to add the test for this bug, I haven't added the pinch to zoom test yet since I haven't found how to perform the action 😅
Change Log for @popperjs/core (2.4.0 → 2.4.1)Bug Fixes
|
Problems I encountered:
documentRect
width was inaccurate (area outside of<html>
/<body>
bounds if it was horizontally scrollable) and possibly the height. I'm not sure what effects this has exactly since it seemed to work before withrootBoundary: "document"
viewportRect
in mobile emulation mode when pinch zooming broke the horizontal demo in Wrong tooltip positions when X axis has overflow (on mobile) #1097 - it looks like the feature detection doesn't work with when the browser has zoomed the viewport out. I cannot think of a way to fix it besides userAgent detection which can easily break in the future if Safari decides to start using "layout viewport"...