Skip to content

Commit

Permalink
fix(connected-position-strategy): position change event not emitting …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@crisbeto @andrewseguin
crisbeto authored and andrewseguin committed Jul 27, 2017
1 parent a51f82f commit 63505dc
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 57 deletions.
21 changes: 8 additions & 13 deletions src/lib/autocomplete/autocomplete-trigger.ts
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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...
Expand Down Expand Up @@ -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();
}

}
89 changes: 50 additions & 39 deletions src/lib/core/overlay/position/connected-position-strategy.spec.ts
View file
Expand Up @@ -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';


Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
10 changes: 5 additions & 5 deletions src/lib/core/overlay/position/connected-position-strategy.ts
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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. */
Expand Down

0 comments on commit 63505dc

Please sign in to comment.