fix(select): not scrolling active option into view when typing (#7620)

* Fixes an issue that caused the active option not to be scrolled into view if it was changed via typing. The issue got introduced after we switched to using `aria-activedescendant` for focus management and it happens because the key presses in typeahead mode are debounced in order to allow the user to type in longer words.
* Introduces a `change` subject on the `ListKeyManager`, allowing users to subscribe to changes. This wasn't necessary before, because everything was synchronous, but it is now because of the typeahead.
* Fixes a potential memory leak in the select due to the `onStable` subscription.
* Refactors the selector to remove the need for managing subscriptions manually. Uses a `takeUntil(destroyed)` instead.

Author: crisbeto

src/cdk/a11y/list-key-manager.spec.ts

@@ -242,6 +242,19 @@ describe('Key managers', () => {
         expect(keyManager.activeItemIndex).toBe(0, 'Expected first item to become active.');
       });
 
+      it('should emit an event whenever the active item changes', () => {
+        const spy = jasmine.createSpy('change spy');
+        const subscription = keyManager.change.subscribe(spy);
+
+        keyManager.onKeydown(fakeKeyEvents.downArrow);
+        expect(spy).toHaveBeenCalledTimes(1);
+
+        keyManager.onKeydown(fakeKeyEvents.upArrow);
+        expect(spy).toHaveBeenCalledTimes(2);
+
+        subscription.unsubscribe();
+      });
+
     });
 
     describe('programmatic focus', () => {

src/cdk/a11y/list-key-manager.ts

@@ -42,6 +42,9 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
    */
   tabOut: Subject<void> = new Subject<void>();
 
+  /** Stream that emits whenever the active item of the list manager changes. */
+  change = new Subject<number>();
+
   /**
    * Turns on wrapping mode, which ensures that the active item will wrap to
    * the other end of list when there are no more items in the given direction.
@@ -98,6 +101,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
   setActiveItem(index: number): void {
     this._activeItemIndex = index;
     this._activeItem = this._items.toArray()[index];
+    this.change.next(index);
   }
 
   /**

src/lib/select/select.spec.ts

@@ -3,7 +3,13 @@ import {DOWN_ARROW, END, ENTER, HOME, SPACE, TAB, UP_ARROW} from '@angular/cdk/k
 import {OverlayContainer} from '@angular/cdk/overlay';
 import {Platform} from '@angular/cdk/platform';
 import {ScrollDispatcher, ViewportRuler} from '@angular/cdk/scrolling';
-import {dispatchFakeEvent, dispatchKeyboardEvent, wrappedErrorMessage} from '@angular/cdk/testing';
+import {
+  dispatchFakeEvent,
+  dispatchEvent,
+  createKeyboardEvent,
+  dispatchKeyboardEvent,
+  wrappedErrorMessage,
+} from '@angular/cdk/testing';
 import {
   ChangeDetectionStrategy,
   Component,
@@ -3158,6 +3164,47 @@ describe('MatSelect', () => {
       // <(option index + group labels) * height> - <panel height> = (9 + 3) * 48 - 256 = 320
       expect(panel.scrollTop).toBe(320, 'Expected scroll to be at the 9th option.');
     }));
+
+    it('should scroll top the top when pressing HOME', fakeAsync(() => {
+      for (let i = 0; i < 20; i++) {
+        dispatchKeyboardEvent(host, 'keydown', DOWN_ARROW);
+        tick();
+        fixture.detectChanges();
+      }
+
+      expect(panel.scrollTop).toBeGreaterThan(0, 'Expected panel to be scrolled down.');
+
+      dispatchKeyboardEvent(host, 'keydown', HOME);
+      tick();
+      fixture.detectChanges();
+
+      expect(panel.scrollTop).toBe(0, 'Expected panel to be scrolled to the top');
+    }));
+
+    it('should scroll to the bottom of the panel when pressing END', fakeAsync(() => {
+      dispatchKeyboardEvent(host, 'keydown', END);
+      tick();
+      fixture.detectChanges();
+
+      // <option amount> * <option height> - <panel height> = 30 * 48 - 256 = 1184
+      expect(panel.scrollTop).toBe(1184, 'Expected panel to be scrolled to the bottom');
+    }));
+
+    it('should scroll to the active option when typing', fakeAsync(() => {
+      for (let i = 0; i < 15; i++) {
+        // Press the letter 'o' 15 times since all the options are named 'Option <index>'
+        dispatchEvent(host, createKeyboardEvent('keydown', 79, undefined, 'o'));
+        tick();
+        fixture.detectChanges();
+        tick(200);
+      }
+
+      tick(200);
+
+      // <option index * height> - <panel height> = 16 * 48 - 256 = 512
+      expect(panel.scrollTop).toBe(512, 'Expected scroll to be at the 16th option.');
+    }));
+
   });
 });
 

src/lib/select/select.ts

@@ -18,7 +18,7 @@ import {
   ScrollStrategy,
   ViewportRuler,
 } from '@angular/cdk/overlay';
-import {filter, first, startWith} from '@angular/cdk/rxjs';
+import {filter, first, startWith, takeUntil, RxChain} from '@angular/cdk/rxjs';
 import {
   AfterContentInit,
   Attribute,
@@ -67,7 +67,6 @@ import {MatFormField, MatFormFieldControl} from '@angular/material/form-field';
 import {Observable} from 'rxjs/Observable';
 import {merge} from 'rxjs/observable/merge';
 import {Subject} from 'rxjs/Subject';
-import {Subscription} from 'rxjs/Subscription';
 import {fadeInContent, transformPanel} from './select-animations';
 import {
   getMatSelectDynamicMultipleError,
@@ -193,15 +192,6 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   /** Whether or not the overlay panel is open. */
   private _panelOpen = false;
 
-  /** Subscriptions to option events. */
-  private _optionSubscription = Subscription.EMPTY;
-
-  /** Subscription to changes in the option list. */
-  private _changeSubscription = Subscription.EMPTY;
-
-  /** Subscription to tab events while overlay is focused. */
-  private _tabSubscription = Subscription.EMPTY;
-
   /** Whether filling out the select is required in the form.  */
   private _required: boolean = false;
 
@@ -220,6 +210,9 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   /** Unique id for this input. */
   private _uid = `mat-select-${nextUniqueId++}`;
 
+  /** Emits whenever the component is destroyed. */
+  private _destroy = new Subject<void>();
+
   /** The last measured value for the trigger's client bounding rect. */
   _triggerRect: ClientRect;
 
@@ -453,10 +446,13 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   ngAfterContentInit() {
     this._initKeyManager();
 
-    this._changeSubscription = startWith.call(this.options.changes, null).subscribe(() => {
-      this._resetOptions();
-      this._initializeSelection();
-    });
+    RxChain.from(this.options.changes)
+      .call(startWith, null)
+      .call(takeUntil, this._destroy)
+      .subscribe(() => {
+        this._resetOptions();
+        this._initializeSelection();
+      });
   }
 
   ngDoCheck() {
@@ -466,9 +462,8 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   }
 
   ngOnDestroy() {
-    this._dropSubscriptions();
-    this._changeSubscription.unsubscribe();
-    this._tabSubscription.unsubscribe();
+    this._destroy.next();
+    this._destroy.complete();
   }
 
   /** Toggles the overlay panel open or closed. */
@@ -493,7 +488,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
     this._changeDetectorRef.markForCheck();
 
     // Set the font size on the panel element once it exists.
-    first.call(this._ngZone.onStable).subscribe(() => {
+    first.call(this._ngZone.onStable.asObservable()).subscribe(() => {
       if (this._triggerFontSize && this.overlayDir.overlayRef &&
           this.overlayDir.overlayRef.overlayElement) {
         this.overlayDir.overlayRef.overlayElement.style.fontSize = `${this._triggerFontSize}px`;
@@ -621,13 +616,6 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
       this._keyManager.activeItem._selectViaInteraction();
     } else {
       this._keyManager.onKeydown(event);
-
-      // TODO(crisbeto): get rid of the Promise.resolve when #6441 gets in.
-      Promise.resolve().then(() => {
-        if (this.panelOpen) {
-          this._scrollActiveOptionIntoView();
-        }
-      });
     }
   }
 
@@ -781,28 +769,32 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   /** Sets up a key manager to listen to keyboard events on the overlay panel. */
   private _initKeyManager() {
     this._keyManager = new ActiveDescendantKeyManager<MatOption>(this.options).withTypeAhead();
-    this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.close());
+
+    takeUntil.call(this._keyManager.tabOut, this._destroy)
+      .subscribe(() => this.close());
+
+    RxChain.from(this._keyManager.change)
+      .call(takeUntil, this._destroy)
+      .call(filter, () => this._panelOpen && !!this.panel)
+      .subscribe(() => this._scrollActiveOptionIntoView());
   }
 
   /** Drops current option subscriptions and IDs and resets from scratch. */
   private _resetOptions(): void {
-    this._dropSubscriptions();
-    this._listenToOptions();
-    this._setOptionIds();
-    this._setOptionMultiple();
-    this._setOptionDisableRipple();
-  }
-
-  /** Listens to user-generated selection events on each option. */
-  private _listenToOptions(): void {
-    this._optionSubscription = filter.call(this.optionSelectionChanges,
-      event => event.isUserInput).subscribe(event => {
+    RxChain.from(this.optionSelectionChanges)
+      .call(takeUntil, merge(this._destroy, this.options.changes))
+      .call(filter, event => event.isUserInput)
+      .subscribe(event => {
         this._onSelect(event.source);
 
         if (!this.multiple) {
           this.close();
         }
       });
+
+    this._setOptionIds();
+    this._setOptionMultiple();
+    this._setOptionDisableRipple();
   }
 
   /** Invoked when an option is clicked. */
@@ -848,11 +840,6 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
     }
   }
 
-  /** Unsubscribes from all option subscriptions. */
-  private _dropSubscriptions(): void {
-    this._optionSubscription.unsubscribe();
-  }
-
   /** Emits change event to set the model value. */
   private _propagateChanges(fallbackValue?: any): void {
     let valueToEmit: any = null;