fix(viewport-ruler): incorrectly caching viewport size #7951
Conversation
* Currently the `ViewportRuler` caches the `ClientRect` of the `document.documentElement` which is only used for determining the scroll position and is being updated on demand anyway. These changes switch to caching the `innerWidth` and `innerHeight` which was the original intent. * Adds a `getViewportSize` method that returns the cached viewport width and height. The reasoning is that for all of the cases where we were using `getViewportRect`, we were only taking the `width` and `height`. `getViewportSize` helps us avoid the overhead from determining the `top`, `bottom`, `left` and `right` via the scroll position.
googlebot
added
the
cla: yes
crisbeto
added
pr: needs review
and removed
in progress
| /** Cached document client rectangle. */ | ||
| private _documentRect?: ClientRect; | ||
| /** Cached viewport dimensions. */ | ||
| private _viewportSize: {width: number; height: number}; |
There was a problem hiding this comment.
Does reading the window width / height actually cost anything? Do we need to cache it at all?
There was a problem hiding this comment.
| // The top-left-corner of the viewport is determined by the scroll position of the document | ||
| // body, normally just (scrollLeft, scrollTop). However, Chrome and Firefox disagree about | ||
| // whether `document.body` or `document.documentElement` is the scrolled element, so reading | ||
| // `scrollTop` and `scrollLeft` is inconsistent. However, using the bounding rect of | ||
| // `document.documentElement` works consistently, where the `top` and `left` values will | ||
| // equal negative the scroll position. | ||
| const top = -documentRect!.top || document.body.scrollTop || window.scrollY || | ||
| const documentRect = document.documentElement.getBoundingClientRect(); |
There was a problem hiding this comment.
Not for this PR, but thinking about this again, I wonder if there's a better way to get what we're after here than calling getBoundingClientRect here.
There was a problem hiding this comment.
AFAIK the cross-browser way to do it was something like this: window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop || 0.
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
ViewportRulercaches theClientRectof thedocument.documentElementwhich is only used for determining the scroll position and is being updated on demand anyway. These changes switch to caching theinnerWidthandinnerHeightwhich was the original intent.getViewportSizemethod that returns the cached viewport width and height. The reasoning is that for all of the cases where we were usinggetViewportRect, we were only taking thewidthandheight.getViewportSizehelps us avoid the overhead from determining thetop,bottom,leftandrightvia the scroll position.