Bug 1648868 - Part 1: Cleanup SubDialog for tab-level usage r=emalysz
authorMark Striemer <mstriemer@mozilla.com>
Sat, 01 Aug 2020 15:13:24 +0000
changeset 543012 d7f8623105f9b52e471b9a61cb9814a9a68a5fd8
parent 543011 0ddd8e15aa4ea4da1fcd704d7e0316bc6423da50
child 543013 a09b54a535f4154e2070b60f68888182cf0cd809
push id37658
push usermalexandru@mozilla.com
push dateSat, 01 Aug 2020 21:35:49 +0000
treeherdermozilla-central@4b6ce69368c3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemalysz
bugs1648868
milestone81.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1648868 - Part 1: Cleanup SubDialog for tab-level usage r=emalysz Ensure all listeners that are added are cleaned up correctly. Add an option to remove the overlay when closed and add a promise for when the dialog is set up. Differential Revision: https://phabricator.services.mozilla.com/D85307
toolkit/modules/SubDialog.jsm
--- a/toolkit/modules/SubDialog.jsm
+++ b/toolkit/modules/SubDialog.jsm
@@ -23,31 +23,35 @@ const { Services } = ChromeUtils.import(
  * @param {String}  id - A unique identifier for the dialog.
  * @param {Object}  dialogOptions - Dialog options object.
  * @param {String[]} [dialogOptions.styleSheets] - An array of URLs to additional
  * stylesheets to inject into the frame.
  * @param {Boolean} [consumeOutsideClicks] - Whether to close the dialog when
  * its background overlay is clicked.
  * @param {SubDialog~resizeCallback} [resizeCallback] - Function to be called on
  * dialog resize.
+ * @param {Boolean} [dialogOptions.reuseDialog] - If false, remove the SubDialog
+ * from the DOM when closed. Defaults to true.
  */
 function SubDialog({
   template,
   parentElement,
   id,
   dialogOptions: {
     styleSheets = [],
     consumeOutsideClicks = true,
     resizeCallback,
+    reuseDialog = true,
   } = {},
 }) {
   this._id = id;
 
   this._injectedStyleSheets = this._injectedStyleSheets.concat(styleSheets);
   this._consumeOutsideClicks = consumeOutsideClicks;
+  this._reuseDialog = reuseDialog;
   this._resizeCallback = resizeCallback;
   this._overlay = template.cloneNode(true);
   this._box = this._overlay.querySelector(".dialogBox");
   this._titleBar = this._overlay.querySelector(".dialogTitleBar");
   this._titleElement = this._overlay.querySelector(".dialogTitle");
   this._closeButton = this._overlay.querySelector(".dialogClose");
   this._frame = this._overlay.querySelector(".dialogFrame");
 
@@ -109,16 +113,21 @@ SubDialog.prototype = {
     let contentStylesheet = doc.createProcessingInstruction(
       "xml-stylesheet",
       'href="' + aStylesheetURL + '" type="text/css"'
     );
     doc.insertBefore(contentStylesheet, doc.documentElement);
   },
 
   async open(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
+    // Create a promise so consumers can tell when we're done setting up.
+    this._dialogReady = new Promise(resolve => {
+      this._resolveDialogReady = resolve;
+    });
+
     // Wait until frame is ready to prevent browser crash in tests
     await this._frameCreated;
 
     if (!this._frame.contentWindow) {
       // Given the binding constructor execution is asynchronous, and "load"
       // event can be dispatched before the browser element is shown, the
       // browser binding might not be constructed at this point.  Forcibly
       // construct the frame and construct the binding.
@@ -208,36 +217,41 @@ SubDialog.prototype = {
       })
     );
 
     this._window.setTimeout(() => {
       // Unload the dialog after the event listeners run so that the load of about:blank isn't
       // cancelled by the ESC <key>.
       let onBlankLoad = e => {
         if (this._frame.contentWindow.location.href == "about:blank") {
-          this._frame.removeEventListener("load", onBlankLoad);
+          this._frame.removeEventListener("load", onBlankLoad, true);
           // We're now officially done closing, so update the state to reflect that.
           this._openedURL = null;
           this._isClosing = false;
+
+          if (!this._reuseDialog) {
+            this._overlay.remove();
+          }
+
           this._resolveClosePromise();
         }
       };
 
       // Depending on the context of the frame, we need either a system caller
       // (chrome) or a null principal (content) to load a new URI.
       let triggeringPrincipal;
       if (this._window.isChromeWindow) {
         triggeringPrincipal = this._window.document.nodePrincipal;
       } else {
         triggeringPrincipal = Services.scriptSecurityManager.createNullPrincipal(
           {}
         );
       }
 
-      this._frame.addEventListener("load", onBlankLoad, { capture: true });
+      this._frame.addEventListener("load", onBlankLoad, true);
       this._frame.loadURI("about:blank", {
         triggeringPrincipal,
       });
     }, 0);
   },
 
   handleEvent(aEvent) {
     switch (aEvent.type) {
@@ -361,16 +375,17 @@ SubDialog.prototype = {
     //
     // In that case, we expect them to define a Promise which will delay measuring
     // until the promise is fulfilled.
     if (target.contentDocument.mozSubdialogReady) {
       await target.contentDocument.mozSubdialogReady;
     }
 
     await this.resizeDialog();
+    this._resolveDialogReady();
   },
 
   async resizeDialog() {
     // Do this on load to wait for the CSS to load and apply before calculating the size.
     let docEl = this._frame.contentDocument.documentElement;
 
     // These are deduced from styles which we don't change, so it's safe to get them now:
     let boxHorizontalBorder =
@@ -610,17 +625,17 @@ SubDialog.prototype = {
       chromeEventHandler.addEventListener("DOMTitleChanged", this, true);
     }
 
     // DOMFrameContentLoaded only fires on the top window
     this._window.addEventListener("DOMFrameContentLoaded", this, true);
 
     // Wait for the stylesheets injected during DOMContentLoaded to load before showing the dialog
     // otherwise there is a flicker of the stylesheet applying.
-    this._frame.addEventListener("load", this, { capture: true });
+    this._frame.addEventListener("load", this, true);
 
     chromeEventHandler.addEventListener("unload", this, true);
 
     // Ensure we get <esc> keypresses even if nothing in the subdialog is focusable
     // (happens on OS X when only text inputs and lists are focusable, and
     //  the subdialog only has checkboxes/radiobuttons/buttons)
     this._window.addEventListener("keydown", this, true);
 
@@ -630,17 +645,17 @@ SubDialog.prototype = {
   _removeDialogEventListeners() {
     let chromeEventHandler = this._chromeEventHandler;
     chromeEventHandler.removeEventListener("DOMTitleChanged", this, true);
     chromeEventHandler.removeEventListener("unload", this, true);
 
     this._closeButton?.removeEventListener("command", this);
 
     this._window.removeEventListener("DOMFrameContentLoaded", this, true);
-    this._frame.removeEventListener("load", this);
+    this._frame.removeEventListener("load", this, true);
     this._frame.contentWindow.removeEventListener("dialogclosing", this);
     this._window.removeEventListener("keydown", this, true);
 
     this._overlay.removeEventListener("click", this, true);
 
     if (this._resizeObserver) {
       this._resizeObserver.disconnect();
       this._resizeObserver = null;
@@ -655,17 +670,17 @@ SubDialog.prototype = {
     this._closeButton?.addEventListener("keydown", this);
 
     this._window.addEventListener("focus", this, true);
   },
 
   _untrapFocus() {
     this._frame.contentDocument.removeEventListener("keydown", this, true);
     this._closeButton?.removeEventListener("keydown", this);
-    this._window.removeEventListener("focus", this);
+    this._window.removeEventListener("focus", this, true);
   },
 };
 
 /**
  * Manages multiple SubDialogs in a dialog stack element.
  */
 class SubDialogManager {
   /**