Skip to content

refactor(autocomplete): make keyboard and visibility calls synchronous #6441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions src/cdk/a11y/activedescendant-key-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,13 @@ export class ActiveDescendantKeyManager<T> extends ListKeyManager<Highlightable
* styles from the previously active item.
*/
setActiveItem(index: number): void {
Promise.resolve().then(() => {
if (this.activeItem) {
this.activeItem.setInactiveStyles();
}
super.setActiveItem(index);
if (this.activeItem) {
this.activeItem.setActiveStyles();
}
});
if (this.activeItem) {
this.activeItem.setInactiveStyles();
}
super.setActiveItem(index);
if (this.activeItem) {
this.activeItem.setActiveStyles();
}
}

}
27 changes: 10 additions & 17 deletions src/cdk/a11y/list-key-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,13 +550,12 @@ describe('Key managers', () => {
describe('ActiveDescendantKeyManager', () => {
let keyManager: ActiveDescendantKeyManager<FakeHighlightable>;

beforeEach(fakeAsync(() => {
beforeEach(() => {
itemList.items = [new FakeHighlightable(), new FakeHighlightable(), new FakeHighlightable()];
keyManager = new ActiveDescendantKeyManager<FakeHighlightable>(itemList);

// first item is already focused
keyManager.setFirstItemActive();
tick();

spyOn(itemList.items[0], 'setActiveStyles');
spyOn(itemList.items[1], 'setActiveStyles');
Expand All @@ -565,44 +564,38 @@ describe('Key managers', () => {
spyOn(itemList.items[0], 'setInactiveStyles');
spyOn(itemList.items[1], 'setInactiveStyles');
spyOn(itemList.items[2], 'setInactiveStyles');
}));
});

it('should set subsequent items as active with the DOWN arrow', fakeAsync(() => {
it('should set subsequent items as active with the DOWN arrow', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);
tick();

expect(itemList.items[1].setActiveStyles).toHaveBeenCalled();
expect(itemList.items[2].setActiveStyles).not.toHaveBeenCalled();

keyManager.onKeydown(fakeKeyEvents.downArrow);
tick();

expect(itemList.items[2].setActiveStyles).toHaveBeenCalled();
}));
});

it('should set previous items as active with the UP arrow', fakeAsync(() => {
it('should set previous items as active with the UP arrow', () => {
keyManager.setLastItemActive();
tick();

keyManager.onKeydown(fakeKeyEvents.upArrow);
tick();

expect(itemList.items[1].setActiveStyles).toHaveBeenCalled();
expect(itemList.items[0].setActiveStyles).not.toHaveBeenCalled();

keyManager.onKeydown(fakeKeyEvents.upArrow);
tick();

expect(itemList.items[0].setActiveStyles).toHaveBeenCalled();
}));
});

it('should set inactive styles on previously active items', fakeAsync(() => {
it('should set inactive styles on previously active items', () => {
keyManager.onKeydown(fakeKeyEvents.downArrow);
tick();
expect(itemList.items[0].setInactiveStyles).toHaveBeenCalled();

keyManager.onKeydown(fakeKeyEvents.upArrow);
tick();
expect(itemList.items[1].setInactiveStyles).toHaveBeenCalled();
}));
});

});

Expand Down
7 changes: 7 additions & 0 deletions src/cdk/rxjs/rx-operators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {startWith as startWithOperator} from 'rxjs/operator/startWith';
import {debounceTime as debounceTimeOperator} from 'rxjs/operator/debounceTime';
import {auditTime as auditTimeOperator} from 'rxjs/operator/auditTime';
import {takeUntil as takeUntilOperator} from 'rxjs/operator/takeUntil';
import {delay as delayOperator} from 'rxjs/operator/delay';

/**
* Represents a strongly-typed chain of RxJS operators.
Expand Down Expand Up @@ -71,6 +72,9 @@ export interface StrictRxChain<T> {

call(operator: takeUntilOperatorType<T>, notifier: Observable<any>): StrictRxChain<T>;

call(operator: delayOperatorType<T>, delay: number | Date, scheduler?: IScheduler):
StrictRxChain<T>;

subscribe(fn: (t: T) => void): Subscription;

result(): Observable<T>;
Expand All @@ -89,6 +93,7 @@ export class StartWithBrand { private _; }
export class DebounceTimeBrand { private _; }
export class AuditTimeBrand { private _; }
export class TakeUntilBrand { private _; }
export class DelayBrand { private _; }
/* tslint:enable:no-unused-variable */


Expand All @@ -104,6 +109,7 @@ export type startWithOperatorType<T> = typeof startWithOperator & StartWithBrand
export type debounceTimeOperatorType<T> = typeof debounceTimeOperator & DebounceTimeBrand;
export type auditTimeOperatorType<T> = typeof auditTimeOperator & AuditTimeBrand;
export type takeUntilOperatorType<T> = typeof takeUntilOperator & TakeUntilBrand;
export type delayOperatorType<T> = typeof delayOperator & DelayBrand;

// We add `Function` to the type intersection to make this nomically different from
// `finallyOperatorType` while still being structurally the same. Without this, TypeScript tries to
Expand All @@ -123,3 +129,4 @@ export const debounceTime =
debounceTimeOperator as typeof debounceTimeOperator & DebounceTimeBrand & Function;
export const auditTime = auditTimeOperator as typeof auditTimeOperator & AuditTimeBrand & Function;
export const takeUntil = takeUntilOperator as typeof takeUntilOperator & TakeUntilBrand & Function;
export const delay = delayOperator as typeof delayOperator & DelayBrand & Function;
23 changes: 12 additions & 11 deletions src/lib/autocomplete/autocomplete-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
ScrollStrategy,
} from '@angular/cdk/overlay';
import {TemplatePortal} from '@angular/cdk/portal';
import {filter, first, map, RxChain, switchMap} from '@angular/cdk/rxjs';
import {filter, first, RxChain, switchMap, doOperator, delay} from '@angular/cdk/rxjs';
import {
ChangeDetectorRef,
Directive,
Expand Down Expand Up @@ -284,11 +284,9 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
this.openPanel();
}

Promise.resolve().then(() => {
if (isArrowKey || this.autocomplete._keyManager.activeItem !== prevActiveItem) {
this._scrollToOption();
}
});
if (isArrowKey || this.autocomplete._keyManager.activeItem !== prevActiveItem) {
this._scrollToOption();
}
}
}

Expand Down Expand Up @@ -356,9 +354,8 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
this.autocomplete._setScrollTop(optionOffset);
} else if (optionOffset + AUTOCOMPLETE_OPTION_HEIGHT > panelTop + AUTOCOMPLETE_PANEL_HEIGHT) {
// Scroll down to reveal selected option scrolled below the panel bottom
const newScrollTop =
Math.max(0, optionOffset - AUTOCOMPLETE_PANEL_HEIGHT + AUTOCOMPLETE_OPTION_HEIGHT);
this.autocomplete._setScrollTop(newScrollTop);
const newScrollTop = optionOffset - AUTOCOMPLETE_PANEL_HEIGHT + AUTOCOMPLETE_OPTION_HEIGHT;
this.autocomplete._setScrollTop(Math.max(0, newScrollTop));
}
}

Expand All @@ -368,8 +365,12 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy {
*/
private _subscribeToClosingActions(): Subscription {
const firstStable = first.call(this._zone.onStable.asObservable());
const optionChanges = map.call(this.autocomplete.options.changes, () =>
this._positionStrategy.recalculateLastPosition());
const optionChanges = RxChain.from(this.autocomplete.options.changes)
.call(doOperator, () => this._positionStrategy.recalculateLastPosition())
// Defer emitting to the stream until the next tick, because changing
// bindings in here will cause "changed after checked" errors.
.call(delay, 0)
.result();

// When the zone is stable initially, and when the option list changes...
return RxChain.from(merge(firstStable, optionChanges))
Expand Down
Loading