Skip to content

Commit

Permalink
fix(material/chips): simplify repeat chip removal prevention
Browse files Browse the repository at this point in the history
We have some logic that prevents chip removal if the users is holding down the backspace key. It currently breaks down in the case where a value is pasted into the input via click.

These changes simplify our detection by using `KeyboardEvent.repeat` which also resolves the paste issue.
  • Loading branch information
crisbeto committed May 15, 2024
1 parent 33b6fc1 commit 494746b
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 66 deletions.
33 changes: 11 additions & 22 deletions src/material/chips/chip-grid.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
TAB,
} from '@angular/cdk/keycodes';
import {
createKeyboardEvent,
dispatchEvent,
dispatchFakeEvent,
dispatchKeyboardEvent,
patchElementFocus,
Expand Down Expand Up @@ -789,28 +791,15 @@ describe('MDC-based MatChipGrid', () => {
expectLastCellFocused();
});

it(
'should not focus the last chip when pressing BACKSPACE after changing input, ' +
'until BACKSPACE is released and pressed again',
() => {
// Change the input
dispatchKeyboardEvent(nativeInput, 'keydown', A);

// It shouldn't focus until backspace is released and pressed again
dispatchKeyboardEvent(nativeInput, 'keydown', BACKSPACE);
dispatchKeyboardEvent(nativeInput, 'keydown', BACKSPACE);
dispatchKeyboardEvent(nativeInput, 'keydown', BACKSPACE);
expectNoCellFocused();

// Still not focused
dispatchKeyboardEvent(nativeInput, 'keyup', BACKSPACE);
expectNoCellFocused();

// Only now should it focus the last element
dispatchKeyboardEvent(nativeInput, 'keydown', BACKSPACE);
expectLastCellFocused();
},
);
it('should not focus the last chip when the BACKSPACE key is being repeated', () => {
// Only now should it focus the last element
const event = createKeyboardEvent('keydown', BACKSPACE);
Object.defineProperty(event, 'repeat', {
get: () => true,
});
dispatchEvent(nativeInput, event);
expectNoCellFocused();
});

it('should focus last chip after pressing BACKSPACE after creating a chip', () => {
// Create a chip
Expand Down
42 changes: 8 additions & 34 deletions src/material/chips/chip-input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import {BACKSPACE, hasModifierKey} from '@angular/cdk/keycodes';
import {
AfterContentInit,
Directive,
ElementRef,
EventEmitter,
Expand Down Expand Up @@ -57,7 +56,6 @@ let nextUniqueId = 0;
// the MDC chips were landed initially with it.
'class': 'mat-mdc-chip-input mat-mdc-input-element mdc-text-field__input mat-input-element',
'(keydown)': '_keydown($event)',
'(keyup)': '_keyup($event)',
'(blur)': '_blur()',
'(focus)': '_focus()',
'(input)': '_onInput()',
Expand All @@ -70,10 +68,7 @@ let nextUniqueId = 0;
},
standalone: true,
})
export class MatChipInput implements MatChipTextControl, AfterContentInit, OnChanges, OnDestroy {
/** Used to prevent focus moving to chips while user is holding backspace */
private _focusLastChipOnBackspace: boolean;

export class MatChipInput implements MatChipTextControl, OnChanges, OnDestroy {
/** Whether the control is focused. */
focused: boolean = false;

Expand Down Expand Up @@ -153,36 +148,17 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
this.chipEnd.complete();
}

ngAfterContentInit(): void {
this._focusLastChipOnBackspace = this.empty;
}

/** Utility method to make host definition/tests more clear. */
_keydown(event?: KeyboardEvent) {
if (event) {
// To prevent the user from accidentally deleting chips when pressing BACKSPACE continuously,
// We focus the last chip on backspace only after the user has released the backspace button,
// And the input is empty (see behaviour in _keyup)
if (event.keyCode === BACKSPACE && this._focusLastChipOnBackspace) {
_keydown(event: KeyboardEvent) {
if (this.empty && event.keyCode === BACKSPACE) {
// Ignore events where the user is holding down backspace
// so that we don't accidentally remove too many chips.
if (!event.repeat) {
this._chipGrid._focusLastChip();
event.preventDefault();
return;
} else {
this._focusLastChipOnBackspace = false;
}
}

this._emitChipEnd(event);
}

/**
* Pass events to the keyboard manager. Available here for tests.
*/
_keyup(event: KeyboardEvent) {
// Allow user to move focus to chips next time he presses backspace
if (!this._focusLastChipOnBackspace && event.keyCode === BACKSPACE && this.empty) {
this._focusLastChipOnBackspace = true;
event.preventDefault();
} else {
this._emitChipEnd(event);
}
}

Expand All @@ -201,7 +177,6 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha

_focus() {
this.focused = true;
this._focusLastChipOnBackspace = this.empty;
this._chipGrid.stateChanges.next();
}

Expand Down Expand Up @@ -231,7 +206,6 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
/** Clears the input */
clear(): void {
this.inputElement.value = '';
this._focusLastChipOnBackspace = true;
}

setDescribedByIds(ids: string[]): void {
Expand Down
22 changes: 18 additions & 4 deletions src/material/chips/chip-row.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ describe('MDC-based Row Chips', () => {

spyOn(testComponent, 'chipRemove');

chipInstance._handleKeydown(DELETE_EVENT);
dispatchEvent(chipNativeElement, DELETE_EVENT);
fixture.detectChanges();

expect(testComponent.chipRemove).toHaveBeenCalled();
Expand All @@ -134,11 +134,25 @@ describe('MDC-based Row Chips', () => {

spyOn(testComponent, 'chipRemove');

chipInstance._handleKeydown(BACKSPACE_EVENT);
dispatchEvent(chipNativeElement, BACKSPACE_EVENT);
fixture.detectChanges();

expect(testComponent.chipRemove).toHaveBeenCalled();
});

it('should not remove for repeated BACKSPACE event', () => {
const BACKSPACE_EVENT = createKeyboardEvent('keydown', BACKSPACE);
Object.defineProperty(BACKSPACE_EVENT, 'repeat', {
get: () => true,
});

spyOn(testComponent, 'chipRemove');

dispatchEvent(chipNativeElement, BACKSPACE_EVENT);
fixture.detectChanges();

expect(testComponent.chipRemove).not.toHaveBeenCalled();
});
});

describe('when removable is false', () => {
Expand All @@ -152,7 +166,7 @@ describe('MDC-based Row Chips', () => {

spyOn(testComponent, 'chipRemove');

chipInstance._handleKeydown(DELETE_EVENT);
dispatchEvent(chipNativeElement, DELETE_EVENT);
fixture.detectChanges();

expect(testComponent.chipRemove).not.toHaveBeenCalled();
Expand All @@ -164,7 +178,7 @@ describe('MDC-based Row Chips', () => {
spyOn(testComponent, 'chipRemove');

// Use the delete to remove the chip
chipInstance._handleKeydown(BACKSPACE_EVENT);
dispatchEvent(chipNativeElement, BACKSPACE_EVENT);
fixture.detectChanges();

expect(testComponent.chipRemove).not.toHaveBeenCalled();
Expand Down
4 changes: 3 additions & 1 deletion src/material/chips/chip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,9 @@ export class MatChip implements OnInit, AfterViewInit, AfterContentInit, DoCheck

/** Handles keyboard events on the chip. */
_handleKeydown(event: KeyboardEvent) {
if (event.keyCode === BACKSPACE || event.keyCode === DELETE) {
// Ignore backspace events where the user is holding down the key
// so that we don't accidentally remove too many chips.
if ((event.keyCode === BACKSPACE && !event.repeat) || event.keyCode === DELETE) {
event.preventDefault();
this.remove();
}
Expand Down
7 changes: 2 additions & 5 deletions tools/public_api_guard/material/chips.md
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ export class MatChipGridChange {
}

// @public
export class MatChipInput implements MatChipTextControl, AfterContentInit, OnChanges, OnDestroy {
export class MatChipInput implements MatChipTextControl, OnChanges, OnDestroy {
constructor(_elementRef: ElementRef<HTMLInputElement>, defaultOptions: MatChipsDefaultOptions, formField?: MatFormField);
addOnBlur: boolean;
_blur(): void;
Expand All @@ -269,15 +269,12 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha
focused: boolean;
id: string;
readonly inputElement: HTMLInputElement;
_keydown(event?: KeyboardEvent): void;
_keyup(event: KeyboardEvent): void;
_keydown(event: KeyboardEvent): void;
// (undocumented)
static ngAcceptInputType_addOnBlur: unknown;
// (undocumented)
static ngAcceptInputType_disabled: unknown;
// (undocumented)
ngAfterContentInit(): void;
// (undocumented)
ngOnChanges(): void;
// (undocumented)
ngOnDestroy(): void;
Expand Down

0 comments on commit 494746b

Please sign in to comment.