From 7fe651b88a214dad3f9f93bf4ac06a67134a9673 Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@users.noreply.github.com>
Date: Thu, 1 Feb 2018 20:23:51 +0100
Subject: [PATCH] fix(overlay): onPositionChange stream not being completed
 (#8562)

Completes the `onPositionChange` stream when the position strategy is disposed. Also cleans up a few cases that were unsubscribing from it explicitly.
---
 src/cdk/overlay/overlay-directives.ts            |  5 +----
 .../position/connected-position-strategy.spec.ts | 16 ++++++++++++++++
 .../position/connected-position-strategy.ts      |  1 +
 src/lib/menu/menu-trigger.ts                     |  4 +---
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/cdk/overlay/overlay-directives.ts b/src/cdk/overlay/overlay-directives.ts
index 7d4c7bc748a..22ed6b8f020 100644
--- a/src/cdk/overlay/overlay-directives.ts
+++ b/src/cdk/overlay/overlay-directives.ts
@@ -98,7 +98,6 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
   private _templatePortal: TemplatePortal;
   private _hasBackdrop = false;
   private _backdropSubscription = Subscription.EMPTY;
-  private _positionSubscription = Subscription.EMPTY;
   private _offsetX: number = 0;
   private _offsetY: number = 0;
   private _position: ConnectedPositionStrategy;
@@ -369,8 +368,7 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
       );
     }
 
-    this._positionSubscription = strategy.onPositionChange
-        .subscribe(pos => this.positionChange.emit(pos));
+    strategy.onPositionChange.subscribe(pos => this.positionChange.emit(pos));
 
     return strategy;
   }
@@ -419,6 +417,5 @@ export class CdkConnectedOverlay implements OnDestroy, OnChanges {
     }
 
     this._backdropSubscription.unsubscribe();
-    this._positionSubscription.unsubscribe();
   }
 }
diff --git a/src/cdk/overlay/position/connected-position-strategy.spec.ts b/src/cdk/overlay/position/connected-position-strategy.spec.ts
index 4a77926d5b9..06b8601d6f8 100644
--- a/src/cdk/overlay/position/connected-position-strategy.spec.ts
+++ b/src/cdk/overlay/position/connected-position-strategy.spec.ts
@@ -407,6 +407,22 @@ describe('ConnectedPositionStrategy', () => {
       subscription.unsubscribe();
     });
 
+    it('should complete the onPositionChange stream on dispose', () => {
+      strategy = positionBuilder.connectedTo(
+          fakeElementRef,
+          {originX: 'end', originY: 'bottom'},
+          {overlayX: 'start', overlayY: 'top'});
+
+      const completeHandler = jasmine.createSpy('complete handler');
+
+      strategy.onPositionChange.subscribe(undefined, undefined, completeHandler);
+      strategy.attach(fakeOverlayRef(overlayElement));
+      strategy.apply();
+      strategy.dispose();
+
+      expect(completeHandler).toHaveBeenCalled();
+    });
+
     it('should pick the fallback position that shows the largest area of the element', () => {
       originElement.style.top = '200px';
       originElement.style.right = '25px';
diff --git a/src/cdk/overlay/position/connected-position-strategy.ts b/src/cdk/overlay/position/connected-position-strategy.ts
index ad8561e1742..a938535cdb4 100644
--- a/src/cdk/overlay/position/connected-position-strategy.ts
+++ b/src/cdk/overlay/position/connected-position-strategy.ts
@@ -108,6 +108,7 @@ export class ConnectedPositionStrategy implements PositionStrategy {
   dispose() {
     this._applied = false;
     this._resizeSubscription.unsubscribe();
+    this._onPositionChange.complete();
   }
 
   /** @docs-private */
diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts
index bf5e454642d..c680cfbf338 100644
--- a/src/lib/menu/menu-trigger.ts
+++ b/src/lib/menu/menu-trigger.ts
@@ -87,7 +87,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
   private _overlayRef: OverlayRef | null = null;
   private _menuOpen: boolean = false;
   private _closeSubscription = Subscription.EMPTY;
-  private _positionSubscription = Subscription.EMPTY;
   private _hoverSubscription = Subscription.EMPTY;
 
   // Tracking input type is necessary so it's possible to only auto-focus
@@ -353,7 +352,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
    * correct, even if a fallback position is used for the overlay.
    */
   private _subscribeToPositions(position: ConnectedPositionStrategy): void {
-    this._positionSubscription = position.onPositionChange.subscribe(change => {
+    position.onPositionChange.subscribe(change => {
       const posX: MenuPositionX = change.connectionPair.overlayX === 'start' ? 'after' : 'before';
       const posY: MenuPositionY = change.connectionPair.overlayY === 'top' ? 'below' : 'above';
 
@@ -408,7 +407,6 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
   /** Cleans up the active subscriptions. */
   private _cleanUpSubscriptions(): void {
     this._closeSubscription.unsubscribe();
-    this._positionSubscription.unsubscribe();
     this._hoverSubscription.unsubscribe();
   }