From 0d6d9cceb01f07edb1f4a686ca9d750bb08acb5b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 2 Nov 2017 04:31:53 +0100 Subject: [PATCH] fix(viewport-ruler): incorrectly caching viewport size (#7951) * 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. --- .../position/connected-position-strategy.ts | 16 +++--- .../scroll/block-scroll-strategy.spec.ts | 25 +++------ .../overlay/scroll/block-scroll-strategy.ts | 2 +- src/cdk/scrolling/viewport-ruler.spec.ts | 10 ++-- src/cdk/scrolling/viewport-ruler.ts | 52 ++++++++----------- src/cdk/testing/fake-viewport-ruler.ts | 4 ++ src/lib/select/select.ts | 8 +-- 7 files changed, 53 insertions(+), 64 deletions(-) diff --git a/src/cdk/overlay/position/connected-position-strategy.ts b/src/cdk/overlay/position/connected-position-strategy.ts index eb08712ba79..10dcbdfe432 100644 --- a/src/cdk/overlay/position/connected-position-strategy.ts +++ b/src/cdk/overlay/position/connected-position-strategy.ts @@ -122,8 +122,8 @@ export class ConnectedPositionStrategy implements PositionStrategy { const originRect = this._origin.getBoundingClientRect(); const overlayRect = element.getBoundingClientRect(); - // We use the viewport rect to determine whether a position would go off-screen. - const viewportRect = this._viewportRuler.getViewportRect(); + // We use the viewport size to determine whether a position would go off-screen. + const viewportSize = this._viewportRuler.getViewportSize(); // Fallback point if none of the fallbacks fit into the viewport. let fallbackPoint: OverlayPoint | undefined; @@ -135,7 +135,7 @@ export class ConnectedPositionStrategy implements PositionStrategy { // Get the (x, y) point of connection on the origin, and then use that to get the // (top, left) coordinate for the overlay at `pos`. let originPoint = this._getOriginConnectionPoint(originRect, pos); - let overlayPoint = this._getOverlayPoint(originPoint, overlayRect, viewportRect, pos); + let overlayPoint = this._getOverlayPoint(originPoint, overlayRect, viewportSize, pos); // If the overlay in the calculated position fits on-screen, put it there and we're done. if (overlayPoint.fitsInViewport) { @@ -169,11 +169,11 @@ export class ConnectedPositionStrategy implements PositionStrategy { const originRect = this._origin.getBoundingClientRect(); const overlayRect = this._pane.getBoundingClientRect(); - const viewportRect = this._viewportRuler.getViewportRect(); + const viewportSize = this._viewportRuler.getViewportSize(); const lastPosition = this._lastConnectedPosition || this._preferredPositions[0]; let originPoint = this._getOriginConnectionPoint(originRect, lastPosition); - let overlayPoint = this._getOverlayPoint(originPoint, overlayRect, viewportRect, lastPosition); + let overlayPoint = this._getOverlayPoint(originPoint, overlayRect, viewportSize, lastPosition); this._setElementPosition(this._pane, overlayRect, overlayPoint, lastPosition); } @@ -281,7 +281,7 @@ export class ConnectedPositionStrategy implements PositionStrategy { private _getOverlayPoint( originPoint: Point, overlayRect: ClientRect, - viewportRect: ClientRect, + viewportSize: {width: number; height: number}, pos: ConnectionPositionPair): OverlayPoint { // Calculate the (overlayStartX, overlayStartY), the start of the potential overlay position // relative to the origin point. @@ -311,9 +311,9 @@ export class ConnectedPositionStrategy implements PositionStrategy { // How much the overlay would overflow at this position, on each side. let leftOverflow = 0 - x; - let rightOverflow = (x + overlayRect.width) - viewportRect.width; + let rightOverflow = (x + overlayRect.width) - viewportSize.width; let topOverflow = 0 - y; - let bottomOverflow = (y + overlayRect.height) - viewportRect.height; + let bottomOverflow = (y + overlayRect.height) - viewportSize.height; // Visible parts of the element on each axis. let visibleWidth = this._subtractOverflows(overlayRect.width, leftOverflow, rightOverflow); diff --git a/src/cdk/overlay/scroll/block-scroll-strategy.spec.ts b/src/cdk/overlay/scroll/block-scroll-strategy.spec.ts index 6126e3a3b8f..60a45960985 100644 --- a/src/cdk/overlay/scroll/block-scroll-strategy.spec.ts +++ b/src/cdk/overlay/scroll/block-scroll-strategy.spec.ts @@ -39,12 +39,12 @@ describe('BlockScrollStrategy', () => { afterEach(inject([OverlayContainer], (container: OverlayContainer) => { overlayRef.dispose(); document.body.removeChild(forceScrollElement); - setScrollPosition(0, 0); + window.scroll(0, 0); container.getContainerElement().parentNode!.removeChild(container.getContainerElement()); })); it('should toggle scroll blocking along the y axis', skipIOS(() => { - setScrollPosition(0, 100); + window.scroll(0, 100); expect(viewport.getViewportScrollPosition().top) .toBe(100, 'Expected viewport to be scrollable initially.'); @@ -52,7 +52,7 @@ describe('BlockScrollStrategy', () => { expect(document.documentElement.style.top) .toBe('-100px', 'Expected element to be offset by the previous scroll amount.'); - setScrollPosition(0, 300); + window.scroll(0, 300); expect(viewport.getViewportScrollPosition().top) .toBe(100, 'Expected the viewport not to scroll.'); @@ -60,7 +60,7 @@ describe('BlockScrollStrategy', () => { expect(viewport.getViewportScrollPosition().top) .toBe(100, 'Expected old scroll position to have bee restored after disabling.'); - setScrollPosition(0, 300); + window.scroll(0, 300); expect(viewport.getViewportScrollPosition().top) .toBe(300, 'Expected user to be able to scroll after disabling.'); })); @@ -70,7 +70,7 @@ describe('BlockScrollStrategy', () => { forceScrollElement.style.height = '100px'; forceScrollElement.style.width = '3000px'; - setScrollPosition(100, 0); + window.scroll(100, 0); expect(viewport.getViewportScrollPosition().left) .toBe(100, 'Expected viewport to be scrollable initially.'); @@ -78,7 +78,7 @@ describe('BlockScrollStrategy', () => { expect(document.documentElement.style.left) .toBe('-100px', 'Expected element to be offset by the previous scroll amount.'); - setScrollPosition(300, 0); + window.scroll(300, 0); expect(viewport.getViewportScrollPosition().left) .toBe(100, 'Expected the viewport not to scroll.'); @@ -86,7 +86,7 @@ describe('BlockScrollStrategy', () => { expect(viewport.getViewportScrollPosition().left) .toBe(100, 'Expected old scroll position to have bee restored after disabling.'); - setScrollPosition(300, 0); + window.scroll(300, 0); expect(viewport.getViewportScrollPosition().left) .toBe(300, 'Expected user to be able to scroll after disabling.'); })); @@ -163,7 +163,6 @@ describe('BlockScrollStrategy', () => { * tests unusable. For example, something as basic as the following won't work: * ``` * window.scroll(0, 100); - * viewport._cacheViewportGeometry(); * expect(viewport.getViewportScrollPosition().top).toBe(100); * ``` * @param spec Test to be executed or skipped. @@ -176,16 +175,6 @@ describe('BlockScrollStrategy', () => { }; } - /** - * Scrolls the viewport and clears the cache. - * @param x Amount to scroll along the x axis. - * @param y Amount to scroll along the y axis. - */ - function setScrollPosition(x: number, y: number) { - window.scroll(x, y); - viewport._cacheViewportGeometry(); - } - }); diff --git a/src/cdk/overlay/scroll/block-scroll-strategy.ts b/src/cdk/overlay/scroll/block-scroll-strategy.ts index b828a96b97b..68e74f04762 100644 --- a/src/cdk/overlay/scroll/block-scroll-strategy.ts +++ b/src/cdk/overlay/scroll/block-scroll-strategy.ts @@ -76,7 +76,7 @@ export class BlockScrollStrategy implements ScrollStrategy { } const body = document.body; - const viewport = this._viewportRuler.getViewportRect(); + const viewport = this._viewportRuler.getViewportSize(); return body.scrollHeight > viewport.height || body.scrollWidth > viewport.width; } } diff --git a/src/cdk/scrolling/viewport-ruler.spec.ts b/src/cdk/scrolling/viewport-ruler.spec.ts index 678e9fb116b..78546a5489d 100644 --- a/src/cdk/scrolling/viewport-ruler.spec.ts +++ b/src/cdk/scrolling/viewport-ruler.spec.ts @@ -37,6 +37,12 @@ describe('ViewportRuler', () => { ruler.ngOnDestroy(); }); + it('should get the viewport size', () => { + let size = ruler.getViewportSize(); + expect(size.width).toBe(window.innerWidth); + expect(size.height).toBe(window.innerHeight); + }); + it('should get the viewport bounds when the page is not scrolled', () => { let bounds = ruler.getViewportRect(); expect(bounds.top).toBe(0); @@ -49,8 +55,6 @@ describe('ViewportRuler', () => { document.body.appendChild(veryLargeElement); scrollTo(1500, 2000); - // Force an update of the cached viewport geometries because IE11 emits the scroll event later. - ruler._cacheViewportGeometry(); let bounds = ruler.getViewportRect(); @@ -87,8 +91,6 @@ describe('ViewportRuler', () => { document.body.appendChild(veryLargeElement); scrollTo(1500, 2000); - // Force an update of the cached viewport geometries because IE11 emits the scroll event later. - ruler._cacheViewportGeometry(); // In the iOS simulator (BrowserStack & SauceLabs), adding the content to the // body causes karma's iframe for the test to stretch to fit that content once we attempt to diff --git a/src/cdk/scrolling/viewport-ruler.ts b/src/cdk/scrolling/viewport-ruler.ts index a2e90e75ff2..a1c8435505c 100644 --- a/src/cdk/scrolling/viewport-ruler.ts +++ b/src/cdk/scrolling/viewport-ruler.ts @@ -24,9 +24,8 @@ export const DEFAULT_RESIZE_TIME = 20; */ @Injectable() export class ViewportRuler implements OnDestroy { - - /** Cached document client rectangle. */ - private _documentRect?: ClientRect; + /** Cached viewport dimensions. */ + private _viewportSize: {width: number; height: number}; /** Stream of viewport change events. */ private _change: Observable; @@ -39,21 +38,24 @@ export class ViewportRuler implements OnDestroy { return merge(fromEvent(window, 'resize'), fromEvent(window, 'orientationchange')); }) : observableOf(); - this._invalidateCache = this.change().subscribe(() => this._cacheViewportGeometry()); + this._invalidateCache = this.change().subscribe(() => this._updateViewportSize()); } ngOnDestroy() { this._invalidateCache.unsubscribe(); } - /** Gets a ClientRect for the viewport's bounds. */ - getViewportRect(documentRect: ClientRect | undefined = this._documentRect): ClientRect { - // Cache the document bounding rect so that we don't recompute it for multiple calls. - if (!documentRect) { - this._cacheViewportGeometry(); - documentRect = this._documentRect; + /** Returns the viewport's width and height. */ + getViewportSize(): Readonly<{width: number, height: number}> { + if (!this._viewportSize) { + this._updateViewportSize(); } + return {width: this._viewportSize.width, height: this._viewportSize.height}; + } + + /** Gets a ClientRect for the viewport's bounds. */ + getViewportRect(): ClientRect { // Use the document element's bounding rect rather than the window scroll properties // (e.g. pageYOffset, scrollY) due to in issue in Chrome and IE where window scroll // properties and client coordinates (boundingClientRect, clientX/Y, etc.) are in different @@ -63,9 +65,8 @@ export class ViewportRuler implements OnDestroy { // We use the documentElement instead of the body because, by default (without a css reset) // browsers typically give the document body an 8px margin, which is not included in // getBoundingClientRect(). - const scrollPosition = this.getViewportScrollPosition(documentRect); - const height = window.innerHeight; - const width = window.innerWidth; + const scrollPosition = this.getViewportScrollPosition(); + const {width, height} = this.getViewportSize(); return { top: scrollPosition.top, @@ -77,27 +78,20 @@ export class ViewportRuler implements OnDestroy { }; } - /** - * Gets the (top, left) scroll position of the viewport. - * @param documentRect - */ - getViewportScrollPosition(documentRect: ClientRect | undefined = this._documentRect) { - // Cache the document bounding rect so that we don't recompute it for multiple calls. - if (!documentRect) { - this._cacheViewportGeometry(); - documentRect = this._documentRect; - } - + /** Gets the (top, left) scroll position of the viewport. */ + getViewportScrollPosition() { // 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(); + + const top = -documentRect.top || document.body.scrollTop || window.scrollY || document.documentElement.scrollTop || 0; - const left = -documentRect!.left || document.body.scrollLeft || window.scrollX || + const left = -documentRect.left || document.body.scrollLeft || window.scrollX || document.documentElement.scrollLeft || 0; return {top, left}; @@ -111,9 +105,9 @@ export class ViewportRuler implements OnDestroy { return throttleTime > 0 ? this._change.pipe(auditTime(throttleTime)) : this._change; } - /** Caches the latest client rectangle of the document element. */ - _cacheViewportGeometry() { - this._documentRect = document.documentElement.getBoundingClientRect(); + /** Updates the cached viewport size. */ + private _updateViewportSize() { + this._viewportSize = {width: window.innerWidth, height: window.innerHeight}; } } diff --git a/src/cdk/testing/fake-viewport-ruler.ts b/src/cdk/testing/fake-viewport-ruler.ts index 644b8ffa310..267c45fa062 100644 --- a/src/cdk/testing/fake-viewport-ruler.ts +++ b/src/cdk/testing/fake-viewport-ruler.ts @@ -14,6 +14,10 @@ export class FakeViewportRuler { }; } + getViewportSize() { + return {width: 1014, height: 686}; + } + getViewportScrollPosition() { return {top: 0, left: 0}; } diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index e42a660550d..e2e608b509e 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -1013,7 +1013,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, */ private _calculateOverlayOffsetX(): void { const overlayRect = this.overlayDir.overlayRef.overlayElement.getBoundingClientRect(); - const viewportRect = this._viewportRuler.getViewportRect(); + const viewportSize = this._viewportRuler.getViewportSize(); const isRtl = this._isRtl(); const paddingWidth = this.multiple ? SELECT_MULTIPLE_PANEL_PADDING_X + SELECT_PANEL_PADDING_X : SELECT_PANEL_PADDING_X * 2; @@ -1034,7 +1034,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, // Determine how much the select overflows on each side. const leftOverflow = 0 - (overlayRect.left + offsetX - (isRtl ? paddingWidth : 0)); - const rightOverflow = overlayRect.right + offsetX - viewportRect.width + const rightOverflow = overlayRect.right + offsetX - viewportSize.width + (isRtl ? 0 : paddingWidth); // If the element overflows on either side, reduce the offset to allow it to fit. @@ -1099,11 +1099,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, */ private _checkOverlayWithinViewport(maxScroll: number): void { const itemHeight = this._getItemHeight(); - const viewportRect = this._viewportRuler.getViewportRect(); + const viewportSize = this._viewportRuler.getViewportSize(); const topSpaceAvailable = this._triggerRect.top - SELECT_PANEL_VIEWPORT_PADDING; const bottomSpaceAvailable = - viewportRect.height - this._triggerRect.bottom - SELECT_PANEL_VIEWPORT_PADDING; + viewportSize.height - this._triggerRect.bottom - SELECT_PANEL_VIEWPORT_PADDING; const panelHeightTop = Math.abs(this._offsetY); const totalPanelHeight =