Skip to content

fix(material/select): run viewportRuler change event outside the zone #18548

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions src/cdk/scrolling/viewport-ruler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {ScrollingModule} from './public-api';
import {ViewportRuler} from './viewport-ruler';
import {dispatchFakeEvent} from '@angular/cdk/testing/private';
import {NgZone} from '@angular/core';
import {Subscription} from 'rxjs';

// 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
Expand All @@ -13,7 +14,8 @@ import {NgZone} from '@angular/core';
// window.innerHeight in the unit test instead of hard-coded values.

describe('ViewportRuler', () => {
let ruler: ViewportRuler;
let viewportRuler: ViewportRuler;
let ngZone: NgZone;

let startingWindowWidth = window.innerWidth;
let startingWindowHeight = window.innerHeight;
Expand All @@ -28,23 +30,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);
Expand All @@ -56,7 +59,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
Expand All @@ -82,7 +85,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);
});
Expand All @@ -102,7 +105,7 @@ describe('ViewportRuler', () => {
return;
}

let scrollPos = ruler.getViewportScrollPosition();
let scrollPos = viewportRuler.getViewportScrollPosition();
expect(scrollPos.top).toBe(2000);
expect(scrollPos.left).toBe(1500);

Expand All @@ -112,7 +115,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();
Expand All @@ -121,7 +124,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();
Expand All @@ -130,7 +133,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();
Expand All @@ -143,12 +146,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();
});

});
});
20 changes: 13 additions & 7 deletions src/cdk/scrolling/viewport-ruler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ export interface ViewportScrollPosition {
left: number;
}

/** Runs this observable outside the zone */
function runOutsideZone<T>(zone: NgZone): any {
return (source: Observable<T>): Observable<T> => {
return new Observable(observer => {
return zone.runOutsideAngular<Subscription>(() => source.subscribe(observer));
});
};
}

/**
* Simple utility for getting the bounds of the browser viewport.
* @docs-private
Expand All @@ -36,15 +45,12 @@ export class ViewportRuler implements OnDestroy {
private _invalidateCache: Subscription;

constructor(private _platform: Platform, ngZone: NgZone) {
ngZone.runOutsideAngular(() => {
this._change = _platform.isBrowser ?
merge(fromEvent(window, 'resize'), fromEvent(window, 'orientationchange')) :
observableOf();
this._change = _platform.isBrowser ?
merge(fromEvent(window, 'resize'), fromEvent(window, 'orientationchange'))
.pipe(runOutsideZone(ngZone)) :
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());
});
}

ngOnDestroy() {
Expand Down
7 changes: 5 additions & 2 deletions src/material/select/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -572,8 +572,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
.pipe(takeUntil(this._destroy))
.subscribe(() => {
if (this._panelOpen) {
this._triggerRect = this.trigger.nativeElement.getBoundingClientRect();
this._changeDetectorRef.markForCheck();
// Run inside the zone because viewport ruler change() event is outside it.
this._ngZone.run(() => {
this._triggerRect = this.trigger.nativeElement.getBoundingClientRect();
this._changeDetectorRef.markForCheck();
});
}
});
}
Expand Down
6 changes: 4 additions & 2 deletions src/material/tabs/paginated-tab-header.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,10 @@ export abstract class MatPaginatedTabHeader implements AfterContentChecked, Afte
// On dir change or window resize, realign the ink bar and update the orientation of
// the key manager if the direction has changed.
merge(dirChange, resize, this._items.changes).pipe(takeUntil(this._destroyed)).subscribe(() => {
realign();
this._keyManager.withHorizontalOrientation(this._getLayoutDirection());
this._ngZone.run(() => {
realign();
this._keyManager.withHorizontalOrientation(this._getLayoutDirection());
});
});

// If there is a change in the focus key manager we need to emit the `indexFocused`
Expand Down