Skip to content

Commit

Permalink
fix(overlay): events not being unbound from destroyed backdrop (#16268)
Browse files Browse the repository at this point in the history
Fixes the `click` and `transitionend` event handlers not being removed from the backdrop that is being destroyed. Since they're referring to things inside the `OverlayRef`, they could cause some memory to be retained.
  • Loading branch information
crisbeto authored and andrewseguin committed Jun 26, 2019
1 parent 2ab8673 commit 453b1e8
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 4 deletions.
13 changes: 9 additions & 4 deletions src/cdk/overlay/overlay-ref.ts
Expand Up @@ -37,6 +37,7 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
private _positionStrategy: PositionStrategy | undefined;
private _scrollStrategy: ScrollStrategy | undefined;
private _locationChanges: SubscriptionLike = Subscription.EMPTY;
private _backdropClickHandler = (event: MouseEvent) => this._backdropClick.next(event);

/**
* Reference to the parent of the `_host` at the time it was detached. Used to restore
Expand Down Expand Up @@ -390,8 +391,7 @@ export class OverlayRef implements PortalOutlet, OverlayReference {

// Forward backdrop clicks such that the consumer of the overlay can perform whatever
// action desired when such a click occurs (usually closing the overlay).
this._backdropElement.addEventListener('click',
(event: MouseEvent) => this._backdropClick.next(event));
this._backdropElement.addEventListener('click', this._backdropClickHandler);

// Add class to fade-in the backdrop after one frame.
if (typeof requestAnimationFrame !== 'undefined') {
Expand Down Expand Up @@ -431,8 +431,13 @@ export class OverlayRef implements PortalOutlet, OverlayReference {
let timeoutId: number;
let finishDetach = () => {
// It may not be attached to anything in certain cases (e.g. unit tests).
if (backdropToDetach && backdropToDetach.parentNode) {
backdropToDetach.parentNode.removeChild(backdropToDetach);
if (backdropToDetach) {
backdropToDetach.removeEventListener('click', this._backdropClickHandler);
backdropToDetach.removeEventListener('transitionend', finishDetach);

if (backdropToDetach.parentNode) {
backdropToDetach.parentNode.removeChild(backdropToDetach);
}
}

// It is possible that a new portal has been attached to this overlay since we started
Expand Down
24 changes: 24 additions & 0 deletions src/cdk/overlay/overlay.spec.ts
Expand Up @@ -745,6 +745,30 @@ describe('Overlay', () => {
.toBeLessThan(children.indexOf(host), 'Expected backdrop to be before the host in the DOM');
});

it('should remove the event listener from the backdrop', () => {
let overlayRef = overlay.create(config);
overlayRef.attach(componentPortal);

viewContainerFixture.detectChanges();
let backdrop = overlayContainerElement.querySelector('.cdk-overlay-backdrop') as HTMLElement;
expect(backdrop).toBeTruthy();
expect(backdrop.classList).not.toContain('cdk-overlay-backdrop-showing');

let backdropClickHandler = jasmine.createSpy('backdropClickHander');
overlayRef.backdropClick().subscribe(backdropClickHandler);

backdrop.click();
expect(backdropClickHandler).toHaveBeenCalledTimes(1);

overlayRef.detach();
dispatchFakeEvent(backdrop, 'transitionend');
zone.simulateZoneExit();
viewContainerFixture.detectChanges();

backdrop.click();
expect(backdropClickHandler).toHaveBeenCalledTimes(1);
});

});

describe('panelClass', () => {
Expand Down

0 comments on commit 453b1e8

Please sign in to comment.