From b0a9fde9d52758d4aced46d52fdbb42f934b2d38 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 4 Feb 2017 11:15:19 +0100 Subject: [PATCH 1/2] 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. --- src/lib/dialog/dialog-container.ts | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/lib/dialog/dialog-container.ts b/src/lib/dialog/dialog-container.ts index bb54a4d79ac7..f858dc32e20e 100644 --- a/src/lib/dialog/dialog-container.ts +++ b/src/lib/dialog/dialog-container.ts @@ -109,10 +109,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(); } /** @@ -137,16 +135,15 @@ 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; + let animationStream = this._onAnimationStateChange; - // 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. + this._ngZone.onMicrotaskEmpty.first().subscribe(() => { if (toFocus && 'focus' in toFocus) { toFocus.focus(); } - this._onAnimationStateChange.complete(); + animationStream.complete(); }); } } From d1186abff7bf606b9f642df8cf07b29cd92a6f40 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Fri, 10 Mar 2017 20:52:43 +0100 Subject: [PATCH 2/2] chore: add comment about `this` usage in zone --- src/lib/dialog/dialog-container.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/dialog/dialog-container.ts b/src/lib/dialog/dialog-container.ts index f858dc32e20e..88531ff39a23 100644 --- a/src/lib/dialog/dialog-container.ts +++ b/src/lib/dialog/dialog-container.ts @@ -136,6 +136,8 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy { // 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. let toFocus = this._elementFocusedBeforeDialogWasOpened as HTMLElement; + + // 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(() => {