Skip to content

Commit

Permalink
fix(scrolling): implement ngOnDestroy in ScrollDispatcher (#9608)
Browse files Browse the repository at this point in the history
Sets up some cleanup logic in the `ScrollDispatcher` in case the module is unloaded.
  • Loading branch information
crisbeto authored and jelbourn committed Jan 26, 2018
1 parent b42fcb9 commit fd17cf2
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 6 deletions.
16 changes: 16 additions & 0 deletions src/cdk/scrolling/scroll-dispatcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,22 @@ describe('Scroll Dispatcher', () => {
.toBe(4, 'Expected scrollable count to stay the same');
});

it('should remove the global subscription on destroy', () => {
expect(scroll._globalSubscription).toBeNull('Expected no global listeners on init.');

const subscription = scroll.scrolled(0).subscribe(() => {});

expect(scroll._globalSubscription).toBeTruthy(
'Expected global listeners after a subscription has been added.');

scroll.ngOnDestroy();

expect(scroll._globalSubscription).toBeNull(
'Expected global listeners to have been removed after the subscription has stopped.');

subscription.unsubscribe();
});

});
});

Expand Down
24 changes: 18 additions & 6 deletions src/cdk/scrolling/scroll-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ElementRef, Injectable, NgZone, Optional, SkipSelf} from '@angular/core';
import {ElementRef, Injectable, NgZone, Optional, SkipSelf, OnDestroy} from '@angular/core';
import {Platform} from '@angular/cdk/platform';
import {Subject} from 'rxjs/Subject';
import {Subscription} from 'rxjs/Subscription';
Expand All @@ -26,7 +26,7 @@ export const DEFAULT_SCROLL_TIME = 20;
* Scrollable references emit a scrolled event.
*/
@Injectable()
export class ScrollDispatcher {
export class ScrollDispatcher implements OnDestroy {
constructor(private _ngZone: NgZone, private _platform: Platform) { }

/** Subject for notifying that a registered scrollable reference element has been scrolled. */
Expand Down Expand Up @@ -97,14 +97,18 @@ export class ScrollDispatcher {
subscription.unsubscribe();
this._scrolledCount--;

if (this._globalSubscription && !this._scrolledCount) {
this._globalSubscription.unsubscribe();
this._globalSubscription = null;
if (!this._scrolledCount) {
this._removeGlobalListener();
}
};
}) : observableOf<void>();
}

ngOnDestroy() {
this._removeGlobalListener();
this.scrollContainers.forEach((_, container) => this.deregister(container));
}

/**
* Returns an observable that emits whenever any of the
* scrollable ancestors of an element are scrolled.
Expand Down Expand Up @@ -146,12 +150,20 @@ export class ScrollDispatcher {
return false;
}

/** Sets up the global scroll and resize listeners. */
/** Sets up the global scroll listeners. */
private _addGlobalListener() {
this._globalSubscription = this._ngZone.runOutsideAngular(() => {
return fromEvent(window.document, 'scroll').subscribe(() => this._scrolled.next());
});
}

/** Cleans up the global scroll listener. */
private _removeGlobalListener() {
if (this._globalSubscription) {
this._globalSubscription.unsubscribe();
this._globalSubscription = null;
}
}
}

/** @docs-private */
Expand Down

0 comments on commit fd17cf2

Please sign in to comment.