Skip to content

Commit b5f15f4

Browse files
crisbetocrisbeto
authored
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.
1 parent 5246431 commit b5f15f4

File tree

15 files changed

+76
-38
lines changed

15 files changed

+76
-38
lines changed

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

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,11 @@ describe('Key managers', () => {
8282
spyOn(keyManager, 'setActiveItem').and.callThrough();
8383
});
8484

85+
afterEach(() => {
86+
keyManager.destroy();
87+
keyManager = null!;
88+
});
89+
8590
it('should maintain the active item if the amount of items changes', () => {
8691
expect(keyManager.activeItemIndex).toBe(0);
8792
expect(keyManager.activeItem!.getLabel()).toBe('one');
@@ -110,6 +115,14 @@ describe('Key managers', () => {
110115
expect(spy).toHaveBeenCalled();
111116
});
112117

118+
it('should complete the tabOut stream on destroy', () => {
119+
const spy = jasmine.createSpy('complete spy');
120+
keyManager.tabOut.pipe(take(1)).subscribe({complete: spy});
121+
keyManager.destroy();
122+
123+
expect(spy).toHaveBeenCalled();
124+
});
125+
113126
it('should emit tabOut when the tab key is pressed with a modifier', () => {
114127
const spy = jasmine.createSpy('tabOut spy');
115128
keyManager.tabOut.pipe(take(1)).subscribe(spy);
@@ -122,27 +135,33 @@ describe('Key managers', () => {
122135

123136
it('should emit an event whenever the active item changes', () => {
124137
const spy = jasmine.createSpy('change spy');
125-
const subscription = keyManager.change.subscribe(spy);
138+
keyManager.change.subscribe(spy);
126139

127140
keyManager.onKeydown(fakeKeyEvents.downArrow);
128141
expect(spy).toHaveBeenCalledTimes(1);
129142

130143
keyManager.onKeydown(fakeKeyEvents.upArrow);
131144
expect(spy).toHaveBeenCalledTimes(2);
132-
133-
subscription.unsubscribe();
134145
});
135146

136147
it('should emit if the active item changed, but not the active index', () => {
137148
const spy = jasmine.createSpy('change spy');
138-
const subscription = keyManager.change.subscribe(spy);
149+
keyManager.change.subscribe(spy);
139150

140151
keyManager.setActiveItem(0);
141152
itemList.reset([new FakeFocusable('zero'), ...itemList.toArray()]);
142153
keyManager.setActiveItem(0);
143154

144155
expect(spy).toHaveBeenCalledTimes(1);
145-
subscription.unsubscribe();
156+
});
157+
158+
it('should complete the change stream on destroy', () => {
159+
const spy = jasmine.createSpy('change spy');
160+
161+
keyManager.change.subscribe({complete: spy});
162+
keyManager.destroy();
163+
164+
expect(spy).toHaveBeenCalled();
146165
});
147166

148167
it('should activate the first item when pressing down on a clean key manager', () => {
@@ -448,7 +467,7 @@ describe('Key managers', () => {
448467
) {
449468
const initialActiveIndex = keyManager.activeItemIndex;
450469
const spy = jasmine.createSpy('change spy');
451-
const subscription = keyManager.change.subscribe(spy);
470+
keyManager.change.subscribe(spy);
452471

453472
expect(context.nextKeyEvent.defaultPrevented).toBe(false);
454473
expect(context.prevKeyEvent.defaultPrevented).toBe(false);
@@ -465,8 +484,6 @@ describe('Key managers', () => {
465484
expect(context.prevKeyEvent.defaultPrevented).toBe(false);
466485
expect(keyManager.activeItemIndex).toBe(initialActiveIndex);
467486
expect(spy).not.toHaveBeenCalled();
468-
469-
subscription.unsubscribe();
470487
}
471488
});
472489

@@ -495,16 +512,14 @@ describe('Key managers', () => {
495512

496513
it('should be able to set the active item without emitting an event', () => {
497514
const spy = jasmine.createSpy('change spy');
498-
const subscription = keyManager.change.subscribe(spy);
515+
keyManager.change.subscribe(spy);
499516

500517
expect(keyManager.activeItemIndex).toBe(0);
501518

502519
keyManager.updateActiveItem(2);
503520

504521
expect(keyManager.activeItemIndex).toBe(2);
505522
expect(spy).not.toHaveBeenCalled();
506-
507-
subscription.unsubscribe();
508523
});
509524

510525
it('should expose the active item correctly', () => {
@@ -629,14 +644,12 @@ describe('Key managers', () => {
629644

630645
it('should not emit an event if the item did not change', () => {
631646
const spy = jasmine.createSpy('change spy');
632-
const subscription = keyManager.change.subscribe(spy);
647+
keyManager.change.subscribe(spy);
633648

634649
keyManager.setActiveItem(2);
635650
keyManager.setActiveItem(2);
636651

637652
expect(spy).toHaveBeenCalledTimes(1);
638-
639-
subscription.unsubscribe();
640653
});
641654
});
642655

@@ -935,6 +948,16 @@ describe('Key managers', () => {
935948

936949
expect(keyManager.isTyping()).toBe(false);
937950
}));
951+
952+
it('should reset isTyping if the key manager is destroyed', fakeAsync(() => {
953+
expect(keyManager.isTyping()).toBe(false);
954+
955+
keyManager.onKeydown(createKeyboardEvent('keydown', 79, 'o')); // types "o"
956+
expect(keyManager.isTyping()).toBe(true);
957+
958+
keyManager.destroy();
959+
expect(keyManager.isTyping()).toBe(false);
960+
}));
938961
});
939962
});
940963

@@ -953,6 +976,11 @@ describe('Key managers', () => {
953976
spyOn(itemList.toArray()[2], 'focus');
954977
});
955978

979+
afterEach(() => {
980+
keyManager.destroy();
981+
keyManager = null!;
982+
});
983+
956984
it('should focus subsequent items when down arrow is pressed', () => {
957985
keyManager.onKeydown(fakeKeyEvents.downArrow);
958986

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
4848
private _wrap = false;
4949
private readonly _letterKeyStream = new Subject<string>();
5050
private _typeaheadSubscription = Subscription.EMPTY;
51+
private _itemChangesSubscription?: Subscription;
5152
private _vertical = true;
5253
private _horizontal: 'ltr' | 'rtl' | null;
5354
private _allowedModifierKeys: ListKeyManagerModifierKey[] = [];
@@ -68,7 +69,7 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
6869
// not have access to a QueryList of the items they want to manage (e.g. when the
6970
// items aren't being collected via `ViewChildren` or `ContentChildren`).
7071
if (_items instanceof QueryList) {
71-
_items.changes.subscribe((newItems: QueryList<T>) => {
72+
this._itemChangesSubscription = _items.changes.subscribe((newItems: QueryList<T>) => {
7273
if (this._activeItem) {
7374
const itemArray = newItems.toArray();
7475
const newIndex = itemArray.indexOf(this._activeItem);
@@ -392,6 +393,16 @@ export class ListKeyManager<T extends ListKeyManagerOption> {
392393
this._activeItemIndex = index;
393394
}
394395

396+
/** Cleans up the key manager. */
397+
destroy() {
398+
this._typeaheadSubscription.unsubscribe();
399+
this._itemChangesSubscription?.unsubscribe();
400+
this._letterKeyStream.complete();
401+
this.tabOut.complete();
402+
this.change.complete();
403+
this._pressedLetters = [];
404+
}
405+
395406
/**
396407
* This method sets the active item, given a list of items and the delta between the
397408
* currently active item and the new active item. It will calculate differently

‎src/cdk/listbox/listbox.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -494,7 +494,7 @@ export class CdkListbox<T = unknown>
494494
}
495495

496496
ngOnDestroy() {
497-
this.listKeyManager.change.complete();
497+
this.listKeyManager?.destroy();
498498
this.destroyed.next();
499499
this.destroyed.complete();
500500
}
@@ -869,9 +869,7 @@ export class CdkListbox<T = unknown>
869869
this.listKeyManager.withHorizontalOrientation(this._dir?.value || 'ltr');
870870
}
871871

872-
this.listKeyManager.change
873-
.pipe(takeUntil(this.destroyed))
874-
.subscribe(() => this._focusActiveOption());
872+
this.listKeyManager.change.subscribe(() => this._focusActiveOption());
875873
}
876874

877875
/** Focus the active option. */

‎src/cdk/menu/menu-base.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ export abstract class CdkMenuBase
111111
}
112112

113113
ngOnDestroy() {
114+
this.keyManager?.destroy();
114115
this.destroyed.next();
115116
this.destroyed.complete();
116117
this.pointerTracker?.destroy();

‎src/cdk/stepper/stepper.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,7 @@ export class CdkStepper implements AfterContentInit, AfterViewInit, OnDestroy {
411411
}
412412

413413
ngOnDestroy() {
414+
this._keyManager?.destroy();
414415
this.steps.destroy();
415416
this._sortedHeaders.destroy();
416417
this._destroyed.next();

‎src/material/autocomplete/autocomplete.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ export abstract class _MatAutocompleteBase
249249
}
250250

251251
ngOnDestroy() {
252+
this._keyManager?.destroy();
252253
this._activeOptionChanges.unsubscribe();
253254
}
254255

‎src/material/chips/chip-set.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,7 @@ export class MatChipSet
147147
}
148148

149149
ngOnDestroy() {
150+
this._keyManager?.destroy();
150151
this._chipActions.destroy();
151152
this._destroyed.next();
152153
this._destroyed.complete();

‎src/material/expansion/accordion.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export class MatAccordion
104104

105105
override ngOnDestroy() {
106106
super.ngOnDestroy();
107+
this._keyManager?.destroy();
107108
this._ownHeaders.destroy();
108109
}
109110
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -411,9 +411,7 @@ export class MatLegacyChipList
411411
.subscribe(dir => this._keyManager.withHorizontalOrientation(dir));
412412
}
413413

414-
this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => {
415-
this._allowFocusEscape();
416-
});
414+
this._keyManager.tabOut.subscribe(() => this._allowFocusEscape());
417415

418416
// When the list changes, re-subscribe
419417
this.chips.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => {
@@ -459,10 +457,10 @@ export class MatLegacyChipList
459457
}
460458

461459
ngOnDestroy() {
460+
this._keyManager?.destroy();
462461
this._destroyed.next();
463462
this._destroyed.complete();
464463
this.stateChanges.complete();
465-
466464
this._dropSubscriptions();
467465
}
468466

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -468,9 +468,7 @@ export class MatLegacySelectionList
468468
}
469469

470470
// If the user attempts to tab out of the selection list, allow focus to escape.
471-
this._keyManager.tabOut.pipe(takeUntil(this._destroyed)).subscribe(() => {
472-
this._allowFocusEscape();
473-
});
471+
this._keyManager.tabOut.subscribe(() => this._allowFocusEscape());
474472

475473
// When the number of options change, update the tabindex of the selection list.
476474
this.options.changes.pipe(startWith(null), takeUntil(this._destroyed)).subscribe(() => {
@@ -522,6 +520,7 @@ export class MatLegacySelectionList
522520
}
523521

524522
ngOnDestroy() {
523+
this._keyManager?.destroy();
525524
this._focusMonitor.stopMonitoring(this._element);
526525
this._destroyed.next();
527526
this._destroyed.complete();

0 commit comments

Comments
 (0)