fix(dialog): leaking MdDialogContainer references (#2944)

crisbeto · tinayuangao · commit 8e6720b60280 · 2017-03-30T05:30:20.000+08:00
* 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
diff --git 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 <body>. 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) {
