Skip to content

Commit

Permalink
fix(overlay): OverlayKeyboardDispatcher not removing global event lis…
Browse files Browse the repository at this point in the history
…tener (#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.
  • Loading branch information
crisbeto authored and jelbourn committed Mar 6, 2018
1 parent 67cc2c2 commit ccf7a4d
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 32 deletions.
14 changes: 14 additions & 0 deletions src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,20 @@ describe('OverlayKeyboardDispatcher', () => {
expect(spy).toHaveBeenCalledTimes(1);
});

it('should dispose of the global keyboard event handler correctly', () => {
const overlayRef = overlay.create();
const body = document.body;

spyOn(body, 'addEventListener');
spyOn(body, 'removeEventListener');

keyboardDispatcher.add(overlayRef);
expect(body.addEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function), true);

overlayRef.dispose();
expect(body.removeEventListener).toHaveBeenCalledWith('keydown', jasmine.any(Function), true);
});

});


Expand Down
57 changes: 25 additions & 32 deletions src/cdk/overlay/keyboard/overlay-keyboard-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@

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

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

private _keydownEventSubscription: Subscription | null;
private _document: Document;
private _isAttached: boolean;

constructor(@Inject(DOCUMENT) private _document: any) {}
constructor(@Inject(DOCUMENT) document: any) {
this._document = document;
}

ngOnDestroy() {
this._unsubscribeFromKeydownEvents();
this._detach();
}

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

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

// Remove the global listener once there are no more overlays.
if (this._attachedOverlays.length === 0) {
this._unsubscribeFromKeydownEvents();
}
}

/**
* Subscribe to keydown events that land on the body and dispatch those
* events to the appropriate overlay.
*/
private _subscribeToKeydownEvents(): void {
const bodyKeydownEvents = fromEvent<KeyboardEvent>(this._document.body, 'keydown', true);

this._keydownEventSubscription = bodyKeydownEvents.pipe(
filter(() => !!this._attachedOverlays.length)
).subscribe(event => {
// Dispatch keydown event to the correct overlay.
this._selectOverlayFromEvent(event)._keydownEvents.next(event);
});
}

/** Removes the global keydown subscription. */
private _unsubscribeFromKeydownEvents(): void {
if (this._keydownEventSubscription) {
this._keydownEventSubscription.unsubscribe();
this._keydownEventSubscription = null;
this._detach();
}
}

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

/** Detaches the global keyboard event listener. */
private _detach() {
if (this._isAttached) {
this._document.body.removeEventListener('keydown', this._keydownListener, true);
this._isAttached = false;
}
}

/** Keyboard event listener that will be attached to the body. */
private _keydownListener = (event: KeyboardEvent) => {
if (this._attachedOverlays.length) {
// Dispatch keydown event to the correct overlay.
this._selectOverlayFromEvent(event)._keydownEvents.next(event);
}
}
}

/** @docs-private */
Expand Down

0 comments on commit ccf7a4d

Please sign in to comment.