Skip to content

fix(material/list): emit selectionChange event when selecting with ctrl + a #20667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 7, 2020
Merged

fix(material/list): emit selectionChange event when selecting with ctrl + a #20667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/material-experimental/mdc-list/selection-list.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -1433,7 +1450,7 @@ describe('MDC-based MatSelectionList with forms', () => {
@Component({template: `
<mat-selection-list
id="selection-list-1"
(selectionChange)="onValueChange($event)"
(selectionChange)="onSelectionChange($event)"
[disableRipple]="listRippleDisabled"
[color]="selectionListColor"
[multiple]="multiple">
Expand Down Expand Up @@ -1462,7 +1479,7 @@ class SelectionListWithListOptions {
selectionListColor: ThemePalette;
firstOptionColor: ThemePalette;

onValueChange(_change: MatSelectionListChange) {}
onSelectionChange(_change: MatSelectionListChange) {}
}

@Component({template: `
Expand Down
20 changes: 13 additions & 7 deletions src/material-experimental/mdc-list/selection-list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -209,8 +215,8 @@ export class MatSelectionList extends MatInteractiveListBase<MatListOption>
}

/** 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. */
Expand Down Expand Up @@ -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]]);
},
};
}
30 changes: 22 additions & 8 deletions src/material/list/selection-list.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1520,7 +1534,7 @@ describe('MatSelectionList with forms', () => {
@Component({template: `
<mat-selection-list
id="selection-list-1"
(selectionChange)="onValueChange($event)"
(selectionChange)="onSelectionChange($event)"
[disableRipple]="listRippleDisabled"
[color]="selectionListColor"
[multiple]="multiple">
Expand Down Expand Up @@ -1549,7 +1563,7 @@ class SelectionListWithListOptions {
selectionListColor: ThemePalette;
firstOptionColor: ThemePalette;

onValueChange(_change: MatSelectionListChange) {}
onSelectionChange(_change: MatSelectionListChange) {}
}

@Component({template: `
Expand Down
35 changes: 24 additions & 11 deletions src/material/list/selection-list.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[]) {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to avoid this future breaking change, we could also emit one event per selected option like we do in mat-select. I decided not to do it, because the mat-select behavior feels broken.

}

/**
Expand Down Expand Up @@ -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]);
}
}

Expand Down Expand Up @@ -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);
Expand All @@ -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. */
Expand Down Expand Up @@ -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]);
}
}
}
Expand All @@ -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);
}
}
}

Expand Down
6 changes: 4 additions & 2 deletions tools/public_api_guard/material/list.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export declare class MatSelectionList extends _MatSelectionListMixinBase impleme
readonly selectionChange: EventEmitter<MatSelectionListChange>;
tabIndex: number;
constructor(_element: ElementRef<HTMLElement>, 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;
Expand All @@ -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[]);
}