From 4cc7c15859a4e6780117ec55f02e41410172aa4e Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 16 May 2024 08:02:01 +0200 Subject: [PATCH] fix(material/chips): simplify repeat chip removal prevention (#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. (cherry picked from commit 46bcc21cbc19ff8ec946485febf70a85146f1026) --- src/material/chips/chip-grid.spec.ts | 34 +++++++------------ src/material/chips/chip-input.ts | 42 +++++------------------- src/material/chips/chip-row.spec.ts | 22 ++++++++++--- src/material/chips/chip.ts | 4 ++- tools/public_api_guard/material/chips.md | 7 ++-- 5 files changed, 42 insertions(+), 67 deletions(-) diff --git a/src/material/chips/chip-grid.spec.ts b/src/material/chips/chip-grid.spec.ts index fcfeeff6122c..acdaa9629856 100644 --- a/src/material/chips/chip-grid.spec.ts +++ b/src/material/chips/chip-grid.spec.ts @@ -1,7 +1,6 @@ import {animate, style, transition, trigger} from '@angular/animations'; import {Direction, Directionality} from '@angular/cdk/bidi'; import { - A, BACKSPACE, DELETE, END, @@ -13,6 +12,8 @@ import { TAB, } from '@angular/cdk/keycodes'; import { + createKeyboardEvent, + dispatchEvent, dispatchFakeEvent, dispatchKeyboardEvent, MockNgZone, @@ -794,28 +795,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 diff --git a/src/material/chips/chip-input.ts b/src/material/chips/chip-input.ts index 68b590bc9882..6f39cbdb906c 100644 --- a/src/material/chips/chip-input.ts +++ b/src/material/chips/chip-input.ts @@ -8,7 +8,6 @@ import {BACKSPACE, hasModifierKey} from '@angular/cdk/keycodes'; import { - AfterContentInit, Directive, ElementRef, EventEmitter, @@ -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()', @@ -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; @@ -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); } } @@ -201,7 +177,6 @@ export class MatChipInput implements MatChipTextControl, AfterContentInit, OnCha _focus() { this.focused = true; - this._focusLastChipOnBackspace = this.empty; this._chipGrid.stateChanges.next(); } @@ -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 { diff --git a/src/material/chips/chip-row.spec.ts b/src/material/chips/chip-row.spec.ts index e44e182ba468..8dcb6166bfe7 100644 --- a/src/material/chips/chip-row.spec.ts +++ b/src/material/chips/chip-row.spec.ts @@ -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(); @@ -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', () => { @@ -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(); @@ -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(); diff --git a/src/material/chips/chip.ts b/src/material/chips/chip.ts index aa981168bd6a..ae2c19b5a225 100644 --- a/src/material/chips/chip.ts +++ b/src/material/chips/chip.ts @@ -333,7 +333,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(); } diff --git a/tools/public_api_guard/material/chips.md b/tools/public_api_guard/material/chips.md index 974205145865..66249e1a9678 100644 --- a/tools/public_api_guard/material/chips.md +++ b/tools/public_api_guard/material/chips.md @@ -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, defaultOptions: MatChipsDefaultOptions, formField?: MatFormField); addOnBlur: boolean; _blur(): void; @@ -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;