Bug 1130411 - Deal with opening dialogs when the previous dialog is not unloaded yet. r=jaws, a=ritu
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Sat, 02 Apr 2016 20:09:16 +0100
changeset 323842 cac4a48a54ae1fa2da8dac399aaee5a4dafb9808
parent 323841 ca9f88def8bb29ee0726c880f981f241e094d939
child 323843 b60a7bd60e507964e2822ff1a8984cede9273cd3
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, ritu
bugs1130411
milestone47.0a2
Bug 1130411 - Deal with opening dialogs when the previous dialog is not unloaded yet. r=jaws, a=ritu MozReview-Commit-ID: IY0W3UfYTdc
browser/components/preferences/in-content/subdialogs.js
browser/components/preferences/in-content/tests/browser.ini
browser/components/preferences/in-content/tests/browser_subdialogs.js
browser/components/preferences/in-content/tests/subdialog2.xul
--- a/browser/components/preferences/in-content/subdialogs.js
+++ b/browser/components/preferences/in-content/subdialogs.js
@@ -36,16 +36,31 @@ var gSubDialog = {
       'xml-stylesheet',
       'href="' + aStylesheetURL + '" type="text/css"'
     );
     this._frame.contentDocument.insertBefore(contentStylesheet,
                                              this._frame.contentDocument.documentElement);
   },
 
   open: function(aURL, aFeatures = null, aParams = null, aClosingCallback = null) {
+    // If we're already open/opening on this URL, do nothing.
+    if (this._openedURL == aURL && !this._isClosing) {
+      return;
+    }
+    // If we're open on some (other) URL or we're closing, open when closing has finished.
+    if (this._openedURL || this._isClosing) {
+      if (!this._isClosing) {
+        this.close();
+      }
+      let args = Array.from(arguments);
+      this._closingPromise.then(() => {
+        this.open.apply(this, args);
+      });
+      return;
+    }
     this._addDialogEventListeners();
 
     let features = (!!aFeatures ? aFeatures + "," : "") + "resizable,dialog=no,centerscreen";
     let dialog = window.openDialog(aURL, "dialogFrame", features, aParams);
     if (aClosingCallback) {
       this._closingCallback = aClosingCallback.bind(dialog);
     }
 
@@ -53,24 +68,26 @@ var gSubDialog = {
     this._isClosing = false;
     this._openedURL = aURL;
 
     features = features.replace(/,/g, "&");
     let featureParams = new URLSearchParams(features.toLowerCase());
     this._box.setAttribute("resizable", featureParams.has("resizable") &&
                                         featureParams.get("resizable") != "no" &&
                                         featureParams.get("resizable") != "0");
-    return dialog;
   },
 
   close: function(aEvent = null) {
     if (this._isClosing) {
       return;
     }
     this._isClosing = true;
+    this._closingPromise = new Promise(resolve => {
+      this._resolveClosePromise = resolve;
+    });
 
     if (this._closingCallback) {
       try {
         this._closingCallback.call(null, aEvent);
       } catch (ex) {
         Cu.reportError(ex);
       }
       this._closingCallback = null;
@@ -85,16 +102,26 @@ var gSubDialog = {
     this._box.removeAttribute("width");
     this._box.removeAttribute("height");
     this._box.style.removeProperty("min-height");
     this._box.style.removeProperty("min-width");
 
     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);
+          // We're now officially done closing, so update the state to reflect that.
+          delete this._openedURL;
+          this._isClosing = false;
+          this._resolveClosePromise();
+        }
+      };
+      this._frame.addEventListener("load", onBlankLoad);
       this._frame.loadURI("about:blank");
     }, 0);
   },
 
   handleEvent: function(aEvent) {
     switch (aEvent.type) {
       case "command":
         this._frame.contentWindow.close();
@@ -127,18 +154,19 @@ var gSubDialog = {
 
   _onUnload: function(aEvent) {
     if (aEvent.target.location.href == this._openedURL) {
       this._frame.contentWindow.close();
     }
   },
 
   _onContentLoaded: function(aEvent) {
-    if (aEvent.target != this._frame || aEvent.target.contentWindow.location == "about:blank")
+    if (aEvent.target != this._frame || aEvent.target.contentWindow.location == "about:blank") {
       return;
+    }
 
     for (let styleSheetURL of this._injectedStyleSheets) {
       this.injectXMLStylesheet(styleSheetURL);
     }
 
     // Provide the ability for the dialog to know that it is being loaded "in-content".
     this._frame.contentDocument.documentElement.setAttribute("subdialog", "true");
 
@@ -164,18 +192,19 @@ var gSubDialog = {
     // XXX: Hack to make focus during the dialog's load functions work. Make the element visible
     // sooner in DOMContentLoaded but mostly invisible instead of changing visibility just before
     // the dialog's load event.
     this._overlay.style.visibility = "visible";
     this._overlay.style.opacity = "0.01";
   },
 
   _onLoad: function(aEvent) {
-    if (aEvent.target.contentWindow.location == "about:blank")
+    if (aEvent.target.contentWindow.location == "about:blank") {
       return;
+    }
 
     // Do this on load to wait for the CSS to load and apply before calculating the size.
     let docEl = this._frame.contentDocument.documentElement;
 
     let groupBoxTitle = document.getAnonymousElementByAttribute(this._box, "class", "groupbox-title");
     let groupBoxTitleHeight = groupBoxTitle.clientHeight +
                               parseFloat(getComputedStyle(groupBoxTitle).borderBottomWidth);
 
@@ -255,18 +284,19 @@ var gSubDialog = {
     // layout of the frame, but afterward they need to be removed
     // or their presence will restrict the contents of the <browser>
     // from resizing to a smaller size.
     frame.style.removeProperty("width");
     frame.style.removeProperty("height");
 
     let docEl = frame.contentDocument.documentElement;
     let persistedAttributes = docEl.getAttribute("persist");
-    if (!persistedAttributes.contains("width") &&
-        !persistedAttributes.contains("height")) {
+    if (!persistedAttributes ||
+        (!persistedAttributes.contains("width") &&
+         !persistedAttributes.contains("height"))) {
       return;
     }
 
     for (let mutation of mutations) {
       if (mutation.attributeName == "width") {
         docEl.setAttribute("width", docEl.scrollWidth);
       } else if (mutation.attributeName == "height") {
         docEl.setAttribute("height", docEl.scrollHeight);
--- a/browser/components/preferences/in-content/tests/browser.ini
+++ b/browser/components/preferences/in-content/tests/browser.ini
@@ -27,12 +27,14 @@ skip-if = true || !healthreport # Bug 11
 [browser_privacypane_1.js]
 [browser_privacypane_3.js]
 [browser_privacypane_4.js]
 [browser_privacypane_5.js]
 [browser_privacypane_8.js]
 [browser_sanitizeOnShutdown_prefLocked.js]
 [browser_searchsuggestions.js]
 [browser_subdialogs.js]
-support-files = subdialog.xul
+support-files =
+  subdialog.xul
+  subdialog2.xul
 [browser_telemetry.js]
 # Skip this test on Android and B2G as FHR and Telemetry are separate systems there.
 skip-if = !healthreport || !telemetry || (os == 'linux' && debug) || (os == 'android') || (os == 'b2g')
--- a/browser/components/preferences/in-content/tests/browser_subdialogs.js
+++ b/browser/components/preferences/in-content/tests/browser_subdialogs.js
@@ -3,27 +3,28 @@
 
 "use strict";
 
 /**
  * Tests for the sub-dialog infrastructure, not for actual sub-dialog functionality.
  */
 
 const gDialogURL = getRootDirectory(gTestPath) + "subdialog.xul";
+const gDialogURL2 = getRootDirectory(gTestPath) + "subdialog2.xul";
 
-function* open_subdialog_and_test_generic_start_state(browser, domcontentloadedFn) {
+function* open_subdialog_and_test_generic_start_state(browser, domcontentloadedFn, url = gDialogURL) {
   let domcontentloadedFnStr = domcontentloadedFn ?
     "(" + domcontentloadedFn.toString() + ")()" :
     "";
-  return ContentTask.spawn(browser, {gDialogURL, domcontentloadedFnStr}, function*(args) {
-    let {gDialogURL, domcontentloadedFnStr} = args;
+  return ContentTask.spawn(browser, {url, domcontentloadedFnStr}, function*(args) {
+    let {url, domcontentloadedFnStr} = args;
     let rv = { acceptCount: 0 };
     let win = content.window;
     let subdialog = win.gSubDialog;
-    subdialog.open(gDialogURL, null, rv);
+    subdialog.open(url, null, rv);
 
     info("waiting for subdialog DOMFrameContentLoaded");
     yield ContentTaskUtils.waitForEvent(win, "DOMFrameContentLoaded", true);
     let result;
     if (domcontentloadedFnStr) {
       result = eval(domcontentloadedFnStr);
     }
 
@@ -123,16 +124,38 @@ add_task(function* check_canceling_dialo
   yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
 
   info("canceling the dialog");
   yield close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
     function() { content.window.gSubDialog._frame.contentDocument.documentElement.cancelDialog(); },
     "cancel", 0);
 });
 
+add_task(function* check_reopening_dialog() {
+  yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
+  info("opening another dialog which will close the first");
+  yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser, "", gDialogURL2);
+  info("closing as normal");
+  yield close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
+    function() { content.window.gSubDialog._frame.contentDocument.documentElement.acceptDialog(); },
+    "accept", 1);
+});
+
+add_task(function* check_opening_while_closing() {
+  yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
+  info("closing");
+  content.window.gSubDialog.close();
+  info("reopening immediately after calling .close()");
+  yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
+  yield close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
+    function() { content.window.gSubDialog._frame.contentDocument.documentElement.acceptDialog(); },
+    "accept", 1);
+
+});
+
 add_task(function* window_close_on_dialog() {
   yield open_subdialog_and_test_generic_start_state(tab.linkedBrowser);
 
   info("canceling the dialog");
   yield close_subdialog_and_test_generic_end_state(tab.linkedBrowser,
     function() { content.window.gSubDialog._frame.contentWindow.window.close(); },
     null, 0);
 });
copy from browser/components/preferences/in-content/tests/subdialog.xul
copy to browser/components/preferences/in-content/tests/subdialog2.xul
--- a/browser/components/preferences/in-content/tests/subdialog.xul
+++ b/browser/components/preferences/in-content/tests/subdialog2.xul
@@ -2,17 +2,17 @@
 
 <!-- Any copyright is dedicated to the Public Domain.
    - http://creativecommons.org/publicdomain/zero/1.0/ -->
 
 <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 
 <dialog id="subDialog"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
-        title="Sample sub-dialog" style="width: 32em; height: 5em;"
+        title="Sample sub-dialog #2" style="width: 32em; height: 5em;"
         onload="document.getElementById('textbox').focus();"
         ondialogaccept="acceptSubdialog();">
   <script>
     function acceptSubdialog() {
       window.arguments[0].acceptCount++;
     }
   </script>