Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: root rects #1099

Merged
merged 2 commits into from May 26, 2020
Merged

fix: root rects #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75c7831
fix: root rects
atomiks May 1, 2020
90afbba
fix: add more document size checks
atomiks May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 23 additions & 4 deletions src/dom-utils/getDocumentRect.js
View file
Open in desktop
@@ -1,17 +1,36 @@
// @flow
import type { Rect } from '../types';
import getDocumentElement from './getDocumentElement';
import getComputedStyle from './getComputedStyle';
import getWindowScrollBarX from './getWindowScrollBarX';
import getWindowScroll from './getWindowScroll';

// Gets the entire size of the scrollable document area, even extending outside
// of the `<html>` and `<body>` rect bounds if horizontally scrollable
export default function getDocumentRect(element: HTMLElement): Rect {
const winScroll = getWindowScroll(element);
const html = getDocumentElement(element);
const winScroll = getWindowScroll(element);
const body = element.ownerDocument.body;

const width = Math.max(html.clientWidth, body ? body.clientWidth : 0);
const height = Math.max(html.clientHeight, body ? body.clientHeight : 0);
const x = -winScroll.scrollLeft;
const width = Math.max(
html.scrollWidth,
html.clientWidth,
body ? body.scrollWidth : 0,
body ? body.clientWidth : 0
);
const height = Math.max(
html.scrollHeight,
html.clientHeight,
body ? body.scrollHeight : 0,
body ? body.clientHeight : 0
);

let x = -winScroll.scrollLeft + getWindowScrollBarX(element);
const y = -winScroll.scrollTop;

if (getComputedStyle(body || html).direction === 'rtl') {
x += Math.max(html.clientWidth, body ? body.clientWidth : 0) - width;
}

return { width, height, x, y };
}
12 changes: 7 additions & 5 deletions src/dom-utils/getViewportRect.js
View file
Open in desktop
Expand Up @@ -25,11 +25,13 @@ export default function getViewportRect(element: Element) {
// Uses Layout Viewport (like Chrome; Safari does not currently)
// In Chrome, it returns a value very close to 0 (+/-) but contains rounding
// errors due to floating point numbers, so we need to check precision.
// Safari returns a number <= 0, usually <= 1 when pinch-zoomed
if (
Math.abs(win.innerWidth / visualViewport.scale - visualViewport.width) <
0.001
) {
// Safari returns a number <= 0, usually < -1 when pinch-zoomed

// Feature detection fails in mobile emulation mode in Chrome.
FezVrasta marked this conversation as resolved.
Show resolved Hide resolved
// Math.abs(win.innerWidth / visualViewport.scale - visualViewport.width) <
// 0.001
// Fallback here: "Not Safari" userAgent
if (!/^((?!chrome|android).)*safari/i.test(navigator.userAgent)) {
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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 😅

x = visualViewport.offsetLeft;
y = visualViewport.offsetTop;
}
Expand Down