Skip to content

Commit

Permalink
fix(overlay): CloseScrollStrategy not triggering change detection on …
Browse files Browse the repository at this point in the history
…close (#7929)

After some refactoring, the `ScrollDispatcher` no longer dispatches inside the `NgZone`, which caused the `CloseScrollStrategy` to break, because it detaches without triggering change detection. These changes bring the detachment back into the `NgZone`.

Fixes #7922.
  • Loading branch information
crisbeto authored and andrewseguin committed Nov 2, 2017
1 parent 6fdc237 commit c0ba25a
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 9 deletions.
13 changes: 12 additions & 1 deletion src/cdk/overlay/scroll/close-scroll-strategy.spec.ts
@@ -1,5 +1,5 @@
import {inject, TestBed, async} from '@angular/core/testing';
import {NgModule, Component} from '@angular/core';
import {NgModule, Component, NgZone} from '@angular/core';
import {Subject} from 'rxjs/Subject';
import {ComponentPortal, PortalModule} from '@angular/cdk/portal';
import {ScrollDispatcher} from '@angular/cdk/scrolling';
Expand Down Expand Up @@ -59,6 +59,17 @@ describe('CloseScrollStrategy', () => {
expect(overlayRef.detach).not.toHaveBeenCalled();
});

it('should detach inside the NgZone', () => {
const spy = jasmine.createSpy('detachment spy');
const subscription = overlayRef.detachments().subscribe(() => spy(NgZone.isInAngularZone()));

overlayRef.attach(componentPortal);
scrolledSubject.next();

expect(spy).toHaveBeenCalledWith(true);
subscription.unsubscribe();
});

});


Expand Down
13 changes: 8 additions & 5 deletions src/cdk/overlay/scroll/close-scroll-strategy.ts
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {NgZone} from '@angular/core';
import {ScrollStrategy, getMatScrollStrategyAlreadyAttachedError} from './scroll-strategy';
import {OverlayRef} from '../overlay-ref';
import {Subscription} from 'rxjs/Subscription';
Expand All @@ -19,7 +20,7 @@ export class CloseScrollStrategy implements ScrollStrategy {
private _scrollSubscription: Subscription|null = null;
private _overlayRef: OverlayRef;

constructor(private _scrollDispatcher: ScrollDispatcher) { }
constructor(private _scrollDispatcher: ScrollDispatcher, private _ngZone: NgZone) { }

/** Attaches this scroll strategy to an overlay. */
attach(overlayRef: OverlayRef) {
Expand All @@ -34,11 +35,13 @@ export class CloseScrollStrategy implements ScrollStrategy {
enable() {
if (!this._scrollSubscription) {
this._scrollSubscription = this._scrollDispatcher.scrolled(0).subscribe(() => {
if (this._overlayRef.hasAttached()) {
this._overlayRef.detach();
}
this._ngZone.run(() => {
this.disable();

this.disable();
if (this._overlayRef.hasAttached()) {
this._overlayRef.detach();
}
});
});
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/cdk/overlay/scroll/scroll-strategy-options.ts
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Injectable} from '@angular/core';
import {Injectable, NgZone} from '@angular/core';
import {CloseScrollStrategy} from './close-scroll-strategy';
import {NoopScrollStrategy} from './noop-scroll-strategy';
import {BlockScrollStrategy} from './block-scroll-strategy';
Expand All @@ -28,13 +28,14 @@ import {
export class ScrollStrategyOptions {
constructor(
private _scrollDispatcher: ScrollDispatcher,
private _viewportRuler: ViewportRuler) { }
private _viewportRuler: ViewportRuler,
private _ngZone: NgZone) { }

/** Do nothing on scroll. */
noop = () => new NoopScrollStrategy();

/** Close the overlay as soon as the user scrolls. */
close = () => new CloseScrollStrategy(this._scrollDispatcher);
close = () => new CloseScrollStrategy(this._scrollDispatcher, this._ngZone);

/** Block scrolling. */
block = () => new BlockScrollStrategy(this._viewportRuler);
Expand Down

0 comments on commit c0ba25a

Please sign in to comment.