From 7df08e5c813de73af9304ab7f0093c6e2508a1dc Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 28 Sep 2020 15:24:36 +0300 Subject: [PATCH] fix(material/list): emit selectionChange event when selecting with ctrl + a Fixes that we weren't emitting the `selectionChange` event when the user toggles all options with the ctrl + a shortcut. Also replaces `MatSelectionListChange.option` with `options` which is an array of all options that were changed by the current event. Fixes #15696. --- .../mdc-list/selection-list.spec.ts | 33 ++++++++++++----- .../mdc-list/selection-list.ts | 20 +++++++---- src/material/list/selection-list.spec.ts | 30 +++++++++++----- src/material/list/selection-list.ts | 35 +++++++++++++------ tools/public_api_guard/material/list.d.ts | 6 ++-- 5 files changed, 88 insertions(+), 36 deletions(-) diff --git a/src/material-experimental/mdc-list/selection-list.spec.ts b/src/material-experimental/mdc-list/selection-list.spec.ts index ce4724a2fbde..1eab8a4b22e0 100644 --- a/src/material-experimental/mdc-list/selection-list.spec.ts +++ b/src/material-experimental/mdc-list/selection-list.spec.ts @@ -71,25 +71,25 @@ describe('MDC-based MatSelectionList without forms', () => { }); it('should not emit a selectionChange event if an option changed programmatically', () => { - spyOn(fixture.componentInstance, 'onValueChange'); + spyOn(fixture.componentInstance, 'onSelectionChange'); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); listOptions[2].componentInstance.toggle(); fixture.detectChanges(); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); }); it('should emit a selectionChange event if an option got clicked', () => { - spyOn(fixture.componentInstance, 'onValueChange'); + spyOn(fixture.componentInstance, 'onSelectionChange'); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); dispatchMouseEvent(listOptions[2].nativeElement, 'click'); fixture.detectChanges(); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(1); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(1); }); it('should be able to dispatch one selected item', () => { @@ -525,6 +525,23 @@ describe('MDC-based MatSelectionList without forms', () => { expect(listOptions.every(option => option.componentInstance.selected)).toBe(false); }); + // MDC does not support the common CTRL + A keyboard shortcut. + // Tracked with: https://github.com/material-components/material-components-web/issues/6366 + // tslint:disable-next-line:ban + xit('should dispatch the selectionChange event when selecting via ctrl + a', () => { + const spy = spyOn(fixture.componentInstance, 'onSelectionChange'); + listOptions.forEach(option => option.componentInstance.disabled = false); + const event = createKeyboardEvent('keydown', A, undefined, {control: true}); + + dispatchEvent(selectionList.nativeElement, event); + fixture.detectChanges(); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(jasmine.objectContaining({ + options: listOptions.map(option => option.componentInstance) + })); + }); + it('should be able to jump focus down to an item by typing', fakeAsync(() => { const firstOption = listOptions[0].nativeElement; @@ -1433,7 +1450,7 @@ describe('MDC-based MatSelectionList with forms', () => { @Component({template: ` @@ -1462,7 +1479,7 @@ class SelectionListWithListOptions { selectionListColor: ThemePalette; firstOptionColor: ThemePalette; - onValueChange(_change: MatSelectionListChange) {} + onSelectionChange(_change: MatSelectionListChange) {} } @Component({template: ` diff --git a/src/material-experimental/mdc-list/selection-list.ts b/src/material-experimental/mdc-list/selection-list.ts index a9ce37d91e28..3a18c2ef2580 100644 --- a/src/material-experimental/mdc-list/selection-list.ts +++ b/src/material-experimental/mdc-list/selection-list.ts @@ -44,10 +44,16 @@ const MAT_SELECTION_LIST_VALUE_ACCESSOR: any = { /** Change event that is being fired whenever the selected state of an option changes. */ export class MatSelectionListChange { constructor( - /** Reference to the selection list that emitted the event. */ - public source: MatSelectionList, - /** Reference to the option that has been changed. */ - public option: MatListOption) {} + /** Reference to the selection list that emitted the event. */ + public source: MatSelectionList, + /** + * Reference to the option that has been changed. + * @deprecated Use `options` instead, because some events may change more than one option. + * @breaking-change 12.0.0 + */ + public option: MatListOption, + /** Reference to the options that have been changed. */ + public options: MatListOption[]) {} } @Component({ @@ -209,8 +215,8 @@ export class MatSelectionList extends MatInteractiveListBase } /** Emits a change event if the selected state of an option changed. */ - _emitChangeEvent(option: MatListOption) { - this.selectionChange.emit(new MatSelectionListChange(this, option)); + _emitChangeEvent(options: MatListOption[]) { + this.selectionChange.emit(new MatSelectionListChange(this, options[0], options)); } /** Implemented as part of ControlValueAccessor. */ @@ -366,7 +372,7 @@ function getSelectionListAdapter(list: MatSelectionList): MDCListAdapter { baseAdapter.setAttributeForElementIndex(index, attribute, value); }, notifyAction(index: number): void { - list._emitChangeEvent(list._itemsArr[index]); + list._emitChangeEvent([list._itemsArr[index]]); }, }; } diff --git a/src/material/list/selection-list.spec.ts b/src/material/list/selection-list.spec.ts index d9988944267b..270368ba7c71 100644 --- a/src/material/list/selection-list.spec.ts +++ b/src/material/list/selection-list.spec.ts @@ -74,25 +74,25 @@ describe('MatSelectionList without forms', () => { }); it('should not emit a selectionChange event if an option changed programmatically', () => { - spyOn(fixture.componentInstance, 'onValueChange'); + spyOn(fixture.componentInstance, 'onSelectionChange'); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); listOptions[2].componentInstance.toggle(); fixture.detectChanges(); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); }); it('should emit a selectionChange event if an option got clicked', () => { - spyOn(fixture.componentInstance, 'onValueChange'); + spyOn(fixture.componentInstance, 'onSelectionChange'); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(0); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(0); dispatchFakeEvent(listOptions[2].nativeElement, 'click'); fixture.detectChanges(); - expect(fixture.componentInstance.onValueChange).toHaveBeenCalledTimes(1); + expect(fixture.componentInstance.onSelectionChange).toHaveBeenCalledTimes(1); }); it('should be able to dispatch one selected item', () => { @@ -566,6 +566,20 @@ describe('MatSelectionList without forms', () => { expect(listOptions.every(option => option.componentInstance.selected)).toBe(false); }); + it('should dispatch the selectionChange event when selecting via ctrl + a', () => { + const spy = spyOn(fixture.componentInstance, 'onSelectionChange'); + listOptions.forEach(option => option.componentInstance.disabled = false); + const event = createKeyboardEvent('keydown', A, undefined, {control: true}); + + dispatchEvent(selectionList.nativeElement, event); + fixture.detectChanges(); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(jasmine.objectContaining({ + options: listOptions.map(option => option.componentInstance) + })); + }); + it('should be able to jump focus down to an item by typing', fakeAsync(() => { const listEl = selectionList.nativeElement; const manager = selectionList.componentInstance._keyManager; @@ -1520,7 +1534,7 @@ describe('MatSelectionList with forms', () => { @Component({template: ` @@ -1549,7 +1563,7 @@ class SelectionListWithListOptions { selectionListColor: ThemePalette; firstOptionColor: ThemePalette; - onValueChange(_change: MatSelectionListChange) {} + onSelectionChange(_change: MatSelectionListChange) {} } @Component({template: ` diff --git a/src/material/list/selection-list.ts b/src/material/list/selection-list.ts index 17341a6a3dbd..4cbef592cf8b 100644 --- a/src/material/list/selection-list.ts +++ b/src/material/list/selection-list.ts @@ -72,8 +72,14 @@ export class MatSelectionListChange { constructor( /** Reference to the selection list that emitted the event. */ public source: MatSelectionList, - /** Reference to the option that has been changed. */ - public option: MatListOption) {} + /** + * Reference to the option that has been changed. + * @deprecated Use `options` instead, because some events may change more than one option. + * @breaking-change 12.0.0 + */ + public option: MatListOption, + /** Reference to the options that have been changed. */ + public options: MatListOption[]) {} } /** @@ -258,7 +264,7 @@ export class MatListOption extends _MatListOptionMixinBase implements AfterConte this.toggle(); // Emit a change event if the selected state of the option changed through user interaction. - this.selectionList._emitChangeEvent(this); + this.selectionList._emitChangeEvent([this]); } } @@ -560,7 +566,7 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD if (keyCode === A && this.multiple && hasModifierKey(event, 'ctrlKey') && !manager.isTyping()) { const shouldSelect = this.options.some(option => !option.disabled && !option.selected); - this._setAllOptionsSelected(shouldSelect, true); + this._setAllOptionsSelected(shouldSelect, true, true); event.preventDefault(); } else { manager.onKeydown(event); @@ -586,8 +592,8 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD } /** Emits a change event if the selected state of an option changed. */ - _emitChangeEvent(option: MatListOption) { - this.selectionChange.emit(new MatSelectionListChange(this, option)); + _emitChangeEvent(options: MatListOption[]) { + this.selectionChange.emit(new MatSelectionListChange(this, options[0], options)); } /** Implemented as part of ControlValueAccessor. */ @@ -648,7 +654,7 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD // Emit a change event because the focused option changed its state through user // interaction. - this._emitChangeEvent(focusedOption); + this._emitChangeEvent([focusedOption]); } } } @@ -657,19 +663,26 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements CanD * Sets the selected state on all of the options * and emits an event if anything changed. */ - private _setAllOptionsSelected(isSelected: boolean, skipDisabled?: boolean) { + private _setAllOptionsSelected( + isSelected: boolean, + skipDisabled?: boolean, + isUserInput?: boolean) { // Keep track of whether anything changed, because we only want to // emit the changed event when something actually changed. - let hasChanged = false; + const changedOptions: MatListOption[] = []; this.options.forEach(option => { if ((!skipDisabled || !option.disabled) && option._setSelected(isSelected)) { - hasChanged = true; + changedOptions.push(option); } }); - if (hasChanged) { + if (changedOptions.length) { this._reportValueChange(); + + if (isUserInput) { + this._emitChangeEvent(changedOptions); + } } } diff --git a/tools/public_api_guard/material/list.d.ts b/tools/public_api_guard/material/list.d.ts index cac2e9d90cd4..0254d85efd36 100644 --- a/tools/public_api_guard/material/list.d.ts +++ b/tools/public_api_guard/material/list.d.ts @@ -116,7 +116,7 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme readonly selectionChange: EventEmitter; tabIndex: number; constructor(_element: ElementRef, tabIndex: string, _changeDetector: ChangeDetectorRef, _focusMonitor?: FocusMonitor | undefined); - _emitChangeEvent(option: MatListOption): void; + _emitChangeEvent(options: MatListOption[]): void; _keydown(event: KeyboardEvent): void; _removeOptionFromList(option: MatListOption): MatListOption | null; _reportValueChange(): void; @@ -140,8 +140,10 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme export declare class MatSelectionListChange { option: MatListOption; + options: MatListOption[]; source: MatSelectionList; constructor( source: MatSelectionList, - option: MatListOption); + option: MatListOption, + options: MatListOption[]); }