From 8e6720b602803b3f82e3a421e689e7de95eab3f6 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 29 Mar 2017 23:30:20 +0200 Subject: [PATCH] fix(dialog): leaking MdDialogContainer references (#2944) * fix(dialog): leaking MdDialogContainer references Fixes an issue that caused the `MdDialogContainer` references to not be cleaned up, and as a result, any of the `MdDialogRef` and `MdDialogConfig` instances as well. The issue seems to come from the fact that a couple of blocks that look like: ``` this._ngZone.onMicrotaskEmpty.first().subscribe(() => { this._elementFocusedBeforeDialogWasOpened = document.activeElement; this._focusTrap.focusFirstTabbableElement(); }); ``` End up being transpiled to: ``` var _this = this; this._ngZone.onMicrotaskEmpty.first().subscribe(function () { _this._elementFocusedBeforeDialogWasOpened = document.activeElement; _this._focusTrap.focusFirstTabbableElement(); }); ``` This seems to cause the browser to retain the `_this` reference after the dialog has been destroyed. Fixes #2876. * chore: add comment about `this` usage in zone --- src/lib/dialog/dialog-container.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/lib/dialog/dialog-container.ts b/src/lib/dialog/dialog-container.ts index 57c1c0850d97..9bd64ca3a2f7 100644 --- a/src/lib/dialog/dialog-container.ts +++ b/src/lib/dialog/dialog-container.ts @@ -118,10 +118,8 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy { // If were to attempt to focus immediately, then the content of the dialog would not yet be // ready in instances where change detection has to run first. To deal with this, we simply // wait for the microtask queue to be empty. - this._ngZone.onMicrotaskEmpty.first().subscribe(() => { - this._elementFocusedBeforeDialogWasOpened = document.activeElement as HTMLElement; - this._focusTrap.focusFirstTabbableElement(); - }); + this._elementFocusedBeforeDialogWasOpened = document.activeElement as HTMLElement; + this._focusTrap.focusFirstTabbableElementWhenReady(); } /** @@ -150,16 +148,17 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy { // the dialog was opened. Wait for the DOM to finish settling before changing the focus so // that it doesn't end up back on the . Also note that we need the extra check, because // IE can set the `activeElement` to null in some cases. - this._ngZone.onMicrotaskEmpty.first().subscribe(() => { - let toFocus = this._elementFocusedBeforeDialogWasOpened as HTMLElement; + let toFocus = this._elementFocusedBeforeDialogWasOpened as HTMLElement; - // We need to check whether the focus method exists at all, because IE seems to throw an - // exception, even if the element is the document.body. + // We shouldn't use `this` inside of the NgZone subscription, because it causes a memory leak. + let animationStream = this._onAnimationStateChange; + + this._ngZone.onMicrotaskEmpty.first().subscribe(() => { if (toFocus && 'focus' in toFocus) { toFocus.focus(); } - this._onAnimationStateChange.complete(); + animationStream.complete(); }); if (this._focusTrap) {