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

Author: devversion

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<any>;
+  let chips: QueryList<MatChip>;
   let manager: FocusKeyManager<MatChip>;
   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: `
+    <mat-form-field>
+      <mat-chip-list>
+        <div *ngFor="let i of chips">
+          <mat-chip [value]="i" (removed)="removeChip($event)">
+            Chip {{i + 1}}
+            <span matChipRemove>Remove</span>
+          </mat-chip>
+        </div>
+      </mat-chip-list>
+    </mat-form-field>
+  `
+})
+class ChipListWithRemove {
+  chips = [0, 1, 2, 3, 4];
+
+  removeChip(event: MatChipEvent) {
+    this.chips.splice(event.chip.value, 1);
+  }
+}

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<MatChip, boolean> = 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<void>();
 
   /** 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;
+      }
     });
   }
 }

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<any>;
+  let fixture: ComponentFixture<TestChip>;
   let testChip: TestChip;
   let chipDebugElement: DebugElement;
   let chipNativeElement: HTMLElement;

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();
   }
 }