fix(scrolling): viewport ruler events being run inside zone

src/cdk/scrolling/viewport-ruler.spec.ts

@@ -3,17 +3,11 @@ import {ScrollingModule} from './public-api';
 import {ViewportRuler} from './viewport-ruler';
 import {dispatchFakeEvent} from '@angular/cdk/testing/private';
 import {NgZone} from '@angular/core';
-
-// For all tests, we assume the browser window is 1024x786 (outerWidth x outerHeight).
-// The karma config has been set to this for local tests, and it is the default size
-// for tests on CI (both SauceLabs and Browserstack).
-
-// While we know the *outer* window width/height, the innerWidth and innerHeight depend on the
-// the size of the individual browser's chrome, so we have to use window.innerWidth and
-// window.innerHeight in the unit test instead of hard-coded values.
+import {Subscription} from 'rxjs';
 
 describe('ViewportRuler', () => {
-  let ruler: ViewportRuler;
+  let viewportRuler: ViewportRuler;
+  let ngZone: NgZone;
 
   let startingWindowWidth = window.innerWidth;
   let startingWindowHeight = window.innerHeight;
@@ -28,23 +22,24 @@ describe('ViewportRuler', () => {
     providers: [ViewportRuler]
   }));
 
-  beforeEach(inject([ViewportRuler], (viewportRuler: ViewportRuler) => {
-    ruler = viewportRuler;
+  beforeEach(inject([ViewportRuler, NgZone], (v: ViewportRuler, n: NgZone) => {
+    viewportRuler = v;
+    ngZone = n;
     scrollTo(0, 0);
   }));
 
   afterEach(() => {
-    ruler.ngOnDestroy();
+    viewportRuler.ngOnDestroy();
   });
 
   it('should get the viewport size', () => {
-    let size = ruler.getViewportSize();
+    let size = viewportRuler.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();
+    let bounds = viewportRuler.getViewportRect();
     expect(bounds.top).toBe(0);
     expect(bounds.left).toBe(0);
     expect(bounds.bottom).toBe(window.innerHeight);
@@ -56,7 +51,7 @@ describe('ViewportRuler', () => {
 
     scrollTo(1500, 2000);
 
-    let bounds = ruler.getViewportRect();
+    let bounds = viewportRuler.getViewportRect();
 
     // 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
@@ -82,7 +77,7 @@ describe('ViewportRuler', () => {
   });
 
   it('should get the scroll position when the page is not scrolled', () => {
-    let scrollPos = ruler.getViewportScrollPosition();
+    let scrollPos = viewportRuler.getViewportScrollPosition();
     expect(scrollPos.top).toBe(0);
     expect(scrollPos.left).toBe(0);
   });
@@ -102,7 +97,7 @@ describe('ViewportRuler', () => {
       return;
     }
 
-    let scrollPos = ruler.getViewportScrollPosition();
+    let scrollPos = viewportRuler.getViewportScrollPosition();
     expect(scrollPos.top).toBe(2000);
     expect(scrollPos.left).toBe(1500);
 
@@ -112,7 +107,7 @@ describe('ViewportRuler', () => {
   describe('changed event', () => {
     it('should dispatch an event when the window is resized', () => {
       const spy = jasmine.createSpy('viewport changed spy');
-      const subscription = ruler.change(0).subscribe(spy);
+      const subscription = viewportRuler.change(0).subscribe(spy);
 
       dispatchFakeEvent(window, 'resize');
       expect(spy).toHaveBeenCalled();
@@ -121,7 +116,7 @@ describe('ViewportRuler', () => {
 
     it('should dispatch an event when the orientation is changed', () => {
       const spy = jasmine.createSpy('viewport changed spy');
-      const subscription = ruler.change(0).subscribe(spy);
+      const subscription = viewportRuler.change(0).subscribe(spy);
 
       dispatchFakeEvent(window, 'orientationchange');
       expect(spy).toHaveBeenCalled();
@@ -130,7 +125,7 @@ describe('ViewportRuler', () => {
 
     it('should be able to throttle the callback', fakeAsync(() => {
       const spy = jasmine.createSpy('viewport changed spy');
-      const subscription = ruler.change(1337).subscribe(spy);
+      const subscription = viewportRuler.change(1337).subscribe(spy);
 
       dispatchFakeEvent(window, 'resize');
       expect(spy).not.toHaveBeenCalled();
@@ -143,12 +138,25 @@ describe('ViewportRuler', () => {
 
     it('should run the resize event outside the NgZone', () => {
       const spy = jasmine.createSpy('viewport changed spy');
-      const subscription = ruler.change(0).subscribe(() => spy(NgZone.isInAngularZone()));
+      const subscription = viewportRuler.change(0).subscribe(() => spy(NgZone.isInAngularZone()));
 
       dispatchFakeEvent(window, 'resize');
       expect(spy).toHaveBeenCalledWith(false);
       subscription.unsubscribe();
     });
 
+    it('should run events outside of the NgZone, even if the subcription is from inside', () => {
+      const spy = jasmine.createSpy('viewport changed spy');
+      let subscription: Subscription;
+
+      ngZone.run(() => {
+        subscription = viewportRuler.change(0).subscribe(() => spy(NgZone.isInAngularZone()));
+        dispatchFakeEvent(window, 'resize');
+      });
+
+      expect(spy).toHaveBeenCalledWith(false);
+      subscription!.unsubscribe();
+    });
+
   });
 });

src/cdk/scrolling/viewport-ruler.ts

@@ -8,7 +8,7 @@
 
 import {Platform} from '@angular/cdk/platform';
 import {Injectable, NgZone, OnDestroy, Optional, Inject} from '@angular/core';
-import {merge, of as observableOf, fromEvent, Observable, Subscription} from 'rxjs';
+import {Observable, Subject} from 'rxjs';
 import {auditTime} from 'rxjs/operators';
 import {DOCUMENT} from '@angular/common';
 
@@ -31,10 +31,12 @@ export class ViewportRuler implements OnDestroy {
   private _viewportSize: {width: number; height: number};
 
   /** Stream of viewport change events. */
-  private _change: Observable<Event>;
+  private _change = new Subject<Event>();
 
-  /** Subscription to streams that invalidate the cached viewport dimensions. */
-  private _invalidateCache: Subscription;
+  /** Event listener that will be used to handle the viewport change events. */
+  private _changeListener = (event: Event) => {
+    this._change.next(event);
+  }
 
   /** Used to reference correct document/window */
   protected _document?: Document;
@@ -46,20 +48,29 @@ export class ViewportRuler implements OnDestroy {
     this._document = document;
 
     ngZone.runOutsideAngular(() => {
-      const window = this._getWindow();
-
-      this._change = _platform.isBrowser ?
-          merge(fromEvent(window, 'resize'), fromEvent(window, 'orientationchange')) :
-          observableOf();
-
-      // Note that we need to do the subscription inside `runOutsideAngular`
-      // since subscribing is what causes the event listener to be added.
-      this._invalidateCache = this.change().subscribe(() => this._updateViewportSize());
+      if (_platform.isBrowser) {
+        const window = this._getWindow();
+
+        // Note that bind the events ourselves, rather than going through something like RxJS's
+        // `fromEvent` so that we can ensure that they're bound outside of the NgZone.
+        window.addEventListener('resize', this._changeListener);
+        window.addEventListener('orientationchange', this._changeListener);
+      }
+
+      // We don't need to keep track of the subscription,
+      // because we complete the `change` stream on destroy.
+      this.change().subscribe(() => this._updateViewportSize());
     });
   }
 
   ngOnDestroy() {
-    this._invalidateCache.unsubscribe();
+    if (this._platform.isBrowser) {
+      const window = this._getWindow();
+      window.removeEventListener('resize', this._changeListener);
+      window.removeEventListener('orientationchange', this._changeListener);
+    }
+
+    this._change.complete();
   }
 
   /** Returns the viewport's width and height. */