Skip to content

Commit

Permalink
fix(material/chips): simplify repeat chip removal prevention (angular…
Browse files Browse the repository at this point in the history
…#29048)

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 16, 2024
1 parent ce87e55 commit 46bcc21
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 67 deletions.
34 changes: 11 additions & 23 deletions src/material/chips/chip-grid.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {animate, style, transition, trigger} from '@angular/animations';
import {Direction, Directionality} from '@angular/cdk/bidi';
import {
A,
BACKSPACE,
DELETE,
END,
Expand All @@ -13,6 +12,8 @@ import {
TAB,
} from '@angular/cdk/keycodes';
import {
createKeyboardEvent,
dispatchEvent,
dispatchFakeEvent,
dispatchKeyboardEvent,
patchElementFocus,
Expand Down Expand Up @@ -789,28 +790,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 46bcc21

Please sign in to comment.