fix(connected-position-strategy): position change event not emitting for fallback positions (#5978)

* Fixes the `onPositionChange` event from the `ConnectedPositionStrategy` not emitting if the strategy picks one of the fallback positions.
* Fixes the autocomplete panel not having the proper offset when it is in the `above` position. This was tightly-coupled to the `onPositionChange` not firing.
* Fixes a couple of tests that were nested in another test, causing them not to be executed. I've removed one of the tests since it was also duplicated with one above. This seems to have been the result of a faulty conflict resolution.

Author: crisbeto

src/lib/autocomplete/autocomplete-trigger.ts

@@ -45,7 +45,7 @@ import {Subscription} from 'rxjs/Subscription';
 import {merge} from 'rxjs/observable/merge';
 import {fromEvent} from 'rxjs/observable/fromEvent';
 import {of as observableOf} from 'rxjs/observable/of';
-import {RxChain, switchMap, first, filter} from '../core/rxjs/index';
+import {RxChain, switchMap, first, filter, map} from '../core/rxjs/index';
 
 /**
  * The following style constants are necessary to save here in order
@@ -379,12 +379,17 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
    * stream every time the option list changes.
    */
   private _subscribeToClosingActions(): Subscription {
+    const firstStable = first.call(this._zone.onStable);
+    const optionChanges = map.call(this.autocomplete.options.changes, () =>
+      this._positionStrategy.recalculateLastPosition());
+
     // When the zone is stable initially, and when the option list changes...
-    return RxChain.from(merge(first.call(this._zone.onStable), this.autocomplete.options.changes))
+    return RxChain.from(merge(firstStable, optionChanges))
       // create a new stream of panelClosingActions, replacing any previous streams
       // that were created, and flatten it so our stream only emits closing events...
       .call(switchMap, () => {
-        this._resetPanel();
+        this._resetActiveItem();
+        this.autocomplete._setVisibility();
         return this.panelClosingActions;
       })
       // when the first closing event occurs...
@@ -481,14 +486,4 @@ export class MdAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
     this.autocomplete._keyManager.setActiveItem(-1);
   }
 
-  /**
-   * Resets the active item and re-calculates alignment of the panel in case its size
-   * has changed due to fewer or greater number of options.
-   */
-  private _resetPanel() {
-    this._resetActiveItem();
-    this._positionStrategy.recalculateLastPosition();
-    this.autocomplete._setVisibility();
-  }
-
 }

src/lib/core/overlay/position/connected-position-strategy.spec.ts

@@ -6,7 +6,6 @@ import {OverlayPositionBuilder} from './overlay-position-builder';
 import {ConnectedOverlayPositionChange} from './connected-position';
 import {Scrollable} from '../scroll/scrollable';
 import {Subscription} from 'rxjs/Subscription';
-import Spy = jasmine.Spy;
 import {ScrollDispatchModule} from '../scroll/index';
 
 
@@ -343,53 +342,65 @@ describe('ConnectedPositionStrategy', () => {
       expect(latestCall.args[0] instanceof ConnectedOverlayPositionChange)
           .toBe(true, `Expected strategy to emit an instance of ConnectedOverlayPositionChange.`);
 
-      it('should pick the fallback position that shows the largest area of the element', () => {
-        positionBuilder = new OverlayPositionBuilder(viewportRuler);
+      // If the strategy is re-applied and the initial position would now fit,
+      // the position change event should be emitted again.
+      originElement.style.top = '200px';
+      originElement.style.left = '200px';
+      strategy.apply(overlayElement);
 
-        originElement.style.top = '200px';
-        originElement.style.right = '25px';
-        originRect = originElement.getBoundingClientRect();
+      expect(positionChangeHandler).toHaveBeenCalledTimes(2);
 
-        strategy = positionBuilder.connectedTo(
-            fakeElementRef,
-            {originX: 'end', originY: 'center'},
-            {overlayX: 'start', overlayY: 'center'})
-            .withFallbackPosition(
-                {originX: 'end', originY: 'top'},
-                {overlayX: 'start', overlayY: 'bottom'})
-            .withFallbackPosition(
-                {originX: 'end', originY: 'top'},
-                {overlayX: 'end', overlayY: 'top'});
+      subscription.unsubscribe();
+    });
 
-        strategy.apply(overlayElement);
+    it('should emit the onPositionChange event even if none of the positions fit', () => {
+      positionBuilder = new OverlayPositionBuilder(viewportRuler);
+      originElement.style.bottom = '25px';
+      originElement.style.right = '25px';
 
-        let overlayRect = overlayElement.getBoundingClientRect();
+      strategy = positionBuilder.connectedTo(
+          fakeElementRef,
+          {originX: 'end', originY: 'bottom'},
+          {overlayX: 'start', overlayY: 'top'})
+          .withFallbackPosition(
+              {originX: 'start', originY: 'bottom'},
+              {overlayX: 'end', overlayY: 'top'});
 
-        expect(Math.floor(overlayRect.top)).toBe(Math.floor(originRect.top));
-        expect(Math.floor(overlayRect.left)).toBe(Math.floor(originRect.left));
-      });
+      const positionChangeHandler = jasmine.createSpy('positionChangeHandler');
+      const subscription = strategy.onPositionChange.subscribe(positionChangeHandler);
 
-      it('should position a panel properly when rtl', () => {
-        // must make the overlay longer than the origin to properly test attachment
-        overlayElement.style.width = `500px`;
-        originRect = originElement.getBoundingClientRect();
-        strategy = positionBuilder.connectedTo(
-            fakeElementRef,
-            {originX: 'start', originY: 'bottom'},
-            {overlayX: 'start', overlayY: 'top'})
-            .withDirection('rtl');
-        originElement.style.top = '0';
-        originElement.style.left = '0';
+      strategy.apply(overlayElement);
 
-        // If the strategy is re-applied and the initial position would now fit,
-        // the position change event should be emitted again.
-        strategy.apply(overlayElement);
-        expect(positionChangeHandler).toHaveBeenCalledTimes(2);
+      expect(positionChangeHandler).toHaveBeenCalled();
 
-        subscription.unsubscribe();
-      });
+      subscription.unsubscribe();
     });
 
+    it('should pick the fallback position that shows the largest area of the element', () => {
+      positionBuilder = new OverlayPositionBuilder(viewportRuler);
+
+      originElement.style.top = '200px';
+      originElement.style.right = '25px';
+      originRect = originElement.getBoundingClientRect();
+
+      strategy = positionBuilder.connectedTo(
+          fakeElementRef,
+          {originX: 'end', originY: 'center'},
+          {overlayX: 'start', overlayY: 'center'})
+          .withFallbackPosition(
+              {originX: 'end', originY: 'top'},
+              {overlayX: 'start', overlayY: 'bottom'})
+          .withFallbackPosition(
+              {originX: 'end', originY: 'top'},
+              {overlayX: 'end', overlayY: 'top'});
+
+      strategy.apply(overlayElement);
+
+      let overlayRect = overlayElement.getBoundingClientRect();
+
+      expect(Math.floor(overlayRect.top)).toBe(Math.floor(originRect.top));
+      expect(Math.floor(overlayRect.left)).toBe(Math.floor(originRect.left));
+    });
 
     /**
      * Run all tests for connecting the overlay to the origin such that first preferred
@@ -485,7 +496,7 @@ describe('ConnectedPositionStrategy', () => {
     let strategy: ConnectedPositionStrategy;
 
     let scrollable: HTMLDivElement;
-    let positionChangeHandler: Spy;
+    let positionChangeHandler: jasmine.Spy;
     let onPositionChangeSubscription: Subscription;
     let positionChange: ConnectedOverlayPositionChange;
 

src/lib/core/overlay/position/connected-position-strategy.ts

@@ -133,11 +133,6 @@ export class ConnectedPositionStrategy implements PositionStrategy {
         // Save the last connected position in case the position needs to be re-calculated.
         this._lastConnectedPosition = pos;
 
-        // Notify that the position has been changed along with its change properties.
-        const scrollableViewProperties = this.getScrollableViewProperties(element);
-        const positionChange = new ConnectedOverlayPositionChange(pos, scrollableViewProperties);
-        this._onPositionChange.next(positionChange);
-
         return;
       } else if (!fallbackPoint || fallbackPoint.visibleArea < overlayPoint.visibleArea) {
         fallbackPoint = overlayPoint;
@@ -395,6 +390,11 @@ export class ConnectedPositionStrategy implements PositionStrategy {
 
     element.style[verticalStyleProperty] = `${y}px`;
     element.style[horizontalStyleProperty] = `${x}px`;
+
+    // Notify that the position has been changed along with its change properties.
+    const scrollableViewProperties = this.getScrollableViewProperties(element);
+    const positionChange = new ConnectedOverlayPositionChange(pos, scrollableViewProperties);
+    this._onPositionChange.next(positionChange);
   }
 
   /** Returns the bounding positions of the provided element with respect to the viewport. */