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

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

@@ -4,17 +4,26 @@ import {ComponentFixture, TestBed, async, inject} from '@angular/core/testing';
 import {Directionality} from '@angular/cdk/bidi';
 import {dispatchKeyboardEvent} from '@angular/cdk/testing';
 import {ESCAPE} from '@angular/cdk/keycodes';
-import {ConnectedOverlayDirective, OverlayModule, OverlayOrigin} from './index';
-import {OverlayContainer} from './overlay-container';
-import {ConnectedPositionStrategy} from './position/connected-position-strategy';
-import {ConnectedOverlayPositionChange} from './position/connected-position';
+import {
+  ConnectedOverlayDirective,
+  OverlayModule,
+  OverlayOrigin,
+  Overlay,
+  ScrollStrategy,
+  ScrollDispatcher,
+  OverlayContainer,
+  ConnectedPositionStrategy,
+  ConnectedOverlayPositionChange,
+} from './index';
+import {Subject} from 'rxjs/Subject';
 
 
 describe('Overlay directives', () => {
   let overlayContainer: OverlayContainer;
   let overlayContainerElement: HTMLElement;
   let fixture: ComponentFixture<ConnectedOverlayDirectiveTest>;
   let dir: {value: string};
+  let scrolledSubject = new Subject();
 
   beforeEach(() => {
     TestBed.configureTestingModule({
@@ -23,7 +32,10 @@ describe('Overlay directives', () => {
       providers: [
         {provide: Directionality, useFactory: () => {
           return dir = {value: 'ltr'};
-        }}
+        }},
+        {provide: ScrollDispatcher, useFactory: () => ({
+          scrolled: () => scrolledSubject.asObservable()
+        })}
       ],
     });
   });
@@ -253,7 +265,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();
@@ -266,7 +278,7 @@ describe('Overlay directives', () => {
       expect(fixture.componentInstance.backdropClicked).toBe(true);
     });
 
-    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();
@@ -279,32 +291,54 @@ 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;
       fixture.detectChanges();
       expect(fixture.componentInstance.detachHandler).toHaveBeenCalled();
     });
 
+    it('should emit when detached externally', inject([Overlay], (overlay: Overlay) => {
+      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();
+    }));
+
   });
 
 });
 
-
 @Component({
   template: `
   <button cdk-overlay-origin #trigger="cdkOverlayOrigin">Toggle menu</button>
   <ng-template cdk-connected-overlay [open]="isOpen" [width]="width" [height]="height"
-            [origin]="trigger"
+            [origin]="trigger" [scrollStrategy]="scrollStrategy"
             [hasBackdrop]="hasBackdrop" backdropClass="mat-test-class"
             (backdropClick)="backdropClicked=true" [offsetX]="offsetX" [offsetY]="offsetY"
             (positionChange)="positionChangeHandler($event)" (attach)="attachHandler()"
@@ -322,10 +356,11 @@ class ConnectedOverlayDirectiveTest {
   offsetY: number = 0;
   hasBackdrop: boolean;
   backdropClicked = false;
+  scrollStrategy: ScrollStrategy;
   positionChangeHandler = jasmine.createSpy('positionChangeHandler');
   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

@@ -268,6 +268,8 @@ export class ConnectedOverlayDirective implements OnDestroy, OnChanges {
     }
 
     this._overlayRef = this._overlay.create(this._buildConfig());
+    this._overlayRef.attachments().subscribe(() => this.attach.emit());
+    this._overlayRef.detachments().subscribe(() => this.detach.emit());
   }
 
   /** Builds the overlay config based on the directive's inputs */
@@ -342,7 +344,6 @@ export class ConnectedOverlayDirective implements OnDestroy, OnChanges {
 
     if (!this._overlayRef.hasAttached()) {
       this._overlayRef.attach(this._templatePortal);
-      this.attach.emit();
     }
 
     if (this.hasBackdrop) {
@@ -356,7 +357,6 @@ export class ConnectedOverlayDirective implements OnDestroy, OnChanges {
   private _detachOverlay() {
     if (this._overlayRef) {
       this._overlayRef.detach();
-      this.detach.emit();
     }
 
     this._backdropSubscription.unsubscribe();