From a8cac46a6f5d941f856a08faa2530ab5d1172734 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sun, 23 Jul 2017 15:35:23 +0300 Subject: [PATCH] fix(connected-position-strategy): position change event not emitting for fallback positions * 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. --- src/lib/autocomplete/autocomplete-trigger.ts | 21 ++--- .../connected-position-strategy.spec.ts | 89 +++++++++++-------- .../position/connected-position-strategy.ts | 10 +-- 3 files changed, 63 insertions(+), 57 deletions(-) diff --git a/src/lib/autocomplete/autocomplete-trigger.ts b/src/lib/autocomplete/autocomplete-trigger.ts index 6ce9c1040cc8..68c059872f8c 100644 --- a/src/lib/autocomplete/autocomplete-trigger.ts +++ b/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 @@ -380,12 +380,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... @@ -482,14 +487,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(); - } - } diff --git a/src/lib/core/overlay/position/connected-position-strategy.spec.ts b/src/lib/core/overlay/position/connected-position-strategy.spec.ts index 054be40dee51..b1ef8baeb505 100644 --- a/src/lib/core/overlay/position/connected-position-strategy.spec.ts +++ b/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; diff --git a/src/lib/core/overlay/position/connected-position-strategy.ts b/src/lib/core/overlay/position/connected-position-strategy.ts index 6539053f5332..0c1c3a532f91 100644 --- a/src/lib/core/overlay/position/connected-position-strategy.ts +++ b/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. */