fix(overlay): overlay directives not emitting when detached externally (#7950)

Currently the `ConnectedOverlayDirective` only emits the `detach` event when it _thinks_ that the overlay is detached (escape press, backdrop click etc.), but this won't necessarily be correct (e.g. when it was closed by a scroll strategy). These changes refactor the outputs to always be one-to-one with the `OverlayRef` detachments.

Author: crisbeto

src/cdk/overlay/overlay-directives.spec.ts

@@ -8,13 +8,21 @@ import {
   dispatchEvent,
 } from '@angular/cdk/testing/private';
 import {ESCAPE, A} from '@angular/cdk/keycodes';
-import {Overlay, CdkConnectedOverlay, OverlayModule, CdkOverlayOrigin} from './index';
+import {
+  Overlay,
+  CdkConnectedOverlay,
+  OverlayModule,
+  CdkOverlayOrigin,
+  ScrollDispatcher,
+  ScrollStrategy,
+} from './index';
 import {OverlayContainer} from './overlay-container';
 import {
   ConnectedOverlayPositionChange,
   ConnectionPositionPair,
 } from './position/connected-position';
 import {FlexibleConnectedPositionStrategy} from './position/flexible-connected-position-strategy';
+import {Subject} from 'rxjs';
 
 
 describe('Overlay directives', () => {
@@ -23,12 +31,17 @@ describe('Overlay directives', () => {
   let overlayContainerElement: HTMLElement;
   let fixture: ComponentFixture<ConnectedOverlayDirectiveTest>;
   let dir: {value: string};
+  let scrolledSubject = new Subject();
 
   beforeEach(() => {
     TestBed.configureTestingModule({
       imports: [OverlayModule],
       declarations: [ConnectedOverlayDirectiveTest, ConnectedOverlayPropertyInitOrder],
-      providers: [{provide: Directionality, useFactory: () => dir = {value: 'ltr'}}],
+      providers: [{provide: Directionality, useFactory: () => dir = {value: 'ltr'}},
+        {provide: ScrollDispatcher, useFactory: () => ({
+          scrolled: () => scrolledSubject.asObservable()
+        })}
+      ],
     });
   });
 
@@ -529,7 +542,7 @@ describe('Overlay directives', () => {
   });
 
   describe('outputs', () => {
-    it('should emit backdropClick appropriately', () => {
+    it('should emit when the backdrop was clicked', () => {
       fixture.componentInstance.hasBackdrop = true;
       fixture.componentInstance.isOpen = true;
       fixture.detectChanges();
@@ -543,7 +556,7 @@ describe('Overlay directives', () => {
           .toHaveBeenCalledWith(jasmine.any(MouseEvent));
     });
 
-    it('should emit positionChange appropriately', () => {
+    it('should emit when the position has changed', () => {
       expect(fixture.componentInstance.positionChangeHandler).not.toHaveBeenCalled();
       fixture.componentInstance.isOpen = true;
       fixture.detectChanges();
@@ -556,15 +569,24 @@ describe('Overlay directives', () => {
           .toBe(true, `Expected directive to emit an instance of ConnectedOverlayPositionChange.`);
     });
 
-    it('should emit attach and detach appropriately', () => {
+    it('should emit when attached', () => {
       expect(fixture.componentInstance.attachHandler).not.toHaveBeenCalled();
-      expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled();
       fixture.componentInstance.isOpen = true;
       fixture.detectChanges();
 
       expect(fixture.componentInstance.attachHandler).toHaveBeenCalled();
       expect(fixture.componentInstance.attachResult instanceof HTMLElement)
           .toBe(true, `Expected pane to be populated with HTML elements when attach was called.`);
+
+      fixture.componentInstance.isOpen = false;
+      fixture.detectChanges();
+    });
+
+    it('should emit when detached', () => {
+      expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled();
+      fixture.componentInstance.isOpen = true;
+      fixture.detectChanges();
+
       expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled();
 
       fixture.componentInstance.isOpen = false;
@@ -584,11 +606,40 @@ describe('Overlay directives', () => {
       expect(fixture.componentInstance.keydownHandler).toHaveBeenCalledWith(event);
     });
 
+    it('should emit when detached externally', () => {
+      expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled();
+      fixture.componentInstance.scrollStrategy = overlay.scrollStrategies.close();
+      fixture.componentInstance.isOpen = true;
+      fixture.detectChanges();
+
+      expect(fixture.componentInstance.detachHandler).not.toHaveBeenCalled();
+
+      scrolledSubject.next();
+      fixture.detectChanges();
+
+      expect(fixture.componentInstance.detachHandler).toHaveBeenCalled();
+    });
+
+    // This is intended as a simplified example of a more complicated bug in g3. Technically
+    // these events shouldn't invoke their listeners after destruction anyway, but in some
+    // tests it can happen. For more context: https://github.com/crisbeto/material2/pull/10
+    it('should not emit after the directive is destroyed', () => {
+      const spy = jasmine.createSpy('detach spy');
+      const subscription =
+          fixture.componentInstance.connectedOverlayDirective.detach.subscribe(spy);
+
+      fixture.componentInstance.isOpen = true;
+      fixture.detectChanges();
+      fixture.destroy();
+
+      expect(spy).not.toHaveBeenCalled();
+      subscription.unsubscribe();
+    });
+
   });
 
 });
 
-
 @Component({
   template: `
   <button cdk-overlay-origin id="trigger" #trigger="cdkOverlayOrigin">Toggle menu</button>
@@ -605,6 +656,7 @@ describe('Overlay directives', () => {
             [cdkConnectedOverlayFlexibleDimensions]="flexibleDimensions"
             [cdkConnectedOverlayGrowAfterOpen]="growAfterOpen"
             [cdkConnectedOverlayPush]="push"
+            [cdkConnectedOverlayScrollStrategy]="scrollStrategy"
             cdkConnectedOverlayBackdropClass="mat-test-class"
             cdkConnectedOverlayPanelClass="cdk-test-panel-class"
             (backdropClick)="backdropClickHandler($event)"
@@ -640,13 +692,14 @@ class ConnectedOverlayDirectiveTest {
   flexibleDimensions: boolean;
   growAfterOpen: boolean;
   push: boolean;
+  scrollStrategy: ScrollStrategy;
   backdropClickHandler = jasmine.createSpy('backdropClick handler');
   positionChangeHandler = jasmine.createSpy('positionChange handler');
   keydownHandler = jasmine.createSpy('keydown handler');
   positionOverrides: ConnectionPositionPair[];
   attachHandler = jasmine.createSpy('attachHandler').and.callFake(() => {
-    this.attachResult =
-        this.connectedOverlayDirective.overlayRef.overlayElement.querySelector('p') as HTMLElement;
+    const overlayElement = this.connectedOverlayDirective.overlayRef.overlayElement;
+    this.attachResult = overlayElement.querySelector('p') as HTMLElement;
   });
   detachHandler = jasmine.createSpy('detachHandler');
   attachResult: HTMLElement;

src/cdk/overlay/overlay-directives.ts

@@ -111,6 +111,8 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
   private _flexibleDimensions = false;
   private _push = false;
   private _backdropSubscription = Subscription.EMPTY;
+  private _attachSubscription = Subscription.EMPTY;
+  private _detachSubscription = Subscription.EMPTY;
   private _offsetX: number;
   private _offsetY: number;
   private _position: FlexibleConnectedPositionStrategy;
@@ -246,11 +248,13 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
   }
 
   ngOnDestroy() {
+    this._attachSubscription.unsubscribe();
+    this._detachSubscription.unsubscribe();
+    this._backdropSubscription.unsubscribe();
+
     if (this._overlayRef) {
       this._overlayRef.dispose();
     }
-
-    this._backdropSubscription.unsubscribe();
   }
 
   ngOnChanges(changes: SimpleChanges) {
@@ -279,9 +283,10 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
       this.positions = defaultPositionList;
     }
 
-    this._overlayRef = this._overlay.create(this._buildConfig());
-
-    this._overlayRef.keydownEvents().subscribe((event: KeyboardEvent) => {
+    const overlayRef = this._overlayRef = this._overlay.create(this._buildConfig());
+    this._attachSubscription = overlayRef.attachments().subscribe(() => this.attach.emit());
+    this._detachSubscription = overlayRef.detachments().subscribe(() => this.detach.emit());
+    overlayRef.keydownEvents().subscribe((event: KeyboardEvent) => {
       this.overlayKeydown.next(event);
 
       if (event.keyCode === ESCAPE && !hasModifierKey(event)) {
@@ -373,7 +378,6 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
 
     if (!this._overlayRef.hasAttached()) {
       this._overlayRef.attach(this._templatePortal);
-      this.attach.emit();
     }
 
     if (this.hasBackdrop) {
@@ -389,7 +393,6 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
   private _detachOverlay() {
     if (this._overlayRef) {
       this._overlayRef.detach();
-      this.detach.emit();
     }
 
     this._backdropSubscription.unsubscribe();