From 81537af7bad6fa37a4d21b709ea9eb90ece494fb Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 12 Jun 2018 18:56:22 +0200 Subject: [PATCH] fix(select): unable to use the MatOption select/deselect API to toggle options (#11528) 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. --- src/cdk/collections/selection.spec.ts | 8 ++ src/cdk/collections/selection.ts | 6 +- src/lib/core/option/option.spec.ts | 44 +++++++++++ src/lib/core/option/option.ts | 16 ++-- src/lib/select/select.spec.ts | 19 ++++- src/lib/select/select.ts | 110 ++++++++++---------------- 6 files changed, 126 insertions(+), 77 deletions(-) diff --git a/src/cdk/collections/selection.spec.ts b/src/cdk/collections/selection.spec.ts index 40ee3eb2287..67b4e9806e6 100644 --- a/src/cdk/collections/selection.spec.ts +++ b/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', () => { diff --git a/src/cdk/collections/selection.ts b/src/cdk/collections/selection.ts index 4cb2ab9b90d..d159b2b11b9 100644 --- a/src/cdk/collections/selection.ts +++ b/src/cdk/collections/selection.ts @@ -13,7 +13,7 @@ import {Subject} from 'rxjs'; */ export class SelectionModel { /** Currently-selected values. */ - private _selection: Set = new Set(); + private _selection = new Set(); /** 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 { * 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); } } diff --git a/src/lib/core/option/option.spec.ts b/src/lib/core/option/option.spec.ts index 9eceeb530e2..d1b4949d3b0 100644 --- a/src/lib/core/option/option.spec.ts +++ b/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; let optionDebugElement: DebugElement; diff --git a/src/lib/core/option/option.ts b/src/lib/core/option/option.ts index 75c468d49b2..3b0339c1804 100644 --- a/src/lib/core/option/option.ts +++ b/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. */ diff --git a/src/lib/select/select.spec.ts b/src/lib/select/select.spec.ts index 0238805cc9d..72f3debc0cd 100644 --- a/src/lib/select/select.spec.ts +++ b/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 = + 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(); diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index dee2892c7d6..92db23b2a12 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -479,7 +479,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, } ngOnInit() { - this._selectionModel = new SelectionModel(this.multiple, undefined, false); + this._selectionModel = new SelectionModel(this.multiple); this.stateChanges.next(); // We need `distinctUntilChanged` here, because some browsers will @@ -503,6 +503,11 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit, 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(); @@ -765,19 +770,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. @@ -793,7 +797,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. @@ -808,29 +812,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(this.options) @@ -858,16 +845,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. @@ -882,51 +867,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); + option.selected ? this._selectionModel.select(option) : this._selectionModel.deselect(option); - if (option.value == null) { - this._propagateChanges(option.value); - } else { - this._selectionModel.select(option); - this.stateChanges.next(); + // 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(); } + + this.stateChanges.next(); } - /** - * 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.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(); } }