From 3da390e36df3a3f63695535c4f9fdac9b137eaee Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 24 Aug 2018 16:52:44 +0200 Subject: [PATCH] fix(chips): focus not restored properly if chip has been removed by click (#12788) * Currently if someone tries to remove a chip by clicking the `[matChipRemove]`, the click event will bubble up to the potential `MatFormField` and cause the `onContainerClick` to be invoked. This causes the focus to be always moved to the first chip (which is not good for accessibility). * Replaces multiple subscriptions in the chip list with a `takeUntil`. Also fixes that the chip remove subscription is re-created multiple times but not cleaned up. * Simplifies and cleans up the logic to restore focus after a chip has been destroyed. PR is based on: #12416 --- src/lib/chips/chip-list.spec.ts | 64 ++++++++++++++++--- src/lib/chips/chip-list.ts | 103 +++++++++++------------------- src/lib/chips/chip-remove.spec.ts | 2 +- src/lib/chips/chip.ts | 14 ++-- 4 files changed, 105 insertions(+), 78 deletions(-) diff --git a/src/lib/chips/chip-list.spec.ts b/src/lib/chips/chip-list.spec.ts index 52f0c3b190b3..f39987cbfa6a 100644 --- a/src/lib/chips/chip-list.spec.ts +++ b/src/lib/chips/chip-list.spec.ts @@ -1,3 +1,4 @@ +import {animate, style, transition, trigger} from '@angular/animations'; import {FocusKeyManager} from '@angular/cdk/a11y'; import {Directionality, Direction} from '@angular/cdk/bidi'; import { @@ -15,28 +16,28 @@ import { createKeyboardEvent, dispatchFakeEvent, dispatchKeyboardEvent, + dispatchMouseEvent, MockNgZone, } from '@angular/cdk/testing'; import { Component, DebugElement, + NgZone, + Provider, QueryList, + Type, ViewChild, ViewChildren, - Type, - Provider, - NgZone, } from '@angular/core'; import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'; import {FormControl, FormsModule, NgForm, ReactiveFormsModule, Validators} from '@angular/forms'; import {MatFormFieldModule} from '@angular/material/form-field'; import {By} from '@angular/platform-browser'; -import {NoopAnimationsModule, BrowserAnimationsModule} from '@angular/platform-browser/animations'; +import {BrowserAnimationsModule, NoopAnimationsModule} from '@angular/platform-browser/animations'; import {MatInputModule} from '../input/index'; import {MatChip} from './chip'; import {MatChipInputEvent} from './chip-input'; -import {MatChipList, MatChipsModule} from './index'; -import {trigger, transition, style, animate} from '@angular/animations'; +import {MatChipEvent, MatChipList, MatChipRemove, MatChipsModule} from './index'; describe('MatChipList', () => { @@ -45,7 +46,7 @@ describe('MatChipList', () => { let chipListNativeElement: HTMLElement; let chipListInstance: MatChipList; let testComponent: StandardChipList; - let chips: QueryList; + let chips: QueryList; let manager: FocusKeyManager; let zone: MockNgZone; @@ -168,6 +169,7 @@ describe('MatChipList', () => { }); describe('on chip destroy', () => { + it('should focus the next item', () => { let array = chips.toArray(); let midItem = array[2]; @@ -183,7 +185,6 @@ describe('MatChipList', () => { expect(manager.activeItemIndex).toEqual(2); }); - it('should focus the previous item', () => { let array = chips.toArray(); let lastIndex = array.length - 1; @@ -505,6 +506,32 @@ describe('MatChipList', () => { }); + describe('with chip remove', () => { + let chipList: MatChipList; + let chipRemoveDebugElements: DebugElement[]; + + beforeEach(() => { + fixture = createComponent(ChipListWithRemove); + fixture.detectChanges(); + + chipList = fixture.debugElement.query(By.directive(MatChipList)).componentInstance; + chipRemoveDebugElements = fixture.debugElement.queryAll(By.directive(MatChipRemove)); + chips = chipList.chips; + }); + + it('should properly focus next item if chip is removed through click', () => { + chips.toArray()[2].focus(); + + // Destroy the third focused chip by dispatching a bubbling click event on the + // associated chip remove element. + dispatchMouseEvent(chipRemoveDebugElements[2].nativeElement, 'click'); + fixture.detectChanges(); + + expect(chips.toArray()[2].value).not.toBe(2, 'Expected the third chip to be removed.'); + expect(chipList._keyManager.activeItemIndex).toBe(2); + }); + }); + describe('selection logic', () => { let formField: HTMLElement; let nativeChips: HTMLElement[]; @@ -1438,3 +1465,24 @@ class StandardChipListWithAnimations { } } +@Component({ + template: ` + + +
+ + Chip {{i + 1}} + Remove + +
+ + + ` +}) +class ChipListWithRemove { + chips = [0, 1, 2, 3, 4]; + + removeChip(event: MatChipEvent) { + this.chips.splice(event.chip.value, 1); + } +} diff --git a/src/lib/chips/chip-list.ts b/src/lib/chips/chip-list.ts index cf4f9bbd93f8..3e0701692051 100644 --- a/src/lib/chips/chip-list.ts +++ b/src/lib/chips/chip-list.ts @@ -32,8 +32,8 @@ import { import {ControlValueAccessor, FormGroupDirective, NgControl, NgForm} from '@angular/forms'; import {CanUpdateErrorState, ErrorStateMatcher, mixinErrorState} from '@angular/material/core'; import {MatFormFieldControl} from '@angular/material/form-field'; -import {merge, Observable, Subscription} from 'rxjs'; -import {startWith} from 'rxjs/operators'; +import {merge, Observable, Subject, Subscription} from 'rxjs'; +import {startWith, takeUntil} from 'rxjs/operators'; import {MatChip, MatChipEvent, MatChipSelectionChange} from './chip'; import {MatChipInput} from './chip-input'; @@ -102,17 +102,15 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo */ readonly controlType: string = 'mat-chip-list'; - /** When a chip is destroyed, we track the index so we can focus the appropriate next chip. */ - protected _lastDestroyedIndex: number|null = null; - - /** Track which chips we're listening to for focus/destruction. */ - protected _chipSet: WeakMap = new WeakMap(); - - /** Subscription to tabbing out from the chip list. */ - private _tabOutSubscription = Subscription.EMPTY; + /** + * When a chip is destroyed, we store the index of the destroyed chip until the chips + * query list notifies about the update. This is necessary because we cannot determine an + * appropriate chip that should receive focus until the array of chips updated completely. + */ + private _lastDestroyedChipIndex: number | null = null; - /** Subscription to changes in the chip list. */ - private _changeSubscription: Subscription; + /** Subject that emits when the component has been destroyed. */ + private _destroyed = new Subject(); /** Subscription to focus changes in the chips. */ private _chipFocusSubscription: Subscription | null; @@ -350,13 +348,13 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo // Prevents the chip list from capturing focus and redirecting // it back to the first chip when the user tabs out. - this._tabOutSubscription = this._keyManager.tabOut.subscribe(() => { + this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => { this._tabIndex = -1; setTimeout(() => this._tabIndex = this._userTabIndex || 0); }); // When the list changes, re-subscribe - this._changeSubscription = this.chips.changes.pipe(startWith(null)).subscribe(() => { + this.chips.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => { this._resetChips(); // Reset chips selected/deselected status @@ -387,18 +385,11 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo } ngOnDestroy() { - this._tabOutSubscription.unsubscribe(); - - if (this._changeSubscription) { - this._changeSubscription.unsubscribe(); - } - - if (this._chipRemoveSubscription) { - this._chipRemoveSubscription.unsubscribe(); - } + this._destroyed.next(); + this._destroyed.complete(); + this.stateChanges.complete(); this._dropSubscriptions(); - this.stateChanges.complete(); } @@ -507,49 +498,19 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo } /** - * Update key manager's active item when chip is deleted. - * If the deleted chip is the last chip in chip list, focus the new last chip. - * Otherwise focus the next chip in the list. - * Save `_lastDestroyedIndex` so we can set the correct focus. - */ - protected _updateKeyManager(chip: MatChip) { - let chipIndex: number = this.chips.toArray().indexOf(chip); - if (this._isValidIndex(chipIndex)) { - if (chip._hasFocus) { - // Check whether the chip is not the last item - if (chipIndex < this.chips.length - 1) { - this._keyManager.setActiveItem(chipIndex); - } else if (chipIndex - 1 >= 0) { - this._keyManager.setActiveItem(chipIndex - 1); - } - } - if (this._keyManager.activeItemIndex === chipIndex) { - this._lastDestroyedIndex = chipIndex; - } - } - } - - /** - * Checks to see if a focus chip was recently destroyed so that we can refocus the next closest - * one. + * If the amount of chips changed, we need to update the key manager state and make sure + * that to so that we can refocus the + * next closest one. */ protected _updateFocusForDestroyedChips() { - const chipsArray = this.chips.toArray(); - - if (this._lastDestroyedIndex != null && chipsArray.length > 0 && (this.focused || - (this._keyManager.activeItem && chipsArray.indexOf(this._keyManager.activeItem) === -1))) { - // Check whether the destroyed chip was the last item - const newFocusIndex = Math.min(this._lastDestroyedIndex, chipsArray.length - 1); - this._keyManager.setActiveItem(newFocusIndex); - const focusChip = this._keyManager.activeItem; - // Focus the chip - if (focusChip) { - focusChip.focus(); - } + if (this._lastDestroyedChipIndex == null || !this.chips.length) { + return; } - // Reset our destroyed index - this._lastDestroyedIndex = null; + const newChipIndex = Math.min(this._lastDestroyedChipIndex, this.chips.length - 1); + + this._keyManager.setActiveItem(newChipIndex); + this._lastDestroyedChipIndex = null; } /** @@ -702,7 +663,6 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo this._listenToChipsRemoved(); } - private _dropSubscriptions() { if (this._chipFocusSubscription) { this._chipFocusSubscription.unsubscribe(); @@ -718,6 +678,11 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo this._chipSelectionSubscription.unsubscribe(); this._chipSelectionSubscription = null; } + + if (this._chipRemoveSubscription) { + this._chipRemoveSubscription.unsubscribe(); + this._chipRemoveSubscription = null; + } } /** Listens to user-generated selection events on each chip. */ @@ -761,7 +726,15 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo private _listenToChipsRemoved(): void { this._chipRemoveSubscription = this.chipRemoveChanges.subscribe(event => { - this._updateKeyManager(event.chip); + const chip = event.chip; + const chipIndex = this.chips.toArray().indexOf(event.chip); + + // In case the chip that will be removed is currently focused, we temporarily store + // the index in order to be able to determine an appropriate sibling chip that will + // receive focus. + if (this._isValidIndex(chipIndex) && chip._hasFocus) { + this._lastDestroyedChipIndex = chipIndex; + } }); } } diff --git a/src/lib/chips/chip-remove.spec.ts b/src/lib/chips/chip-remove.spec.ts index e9294176b286..7e7dc1f6404a 100644 --- a/src/lib/chips/chip-remove.spec.ts +++ b/src/lib/chips/chip-remove.spec.ts @@ -4,7 +4,7 @@ import {async, ComponentFixture, TestBed} from '@angular/core/testing'; import {MatChip, MatChipsModule} from './index'; describe('Chip Remove', () => { - let fixture: ComponentFixture; + let fixture: ComponentFixture; let testChip: TestChip; let chipDebugElement: DebugElement; let chipNativeElement: HTMLElement; diff --git a/src/lib/chips/chip.ts b/src/lib/chips/chip.ts index f4e41ac1e4fb..d33193b8461e 100644 --- a/src/lib/chips/chip.ts +++ b/src/lib/chips/chip.ts @@ -391,17 +391,23 @@ export class MatChip extends _MatChipMixinBase implements FocusableOption, OnDes selector: '[matChipRemove]', host: { 'class': 'mat-chip-remove mat-chip-trailing-icon', - '(click)': '_handleClick()', + '(click)': '_handleClick($event)', } }) export class MatChipRemove { - constructor(protected _parentChip: MatChip) { - } + constructor(protected _parentChip: MatChip) {} /** Calls the parent chip's public `remove()` method if applicable. */ - _handleClick(): void { + _handleClick(event: Event): void { if (this._parentChip.removable) { this._parentChip.remove(); } + + // We need to stop event propagation because otherwise the event will bubble up to the + // form field and cause the `onContainerClick` method to be invoked. This method would then + // reset the focused chip that has been focused after chip removal. Usually the parent + // the parent click listener of the `MatChip` would prevent propagation, but it can happen + // that the chip is being removed before the event bubbles up. + event.stopPropagation(); } }