fix(autocomplete): up arrow should set last item active (#2776)

Author: kara

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);
   }
 
   /**

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<HTMLElement>;
+
+      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};
-  }
-}
-

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);
+      });
+
     });
 
   });

src/lib/core/a11y/list-key-manager.ts

@@ -96,12 +96,13 @@ export class ListKeyManager<T extends CanDisable> {
 
   /** 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);
   }
 
   /**