fix(select): unable to use the MatOption select/deselect API to toggle options

Reworks the way `MatSelect` handles syncing its selected state between its child options and the `SelectionModel`, making it easier to follow and generally more robust. Previously we kept syncing the selected state in parallel between the options themselves and the selection model, which meant that we had to prevent infinite loops by ignoring any non-user changes to the options' `selected` state. This made the `MatOption.select` and `MatOption.deselect` methods useless to consumers and was very error prone. The new approach makes the `select` and `deselect` methods usable and allows us to turn the `MatOption.selected` property into an input, getting it closer to the native `option` API.

These changes also address the following issues that I bumped into along the way:
* The `MatOption.onSelectionChange` was emitting even if the selection hadn't changed.
* The `SelectionModel.sort` wasn't sorting its values if the consumer hadn't attempted to access the `selected` value since the last time it changed.

Fixes #9314.

Author: crisbeto

src/cdk/collections/selection.spec.ts

@@ -82,6 +82,14 @@ describe('SelectionModel', () => {
 
       expect(model.selected).toEqual([1, 2, 3]);
     });
+
+    it('should sort values if `selected` has not been accessed before', () => {
+      model = new SelectionModel(true, [2, 3, 1]);
+
+      // Important: don't assert `selected` before sorting so the getter isn't invoked
+      model.sort();
+      expect(model.selected).toEqual([1, 2, 3]);
+    });
   });
 
   describe('onChange event', () => {

src/cdk/collections/selection.ts

@@ -13,7 +13,7 @@ import {Subject} from 'rxjs';
  */
 export class SelectionModel<T> {
   /** Currently-selected values. */
-  private _selection: Set<T> = new Set();
+  private _selection = new Set<T>();
 
   /** Keeps track of the deselected options that haven't been emitted by the change event. */
   private _deselectedToEmit: T[] = [];
@@ -111,8 +111,8 @@ export class SelectionModel<T> {
    * Sorts the selected values based on a predicate function.
    */
   sort(predicate?: (a: T, b: T) => number): void {
-    if (this._multiple && this._selected) {
-      this._selected.sort(predicate);
+    if (this._multiple && this.selected) {
+      this._selected!.sort(predicate);
     }
   }
 

src/lib/core/option/option.spec.ts

@@ -27,6 +27,50 @@ describe('MatOption component', () => {
     subscription.unsubscribe();
   });
 
+  it('should not emit to `onSelectionChange` if selecting an already-selected option', () => {
+    const fixture = TestBed.createComponent(OptionWithDisable);
+    fixture.detectChanges();
+
+    const optionInstance: MatOption =
+        fixture.debugElement.query(By.directive(MatOption)).componentInstance;
+
+    optionInstance.select();
+    expect(optionInstance.selected).toBe(true);
+
+    const spy = jasmine.createSpy('selection change spy');
+    const subscription = optionInstance.onSelectionChange.subscribe(spy);
+
+    optionInstance.select();
+    fixture.detectChanges();
+
+    expect(optionInstance.selected).toBe(true);
+    expect(spy).not.toHaveBeenCalled();
+
+    subscription.unsubscribe();
+  });
+
+  it('should not emit to `onSelectionChange` if deselecting an unselected option', () => {
+    const fixture = TestBed.createComponent(OptionWithDisable);
+    fixture.detectChanges();
+
+    const optionInstance: MatOption =
+        fixture.debugElement.query(By.directive(MatOption)).componentInstance;
+
+    optionInstance.deselect();
+    expect(optionInstance.selected).toBe(false);
+
+    const spy = jasmine.createSpy('selection change spy');
+    const subscription = optionInstance.onSelectionChange.subscribe(spy);
+
+    optionInstance.deselect();
+    fixture.detectChanges();
+
+    expect(optionInstance.selected).toBe(false);
+    expect(spy).not.toHaveBeenCalled();
+
+    subscription.unsubscribe();
+  });
+
   describe('ripples', () => {
     let fixture: ComponentFixture<OptionWithDisable>;
     let optionDebugElement: DebugElement;

src/lib/core/option/option.ts

@@ -145,16 +145,20 @@ export class MatOption implements AfterViewChecked, OnDestroy {
 
   /** Selects the option. */
   select(): void {
-    this._selected = true;
-    this._changeDetectorRef.markForCheck();
-    this._emitSelectionChangeEvent();
+    if (!this._selected) {
+      this._selected = true;
+      this._changeDetectorRef.markForCheck();
+      this._emitSelectionChangeEvent();
+    }
   }
 
   /** Deselects the option. */
   deselect(): void {
-    this._selected = false;
-    this._changeDetectorRef.markForCheck();
-    this._emitSelectionChangeEvent();
+    if (this._selected) {
+      this._selected = false;
+      this._changeDetectorRef.markForCheck();
+      this._emitSelectionChangeEvent();
+    }
   }
 
   /** Sets focus onto this option. */

src/lib/select/select.spec.ts

@@ -86,7 +86,7 @@ describe('MatSelect', () => {
    * overall test time.
    * @param declarations Components to declare for this block
    */
-  function configureMatSelectTestingModule(declarations) {
+  function configureMatSelectTestingModule(declarations: any[]) {
     TestBed.configureTestingModule({
       imports: [
         MatFormFieldModule,
@@ -1096,6 +1096,23 @@ describe('MatSelect', () => {
             .toBe(fixture.componentInstance.options.first);
       }));
 
+      it('should be able to select an option using the MatOption API', fakeAsync(() => {
+        trigger.click();
+        fixture.detectChanges();
+        flush();
+
+        const optionInstances = fixture.componentInstance.options.toArray();
+        const optionNodes: NodeListOf<HTMLElement> =
+            overlayContainerElement.querySelectorAll('mat-option');
+
+        optionInstances[1].select();
+        fixture.detectChanges();
+
+        expect(optionNodes[1].classList).toContain('mat-selected');
+        expect(optionInstances[1].selected).toBe(true);
+        expect(fixture.componentInstance.select.selected).toBe(optionInstances[1]);
+      }));
+
       it('should deselect other options when one is selected', fakeAsync(() => {
         trigger.click();
         fixture.detectChanges();

src/lib/select/select.ts

@@ -468,13 +468,18 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   }
 
   ngOnInit() {
-    this._selectionModel = new SelectionModel<MatOption>(this.multiple, undefined, false);
+    this._selectionModel = new SelectionModel<MatOption>(this.multiple);
     this.stateChanges.next();
   }
 
   ngAfterContentInit() {
     this._initKeyManager();
 
+    this._selectionModel.onChange!.pipe(takeUntil(this._destroy)).subscribe(event => {
+      event.added.forEach(option => option.select());
+      event.removed.forEach(option => option.deselect());
+    });
+
     this.options.changes.pipe(startWith(null), takeUntil(this._destroy)).subscribe(() => {
       this._resetOptions();
       this._initializeSelection();
@@ -753,19 +758,18 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    * Sets the selected option based on a value. If no option can be
    * found with the designated value, the select trigger is cleared.
    */
-  private _setSelectionByValue(value: any | any[], isUserInput = false): void {
+  private _setSelectionByValue(value: any | any[]): void {
     if (this.multiple && value) {
       if (!Array.isArray(value)) {
         throw getMatSelectNonArrayValueError();
       }
 
-      this._clearSelection();
-      value.forEach((currentValue: any) => this._selectValue(currentValue, isUserInput));
+      this._selectionModel.clear();
+      value.forEach((currentValue: any) => this._selectValue(currentValue));
       this._sortValues();
     } else {
-      this._clearSelection();
-
-      const correspondingOption = this._selectValue(value, isUserInput);
+      this._selectionModel.clear();
+      const correspondingOption = this._selectValue(value);
 
       // Shift focus to the active item. Note that we shouldn't do this in multiple
       // mode, because we don't know what option the user interacted with last.
@@ -781,7 +785,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
    * Finds and selects and option based on its value.
    * @returns Option that has the corresponding value.
    */
-  private _selectValue(value: any, isUserInput = false): MatOption | undefined {
+  private _selectValue(value: any): MatOption | undefined {
     const correspondingOption = this.options.find((option: MatOption) => {
       try {
         // Treat null as a special reset value.
@@ -796,29 +800,12 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
     });
 
     if (correspondingOption) {
-      isUserInput ? correspondingOption._selectViaInteraction() : correspondingOption.select();
       this._selectionModel.select(correspondingOption);
-      this.stateChanges.next();
     }
 
     return correspondingOption;
   }
 
-
-  /**
-   * Clears the select trigger and deselects every option in the list.
-   * @param skip Option that should not be deselected.
-   */
-  private _clearSelection(skip?: MatOption): void {
-    this._selectionModel.clear();
-    this.options.forEach(option => {
-      if (option !== skip) {
-        option.deselect();
-      }
-    });
-    this.stateChanges.next();
-  }
-
   /** Sets up a key manager to listen to keyboard events on the overlay panel. */
   private _initKeyManager() {
     this._keyManager = new ActiveDescendantKeyManager<MatOption>(this.options)
@@ -846,16 +833,14 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   private _resetOptions(): void {
     const changedOrDestroyed = merge(this.options.changes, this._destroy);
 
-    this.optionSelectionChanges
-      .pipe(takeUntil(changedOrDestroyed), filter(event => event.isUserInput))
-      .subscribe(event => {
-        this._onSelect(event.source);
+    this.optionSelectionChanges.pipe(takeUntil(changedOrDestroyed)).subscribe(event => {
+      this._onSelect(event.source, event.isUserInput);
 
-        if (!this.multiple && this._panelOpen) {
-          this.close();
-          this.focus();
-        }
-      });
+      if (event.isUserInput && !this.multiple && this._panelOpen) {
+        this.close();
+        this.focus();
+      }
+    });
 
     // Listen to changes in the internal state of the options and react accordingly.
     // Handles cases like the labels of the selected options changing.
@@ -870,51 +855,42 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
   }
 
   /** Invoked when an option is clicked. */
-  private _onSelect(option: MatOption): void {
+  private _onSelect(option: MatOption, isUserInput: boolean): void {
     const wasSelected = this._selectionModel.isSelected(option);
 
-    // TODO(crisbeto): handle blank/null options inside multi-select.
-    if (this.multiple) {
-      this._selectionModel.toggle(option);
-      this.stateChanges.next();
-      wasSelected ? option.deselect() : option.select();
-      this._keyManager.setActiveItem(option);
-      this._sortValues();
-
-      // In case the user select the option with their mouse, we
-      // want to restore focus back to the trigger, in order to
-      // prevent the select keyboard controls from clashing with
-      // the ones from `mat-option`.
-      this.focus();
+    if (option.value == null) {
+      this._selectionModel.clear();
+      this._propagateChanges(option.value);
     } else {
-      this._clearSelection(option.value == null ? undefined : option);
-
-      if (option.value == null) {
-        this._propagateChanges(option.value);
-      } else {
-        this._selectionModel.select(option);
-        this.stateChanges.next();
+      option.selected ? this._selectionModel.select(option) : this._selectionModel.deselect(option);
+
+      // TODO(crisbeto): handle blank/null options inside multi-select.
+      if (this.multiple) {
+        this._sortValues();
+
+        if (isUserInput) {
+          this._keyManager.setActiveItem(option);
+          // In case the user selected the option with their mouse, we
+          // want to restore focus back to the trigger, in order to
+          // prevent the select keyboard controls from clashing with
+          // the ones from `mat-option`.
+          this.focus();
+        }
       }
     }
 
     if (wasSelected !== this._selectionModel.isSelected(option)) {
       this._propagateChanges();
     }
-  }
 
-  /**
-   * Sorts the model values, ensuring that they keep the same
-   * order that they have in the panel.
-   */
-  private _sortValues(): void {
-    if (this._multiple) {
-      this._selectionModel.clear();
+    this.stateChanges.next();
+  }
 
-      this.options.forEach(option => {
-        if (option.selected) {
-          this._selectionModel.select(option);
-        }
-      });
+  /** Sorts the selected values in the selected based on their order in the panel. */
+  private _sortValues() {
+    if (this.multiple) {
+      const options = this.options.toArray();
+      this._selectionModel.sort((a, b) => options.indexOf(a) - options.indexOf(b));
       this.stateChanges.next();
     }
   }