Skip to content

Commit ccf7a4d

Browse files
crisbetojelbourn
authored andcommitted
fix(overlay): OverlayKeyboardDispatcher not removing global event listener (#10160)
Fixes the `OverlayKeyboardDispatcher` not removing the global event listener, even though the subscription gets removed correctly. The issue seems to come from the fact that rxjs attaches the event using `useCapture = true`, however it doesn't pass the `useCapture` parameter when unsubscribing, which leaves listener in place. These changes switch to using `addEventListener` and `removeEventListener` to manage the event. Fixes #10143.
1 parent 67cc2c2 commit ccf7a4d

File tree

2 files changed

+39
-32
lines changed

2 files changed

+39
-32
lines changed

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.spec.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,20 @@ describe('OverlayKeyboardDispatcher', () => {
151151
expect(spy).toHaveBeenCalledTimes(1);
152152
});
153153

154+
it('should dispose of the global keyboard event handler correctly', () => {
155+
const overlayRef = overlay.create();
156+
const body = document.body;
157+
158+
spyOn(body, 'addEventListener');
159+
spyOn(body, 'removeEventListener');
160+
161+
keyboardDispatcher.add(overlayRef);
162+
expect(body.addEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function), true);
163+
164+
overlayRef.dispose();
165+
expect(body.removeEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function), true);
166+
});
167+
154168
});
155169

156170

src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts

Lines changed: 25 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,6 @@
88

99
import {Injectable, Inject, InjectionToken, Optional, SkipSelf, OnDestroy} from '@angular/core';
1010
import {OverlayRef} from '../overlay-ref';
11-
import {Subscription} from 'rxjs/Subscription';
12-
import {filter} from 'rxjs/operators/filter';
13-
import {fromEvent} from 'rxjs/observable/fromEvent';
1411
import {DOCUMENT} from '@angular/common';
1512

1613
/**
@@ -24,19 +21,23 @@ export class OverlayKeyboardDispatcher implements OnDestroy {
2421
/** Currently attached overlays in the order they were attached. */
2522
_attachedOverlays: OverlayRef[] = [];
2623

27-
private _keydownEventSubscription: Subscription | null;
24+
private _document: Document;
25+
private _isAttached: boolean;
2826

29-
constructor(@Inject(DOCUMENT) private _document: any) {}
27+
constructor(@Inject(DOCUMENT) document: any) {
28+
this._document = document;
29+
}
3030

3131
ngOnDestroy() {
32-
this._unsubscribeFromKeydownEvents();
32+
this._detach();
3333
}
3434

3535
/** Add a new overlay to the list of attached overlay refs. */
3636
add(overlayRef: OverlayRef): void {
3737
// Lazily start dispatcher once first overlay is added
38-
if (!this._keydownEventSubscription) {
39-
this._subscribeToKeydownEvents();
38+
if (!this._isAttached) {
39+
this._document.body.addEventListener('keydown', this._keydownListener, true);
40+
this._isAttached = true;
4041
}
4142

4243
this._attachedOverlays.push(overlayRef);
@@ -52,30 +53,7 @@ export class OverlayKeyboardDispatcher implements OnDestroy {
5253

5354
// Remove the global listener once there are no more overlays.
5455
if (this._attachedOverlays.length === 0) {
55-
this._unsubscribeFromKeydownEvents();
56-
}
57-
}
58-
59-
/**
60-
* Subscribe to keydown events that land on the body and dispatch those
61-
* events to the appropriate overlay.
62-
*/
63-
private _subscribeToKeydownEvents(): void {
64-
const bodyKeydownEvents = fromEvent<KeyboardEvent>(this._document.body, 'keydown', true);
65-
66-
this._keydownEventSubscription = bodyKeydownEvents.pipe(
67-
filter(() => !!this._attachedOverlays.length)
68-
).subscribe(event => {
69-
// Dispatch keydown event to the correct overlay.
70-
this._selectOverlayFromEvent(event)._keydownEvents.next(event);
71-
});
72-
}
73-
74-
/** Removes the global keydown subscription. */
75-
private _unsubscribeFromKeydownEvents(): void {
76-
if (this._keydownEventSubscription) {
77-
this._keydownEventSubscription.unsubscribe();
78-
this._keydownEventSubscription = null;
56+
this._detach();
7957
}
8058
}
8159

@@ -91,6 +69,21 @@ export class OverlayKeyboardDispatcher implements OnDestroy {
9169
return targetedOverlay || this._attachedOverlays[this._attachedOverlays.length - 1];
9270
}
9371

72+
/** Detaches the global keyboard event listener. */
73+
private _detach() {
74+
if (this._isAttached) {
75+
this._document.body.removeEventListener('keydown', this._keydownListener, true);
76+
this._isAttached = false;
77+
}
78+
}
79+
80+
/** Keyboard event listener that will be attached to the body. */
81+
private _keydownListener = (event: KeyboardEvent) => {
82+
if (this._attachedOverlays.length) {
83+
// Dispatch keydown event to the correct overlay.
84+
this._selectOverlayFromEvent(event)._keydownEvents.next(event);
85+
}
86+
}
9487
}
9588

9689
/** @docs-private */

0 commit comments

Comments
 (0)