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.
angular · Nov 2, 2017 · 0d6d9cc · 0d6d9cc
1 parent ff7a13b
commit 0d6d9cc
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 64 deletions.
diff --git 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
@@ -39,28 +39,28 @@ 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.');
 
     overlayRef.attach(componentPortal);
     expect(document.documentElement.style.top)
         .toBe('-100px', 'Expected <html> 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.');
 
     overlayRef.detach();
     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,23 +70,23 @@ 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.');
 
     overlayRef.attach(componentPortal);
     expect(document.documentElement.style.left)
         .toBe('-100px', 'Expected <html> 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.');
 
     overlayRef.detach();
     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
@@ -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
@@ -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
@@ -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<Event>;
@@ -39,21 +38,24 @@ export class ViewportRuler implements OnDestroy {
       return merge<Event>(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
@@ -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
@@ -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 =