fix(cdk/a11y): clean up list key manager on destroy (#25715)

Historically we haven't needed to do any cleanup for the `ListKeyManager`, because it didn't contain any rxjs subscriptions. Over time we've accumulated more functionality which now needs to be cleaned up.

These changes introduce a `destroy` method and calls it in all the relevant places. I also removed any places where we previously had to do manual cleanup.

Fixes #25537.

Author: crisbeto

src/cdk/a11y/key-manager/list-key-manager.spec.ts

@@ -82,6 +82,11 @@ describe('Key managers', () => {
       spyOn(keyManager, 'setActiveItem').and.callThrough();
     });
 
+    afterEach(() => {
+      keyManager.destroy();
+      keyManager = null!;
+    });
+
     it('should maintain the active item if the amount of items changes', () => {
       expect(keyManager.activeItemIndex).toBe(0);
       expect(keyManager.activeItem!.getLabel()).toBe('one');
@@ -110,6 +115,14 @@ describe('Key managers', () => {
         expect(spy).toHaveBeenCalled();
       });
 
+      it('should complete the tabOut stream on destroy', () => {
+        const spy = jasmine.createSpy('complete spy');
+        keyManager.tabOut.pipe(take(1)).subscribe({complete: spy});
+        keyManager.destroy();
+
+        expect(spy).toHaveBeenCalled();
+      });
+
       it('should emit tabOut when the tab key is pressed with a modifier', () => {
         const spy = jasmine.createSpy('tabOut spy');
         keyManager.tabOut.pipe(take(1)).subscribe(spy);
@@ -122,27 +135,33 @@ describe('Key managers', () => {
 
       it('should emit an event whenever the active item changes', () => {
         const spy = jasmine.createSpy('change spy');
-        const subscription = keyManager.change.subscribe(spy);
+        keyManager.change.subscribe(spy);
 
         keyManager.onKeydown(fakeKeyEvents.downArrow);
         expect(spy).toHaveBeenCalledTimes(1);
 
         keyManager.onKeydown(fakeKeyEvents.upArrow);
         expect(spy).toHaveBeenCalledTimes(2);
-
-        subscription.unsubscribe();
       });
 
       it('should emit if the active item changed, but not the active index', () => {
         const spy = jasmine.createSpy('change spy');
-        const subscription = keyManager.change.subscribe(spy);
+        keyManager.change.subscribe(spy);
 
         keyManager.setActiveItem(0);
         itemList.reset([new FakeFocusable('zero'), ...itemList.toArray()]);
         keyManager.setActiveItem(0);
 
         expect(spy).toHaveBeenCalledTimes(1);
-        subscription.unsubscribe();
+      });
+
+      it('should complete the change stream on destroy', () => {
+        const spy = jasmine.createSpy('change spy');
+
+        keyManager.change.subscribe({complete: spy});
+        keyManager.destroy();
+
+        expect(spy).toHaveBeenCalled();
       });
 
       it('should activate the first item when pressing down on a clean key manager', () => {
@@ -448,7 +467,7 @@ describe('Key managers', () => {
       ) {
         const initialActiveIndex = keyManager.activeItemIndex;
         const spy = jasmine.createSpy('change spy');
-        const subscription = keyManager.change.subscribe(spy);
+        keyManager.change.subscribe(spy);
 
         expect(context.nextKeyEvent.defaultPrevented).toBe(false);
         expect(context.prevKeyEvent.defaultPrevented).toBe(false);
@@ -465,8 +484,6 @@ describe('Key managers', () => {
         expect(context.prevKeyEvent.defaultPrevented).toBe(false);
         expect(keyManager.activeItemIndex).toBe(initialActiveIndex);
         expect(spy).not.toHaveBeenCalled();
-
-        subscription.unsubscribe();
       }
     });
 
@@ -495,16 +512,14 @@ describe('Key managers', () => {
 
       it('should be able to set the active item without emitting an event', () => {
         const spy = jasmine.createSpy('change spy');
-        const subscription = keyManager.change.subscribe(spy);
+        keyManager.change.subscribe(spy);
 
         expect(keyManager.activeItemIndex).toBe(0);
 
         keyManager.updateActiveItem(2);
 
         expect(keyManager.activeItemIndex).toBe(2);
         expect(spy).not.toHaveBeenCalled();
-
-        subscription.unsubscribe();
       });
 
       it('should expose the active item correctly', () => {
@@ -629,14 +644,12 @@ describe('Key managers', () => {
 
       it('should not emit an event if the item did not change', () => {
         const spy = jasmine.createSpy('change spy');
-        const subscription = keyManager.change.subscribe(spy);
+        keyManager.change.subscribe(spy);
 
         keyManager.setActiveItem(2);
         keyManager.setActiveItem(2);
 
         expect(spy).toHaveBeenCalledTimes(1);
-
-        subscription.unsubscribe();
       });
     });
 
@@ -935,6 +948,16 @@ describe('Key managers', () => {
 
         expect(keyManager.isTyping()).toBe(false);
       }));
+
+      it('should reset isTyping if the key manager is destroyed', fakeAsync(() => {
+        expect(keyManager.isTyping()).toBe(false);
+
+        keyManager.onKeydown(createKeyboardEvent('keydown', 79, 'o')); // types "o"
+        expect(keyManager.isTyping()).toBe(true);
+
+        keyManager.destroy();
+        expect(keyManager.isTyping()).toBe(false);
+      }));
     });
   });
 
@@ -953,6 +976,11 @@ describe('Key managers', () => {
       spyOn(itemList.toArray()[2], 'focus');
     });
 
+    afterEach(() => {
+      keyManager.destroy();
+      keyManager = null!;
+    });
+
     it('should focus subsequent items when down arrow is pressed', () => {
       keyManager.onKeydown(fakeKeyEvents.downArrow);
 

src/cdk/a11y/key-manager/list-key-manager.ts

@@ -48,6 +48,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
   private _wrap = false;
   private readonly _letterKeyStream = new Subject<string>();
   private _typeaheadSubscription = Subscription.EMPTY;
+  private _itemChangesSubscription?: Subscription;
   private _vertical = true;
   private _horizontal: 'ltr' | 'rtl' | null;
   private _allowedModifierKeys: ListKeyManagerModifierKey[] = [];
@@ -68,7 +69,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
     // not have access to a QueryList of the items they want to manage (e.g. when the
     // items aren't being collected via `ViewChildren` or `ContentChildren`).
     if (_items instanceof QueryList) {
-      _items.changes.subscribe((newItems: QueryList<T>) => {
+      this._itemChangesSubscription = _items.changes.subscribe((newItems: QueryList<T>) => {
         if (this._activeItem) {
           const itemArray = newItems.toArray();
           const newIndex = itemArray.indexOf(this._activeItem);
@@ -392,6 +393,16 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
     this._activeItemIndex = index;
   }
 
+  /** Cleans up the key manager. */
+  destroy() {
+    this._typeaheadSubscription.unsubscribe();
+    this._itemChangesSubscription?.unsubscribe();
+    this._letterKeyStream.complete();
+    this.tabOut.complete();
+    this.change.complete();
+    this._pressedLetters = [];
+  }
+
   /**
    * This method sets the active item, given a list of items and the delta between the
    * currently active item and the new active item. It will calculate differently

src/cdk/listbox/listbox.ts

@@ -494,7 +494,7 @@ export class CdkListbox<T = unknown>
   }
 
   ngOnDestroy() {
-    this.listKeyManager.change.complete();
+    this.listKeyManager?.destroy();
     this.destroyed.next();
     this.destroyed.complete();
   }
@@ -869,9 +869,7 @@ export class CdkListbox<T = unknown>
       this.listKeyManager.withHorizontalOrientation(this._dir?.value || 'ltr');
     }
 
-    this.listKeyManager.change
-      .pipe(takeUntil(this.destroyed))
-      .subscribe(() => this._focusActiveOption());
+    this.listKeyManager.change.subscribe(() => this._focusActiveOption());
   }
 
   /** Focus the active option. */

src/cdk/menu/menu-base.ts

@@ -111,6 +111,7 @@ export abstract class CdkMenuBase
   }
 
   ngOnDestroy() {
+    this.keyManager?.destroy();
     this.destroyed.next();
     this.destroyed.complete();
     this.pointerTracker?.destroy();

src/cdk/stepper/stepper.ts

@@ -411,6 +411,7 @@ export class CdkStepper implements AfterContentInit, AfterViewInit, OnDestroy {
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this.steps.destroy();
     this._sortedHeaders.destroy();
     this._destroyed.next();

src/material/autocomplete/autocomplete.ts

@@ -249,6 +249,7 @@ export abstract class _MatAutocompleteBase
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._activeOptionChanges.unsubscribe();
   }
 

src/material/chips/chip-set.ts

@@ -147,6 +147,7 @@ export class MatChipSet
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._chipActions.destroy();
     this._destroyed.next();
     this._destroyed.complete();

src/material/expansion/accordion.ts

@@ -104,6 +104,7 @@ export class MatAccordion
 
   override ngOnDestroy() {
     super.ngOnDestroy();
+    this._keyManager?.destroy();
     this._ownHeaders.destroy();
   }
 }

src/material/legacy-chips/chip-list.ts

@@ -411,9 +411,7 @@ export class MatLegacyChipList
         .subscribe(dir => this._keyManager.withHorizontalOrientation(dir));
     }
 
-    this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => {
-      this._allowFocusEscape();
-    });
+    this._keyManager.tabOut.subscribe(() => this._allowFocusEscape());
 
     // When the list changes, re-subscribe
     this.chips.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => {
@@ -459,10 +457,10 @@ export class MatLegacyChipList
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._destroyed.next();
     this._destroyed.complete();
     this.stateChanges.complete();
-
     this._dropSubscriptions();
   }
 

src/material/legacy-list/selection-list.ts

@@ -468,9 +468,7 @@ export class MatLegacySelectionList
     }
 
     // If the user attempts to tab out of the selection list, allow focus to escape.
-    this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => {
-      this._allowFocusEscape();
-    });
+    this._keyManager.tabOut.subscribe(() => this._allowFocusEscape());
 
     // When the number of options change, update the tabindex of the selection list.
     this.options.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => {
@@ -522,6 +520,7 @@ export class MatLegacySelectionList
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._focusMonitor.stopMonitoring(this._element);
     this._destroyed.next();
     this._destroyed.complete();

src/material/list/selection-list.ts

@@ -176,6 +176,7 @@ export class MatSelectionList
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._element.nativeElement.removeEventListener('focusin', this._handleFocusin);
     this._element.nativeElement.removeEventListener('focusout', this._handleFocusout);
     this._destroyed.next();
@@ -377,9 +378,7 @@ export class MatSelectionList
     this._resetActiveOption();
 
     // Move the tabindex to the currently-focused list item.
-    this._keyManager.change
-      .pipe(takeUntil(this._destroyed))
-      .subscribe(activeItemIndex => this._setActiveOption(activeItemIndex));
+    this._keyManager.change.subscribe(activeItemIndex => this._setActiveOption(activeItemIndex));
 
     // If the active item is removed from the list, reset back to the first one.
     this._items.changes.pipe(takeUntil(this._destroyed)).subscribe(() => {

src/material/menu/menu.ts

@@ -40,7 +40,7 @@ import {
   UP_ARROW,
   hasModifierKey,
 } from '@angular/cdk/keycodes';
-import {merge, Observable, Subject, Subscription} from 'rxjs';
+import {merge, Observable, Subject} from 'rxjs';
 import {startWith, switchMap, take} from 'rxjs/operators';
 import {MatMenuItem} from './menu-item';
 import {MatMenuPanel, MAT_MENU_PANEL} from './menu-panel';
@@ -112,9 +112,6 @@ export class _MatMenuBase
   /** Only the direct descendant menu items. */
   _directDescendantItems = new QueryList<MatMenuItem>();
 
-  /** Subscription to tab events on the menu panel */
-  private _tabSubscription = Subscription.EMPTY;
-
   /** Config object to be passed into the menu's ngClass */
   _classList: {[key: string]: boolean} = {};
 
@@ -305,7 +302,7 @@ export class _MatMenuBase
       .withWrap()
       .withTypeAhead()
       .withHomeAndEnd();
-    this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.closed.emit('tab'));
+    this._keyManager.tabOut.subscribe(() => this.closed.emit('tab'));
 
     // If a user manually (programmatically) focuses a menu item, we need to reflect that focus
     // change back to the key manager. Note that we don't need to unsubscribe here because _focused
@@ -337,8 +334,8 @@ export class _MatMenuBase
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._directDescendantItems.destroy();
-    this._tabSubscription.unsubscribe();
     this.closed.complete();
   }
 

src/material/select/select.ts

@@ -581,6 +581,7 @@ export abstract class _MatSelectBase<C>
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._destroy.next();
     this._destroy.complete();
     this.stateChanges.complete();
@@ -917,7 +918,7 @@ export abstract class _MatSelectBase<C>
       .withPageUpDown()
       .withAllowedModifierKeys(['shiftKey']);
 
-    this._keyManager.tabOut.pipe(takeUntil(this._destroy)).subscribe(() => {
+    this._keyManager.tabOut.subscribe(() => {
       if (this.panelOpen) {
         // Select the active item when tabbing away. This is consistent with how the native
         // select behaves. Note that we only want to do this in single selection mode.
@@ -932,7 +933,7 @@ export abstract class _MatSelectBase<C>
       }
     });
 
-    this._keyManager.change.pipe(takeUntil(this._destroy)).subscribe(() => {
+    this._keyManager.change.subscribe(() => {
       if (this._panelOpen && this.panel) {
         this._scrollOptionIntoView(this._keyManager.activeItemIndex || 0);
       } else if (!this._panelOpen && !this.multiple && this._keyManager.activeItem) {

src/material/tabs/paginated-tab-header.ts

@@ -249,7 +249,7 @@ export abstract class MatPaginatedTabHeader
     // If there is a change in the focus key manager we need to emit the `indexFocused`
     // event in order to provide a public event that notifies about focus changes. Also we realign
     // the tabs container by scrolling the new focused tab into the visible section.
-    this._keyManager.change.pipe(takeUntil(this._destroyed)).subscribe(newFocusIndex => {
+    this._keyManager.change.subscribe(newFocusIndex => {
       this.indexFocused.emit(newFocusIndex);
       this._setTabFocus(newFocusIndex);
     });
@@ -313,6 +313,7 @@ export abstract class MatPaginatedTabHeader
   }
 
   ngOnDestroy() {
+    this._keyManager?.destroy();
     this._destroyed.next();
     this._destroyed.complete();
     this._stopScrolling.complete();

tools/public_api_guard/cdk/a11y.md

@@ -343,6 +343,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
     get activeItem(): T | null;
     get activeItemIndex(): number | null;
     readonly change: Subject<number>;
+    destroy(): void;
     isTyping(): boolean;
     onKeydown(event: KeyboardEvent): void;
     setActiveItem(index: number): void;