From fd5e4d9b9fe325eca793529f11a65a13f9380600 Mon Sep 17 00:00:00 2001 From: Kara Date: Fri, 27 Jan 2017 14:16:43 -0800 Subject: [PATCH] fix(autocomplete): up arrow should set last item active (#2776) --- src/lib/autocomplete/autocomplete-trigger.ts | 14 +++--- src/lib/autocomplete/autocomplete.spec.ts | 51 ++++++++++++++------ src/lib/core/a11y/list-key-manager.spec.ts | 38 +++++++++++++++ src/lib/core/a11y/list-key-manager.ts | 5 +- 4 files changed, 83 insertions(+), 25 deletions(-) diff --git a/src/lib/autocomplete/autocomplete-trigger.ts b/src/lib/autocomplete/autocomplete-trigger.ts index 3e0453c51ced..e6e74ef328fb 100644 --- a/src/lib/autocomplete/autocomplete-trigger.ts +++ b/src/lib/autocomplete/autocomplete-trigger.ts @@ -48,7 +48,7 @@ export class MdAutocompleteTrigger implements AfterContentInit, OnDestroy { private _panelOpen: boolean = false; /** The subscription to positioning changes in the autocomplete panel. */ - private _panelPositionSub: Subscription; + private _panelPositionSubscription: Subscription; /** Manages active item in option list based on key events. */ private _keyManager: ActiveDescendantKeyManager; @@ -62,12 +62,12 @@ export class MdAutocompleteTrigger implements AfterContentInit, OnDestroy { @Optional() private _controlDir: NgControl, @Optional() private _dir: Dir) {} ngAfterContentInit() { - this._keyManager = new ActiveDescendantKeyManager(this.autocomplete.options); + this._keyManager = new ActiveDescendantKeyManager(this.autocomplete.options).withWrap(); } ngOnDestroy() { - if (this._panelPositionSub) { - this._panelPositionSub.unsubscribe(); + if (this._panelPositionSubscription) { + this._panelPositionSubscription.unsubscribe(); } this._destroyPanel(); @@ -225,7 +225,7 @@ export class MdAutocompleteTrigger implements AfterContentInit, OnDestroy { * y-offset can be adjusted to match the new position. */ private _subscribeToPositionChanges(strategy: ConnectedPositionStrategy) { - this._panelPositionSub = strategy.onPositionChange.subscribe(change => { + this._panelPositionSubscription = strategy.onPositionChange.subscribe(change => { this.autocomplete.positionY = change.connectionPair.originY === 'top' ? 'above' : 'below'; }); } @@ -235,9 +235,9 @@ export class MdAutocompleteTrigger implements AfterContentInit, OnDestroy { return this._element.nativeElement.getBoundingClientRect().width; } - /** Reset active item to -1 so DOWN_ARROW event will activate the first option.*/ + /** Reset active item to null so arrow events will activate the correct options.*/ private _resetActiveItem(): void { - this._keyManager.setActiveItem(-1); + this._keyManager.setActiveItem(null); } /** diff --git a/src/lib/autocomplete/autocomplete.spec.ts b/src/lib/autocomplete/autocomplete.spec.ts index 246f109f7761..9b62e2d80987 100644 --- a/src/lib/autocomplete/autocomplete.spec.ts +++ b/src/lib/autocomplete/autocomplete.spec.ts @@ -7,9 +7,11 @@ import {MdInputModule} from '../input/index'; import {Dir, LayoutDirection} from '../core/rtl/dir'; import {FormControl, ReactiveFormsModule} from '@angular/forms'; import {Subscription} from 'rxjs/Subscription'; -import {ENTER, DOWN_ARROW, SPACE} from '../core/keyboard/keycodes'; +import {ENTER, DOWN_ARROW, SPACE, UP_ARROW} from '../core/keyboard/keycodes'; import {MdOption} from '../core/option/option'; import {ViewportRuler} from '../core/overlay/position/viewport-ruler'; +import {FakeViewportRuler} from '../core/overlay/position/fake-viewport-ruler'; + describe('MdAutocomplete', () => { let overlayContainerElement: HTMLElement; @@ -294,6 +296,36 @@ describe('MdAutocomplete', () => { }); })); + it('should set the active item to the last option when UP key is pressed', async(() => { + fixture.componentInstance.trigger.openPanel(); + fixture.detectChanges(); + + const optionEls = + overlayContainerElement.querySelectorAll('md-option') as NodeListOf; + + const UP_ARROW_EVENT = new FakeKeyboardEvent(UP_ARROW) as KeyboardEvent; + fixture.componentInstance.trigger._handleKeydown(UP_ARROW_EVENT); + + fixture.whenStable().then(() => { + fixture.detectChanges(); + expect(fixture.componentInstance.trigger.activeOption) + .toBe(fixture.componentInstance.options.last, 'Expected last option to be active.'); + expect(optionEls[10].classList).toContain('md-active'); + expect(optionEls[0].classList).not.toContain('md-active'); + + fixture.componentInstance.trigger._handleKeydown(DOWN_ARROW_EVENT); + + fixture.whenStable().then(() => { + fixture.detectChanges(); + expect(fixture.componentInstance.trigger.activeOption) + .toBe(fixture.componentInstance.options.first, + 'Expected first option to be active.'); + expect(optionEls[0].classList).toContain('md-active'); + expect(optionEls[10].classList).not.toContain('md-active'); + }); + }); + })); + it('should set the active item properly after filtering', async(() => { fixture.componentInstance.trigger.openPanel(); fixture.detectChanges(); @@ -532,7 +564,7 @@ describe('MdAutocomplete', () => { it('should fall back to above position if panel cannot fit below', () => { // Push the autocomplete trigger down so it won't have room to open "below" - input.style.top = '400px'; + input.style.top = '600px'; input.style.position = 'relative'; fixture.componentInstance.trigger.openPanel(); @@ -551,7 +583,7 @@ describe('MdAutocomplete', () => { it('should align panel properly when filtering in "above" position', () => { // Push the autocomplete trigger down so it won't have room to open "below" - input.style.top = '400px'; + input.style.top = '600px'; input.style.position = 'relative'; fixture.componentInstance.trigger.openPanel(); @@ -645,16 +677,3 @@ class FakeKeyboardEvent { constructor(public keyCode: number) {} preventDefault() {} } - -class FakeViewportRuler { - getViewportRect() { - return { - left: 0, top: 0, width: 500, height: 500, bottom: 500, right: 500 - }; - } - - getViewportScrollPosition() { - return {top: 0, left: 0}; - } -} - diff --git a/src/lib/core/a11y/list-key-manager.spec.ts b/src/lib/core/a11y/list-key-manager.spec.ts index da36d2e8b99f..2f6bd8657f8e 100644 --- a/src/lib/core/a11y/list-key-manager.spec.ts +++ b/src/lib/core/a11y/list-key-manager.spec.ts @@ -87,6 +87,17 @@ describe('Key managers', () => { expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(0); }); + it('should set first item active when down arrow pressed if no active item', () => { + keyManager.setActiveItem(null); + keyManager.onKeydown(DOWN_ARROW_EVENT); + + expect(keyManager.activeItemIndex) + .toBe(0, 'Expected active item to be 0 after down key if active item was null.'); + expect(keyManager.setActiveItem).toHaveBeenCalledWith(0); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(1); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(2); + }); + it('should set previous items as active when up arrow is pressed', () => { keyManager.onKeydown(DOWN_ARROW_EVENT); @@ -101,6 +112,17 @@ describe('Key managers', () => { expect(keyManager.setActiveItem).toHaveBeenCalledWith(0); }); + it('should do nothing when up arrow is pressed if no active item and not wrap', () => { + keyManager.setActiveItem(null); + keyManager.onKeydown(UP_ARROW_EVENT); + + expect(keyManager.activeItemIndex) + .toBe(null, 'Expected nothing to happen if up arrow occurs and no active item.'); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(0); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(1); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(2); + }); + it('should skip disabled items using arrow keys', () => { itemList.items[1].disabled = true; @@ -347,6 +369,22 @@ describe('Key managers', () => { expect(keyManager.activeItemIndex).toBe(2, 'Expected active item to wrap to end.'); }); + it('should set last item active when up arrow is pressed if no active item', () => { + keyManager.withWrap(); + keyManager.setActiveItem(null); + keyManager.onKeydown(UP_ARROW_EVENT); + + expect(keyManager.activeItemIndex) + .toBe(2, 'Expected last item to be active on up arrow if no active item.'); + expect(keyManager.setActiveItem).not.toHaveBeenCalledWith(0); + expect(keyManager.setActiveItem).toHaveBeenCalledWith(2); + + keyManager.onKeydown(DOWN_ARROW_EVENT); + expect(keyManager.activeItemIndex) + .toBe(0, 'Expected active item to be 0 after wrapping back to beginning.'); + expect(keyManager.setActiveItem).toHaveBeenCalledWith(0); + }); + }); }); diff --git a/src/lib/core/a11y/list-key-manager.ts b/src/lib/core/a11y/list-key-manager.ts index 01004d2239d4..308e10a67dca 100644 --- a/src/lib/core/a11y/list-key-manager.ts +++ b/src/lib/core/a11y/list-key-manager.ts @@ -96,12 +96,13 @@ export class ListKeyManager { /** Sets the active item to the next enabled item in the list. */ setNextItemActive(): void { - this._setActiveItemByDelta(1); + this._activeItemIndex === null ? this.setFirstItemActive() : this._setActiveItemByDelta(1); } /** Sets the active item to a previous enabled item in the list. */ setPreviousItemActive(): void { - this._setActiveItemByDelta(-1); + this._activeItemIndex === null && this._wrap ? this.setLastItemActive() + : this._setActiveItemByDelta(-1); } /**