Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
75c7831
90afbba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 theoffsetLeft/Top
values to determine the offset relative togetBoundingClientRect()
always need to be0
, 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 😅