Bug 1674135 - Don't destroy frames from hideDialog() as we rely on printing hidden frames. r=Gijs,preferences-reviewers

Using `visibility` preserves frames of the content inside the dialog,
which we rely on to print the preview `<browser>` element.

This was working before bug 1662336 mostly by chance, because we were
doing an extra clone and that happened to mostly not rely on the cloned
document being rendered.

I'd rather fix it in the front-end (by not trying to print a
`display: none` <browser>) than going back to do a separate clone,
because that can get expensive (specially with fission).

It's not super-clear to me how to best test the "print from system
dialog" case, but ideas certainly welcome.

Differential Revision: https://phabricator.services.mozilla.com/D95501

browser/base/content/browser.css

@@ -1450,6 +1450,12 @@ toolbar[keyNav=true]:not([collapsed=true]):not([customizing=true]) toolbartabsto
   z-index: 2;
 }
 
+.dialogStack.temporarilyHidden {
+  /* For some printing use cases we need to visually hide the dialog before
+   * actually closing it / make it disappear from the frame tree. */
+  visibility: hidden;
+}
+
 .dialogOverlay {
   visibility: hidden;
 }

browser/base/content/test/tabdialogs/browser_tabdialogbox_navigation.js

@@ -133,7 +133,10 @@ add_task(async function test_tabdialogbox_hide() {
     info("Waiting for dialogs to open.");
     await Promise.all(dialogs.map(dialog => dialog._dialogReady));
 
-    is(dialogBoxManager._dialogStack.hidden, false, "Dialog stack is showing");
+    ok(
+      !BrowserTestUtils.is_hidden(dialogBoxManager._dialogStack),
+      "Dialog stack is showing"
+    );
 
     dialogBoxManager.hideDialog(browser);
 
@@ -143,7 +146,10 @@ add_task(async function test_tabdialogbox_hide() {
       "Dialog manager still has two dialogs."
     );
 
-    is(dialogBoxManager._dialogStack.hidden, true, "Dialog stack is hidden");
+    ok(
+      BrowserTestUtils.is_hidden(dialogBoxManager._dialogStack),
+      "Dialog stack is hidden"
+    );
 
     // Navigate to a different page
     BrowserTestUtils.loadURI(browser, "https://example.org");

browser/base/content/test/tabdialogs/browser_tabdialogbox_tab_switch_focus.js

@@ -110,7 +110,10 @@ add_task(async function test_tabdialogbox_tab_switch_hidden() {
   // Hide the top dialog
   dialogBoxManager.hideDialog(browser);
 
-  is(dialogBoxManager._dialogStack.hidden, true, "Dialog stack is hidden");
+  ok(
+    BrowserTestUtils.is_hidden(dialogBoxManager._dialogStack),
+    "Dialog stack is hidden"
+  );
 
   // Switch to first tab
   await BrowserTestUtils.switchTab(gBrowser, tabs[0]);

browser/components/preferences/tests/browser_advanced_update.js

@@ -105,13 +105,19 @@ add_task(async function() {
   mockUpdateManager.register();
 
   // Test the dialog window opens
-  is(dialogOverlay.style.visibility, "", "The dialog should be invisible");
+  ok(
+    BrowserTestUtils.is_hidden(dialogOverlay),
+    "The dialog should be invisible"
+  );
   let promiseSubDialogLoaded = promiseLoadSubDialog(
     "chrome://mozapps/content/update/history.xhtml"
   );
   showBtn.doCommand();
   await promiseSubDialogLoaded;
-  is(dialogOverlay.style.visibility, "visible", "The dialog should be visible");
+  ok(
+    !BrowserTestUtils.is_hidden(dialogOverlay),
+    "The dialog should be visible"
+  );
 
   let dialogFrame = dialogOverlay.querySelector(".dialogFrame");
   let frameDoc = dialogFrame.contentDocument;
@@ -174,7 +180,10 @@ add_task(async function() {
   // Test the dialog window closes
   let closeBtn = dialogOverlay.querySelector(".dialogClose");
   closeBtn.doCommand();
-  is(dialogOverlay.style.visibility, "", "The dialog should be invisible");
+  ok(
+    BrowserTestUtils.is_hidden(dialogOverlay),
+    "The dialog should be invisible"
+  );
 
   mockUpdateManager.unregister();
   gBrowser.removeCurrentTab();

browser/components/preferences/tests/browser_languages_subdialog.js

@@ -11,7 +11,7 @@ add_task(async function() {
     const win = await promiseSubDialogLoaded;
     win.Preferences.forceEnableInstantApply();
     dialogOverlay = content.gSubDialog._topDialog._overlay;
-    is(dialogOverlay.style.visibility, "visible", "The dialog is visible.");
+    ok(!BrowserTestUtils.is_hidden(dialogOverlay), "The dialog is visible.");
     return win;
   }
 
@@ -20,14 +20,14 @@ add_task(async function() {
     closeBtn.doCommand();
   }
 
-  is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
+  ok(BrowserTestUtils.is_hidden(dialogOverlay), "The dialog is invisible.");
   let win = await languagesSubdialogOpened();
   ok(
     win.document.getElementById("spoofEnglish").hidden,
     "The 'Request English' checkbox is hidden."
   );
   closeLanguagesSubdialog();
-  is(dialogOverlay.style.visibility, "", "The dialog is invisible.");
+  ok(BrowserTestUtils.is_hidden(dialogOverlay), "The dialog is invisible.");
 
   await SpecialPowers.pushPrefEnv({
     set: [

browser/components/preferences/tests/siteData/head.js

@@ -160,9 +160,8 @@ function openSiteDataSettingsDialog() {
     dialogLoadPromise,
     dialogInitPromise,
   ]).then(() => {
-    is(
-      dialogOverlay.style.visibility,
-      "visible",
+    ok(
+      is_element_visible(dialogOverlay),
       "The Settings dialog should be visible"
     );
   });
@@ -182,9 +181,8 @@ function promiseSettingsDialogClose() {
           dialogWin.document.documentURI ===
           "chrome://browser/content/preferences/dialogs/siteDataSettings.xhtml"
         ) {
-          isnot(
-            dialogOverlay.style.visibility,
-            "visible",
+          ok(
+            is_element_hidden(dialogOverlay),
             "The Settings dialog should be hidden"
           );
           resolve();

toolkit/components/printing/tests/head.js

@@ -140,6 +140,10 @@ class PrintHelper {
   assertDialogHidden() {
     is(this._dialogs.length, 1, "There is one print dialog");
     ok(BrowserTestUtils.is_hidden(this.dialog._box), "The dialog is hidden");
+    ok(
+      this.dialog._box.getBoundingClientRect().width > 0,
+      "The dialog should still have boxes"
+    );
   }
 
   async assertPrintToFile(file, testFn) {

toolkit/modules/SubDialog.jsm

@@ -377,7 +377,8 @@ SubDialog.prototype = {
     // 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";
+    // Note that this needs to inherit so that hideDialog() works as expected.
+    this._overlay.style.visibility = "inherit";
     this._overlay.style.opacity = "0.01";
 
     // Ensure the document gets an a11y role of dialog.
@@ -450,7 +451,7 @@ SubDialog.prototype = {
         detail: { dialog: this },
       })
     );
-    this._overlay.style.visibility = "visible";
+    this._overlay.style.visibility = "inherit";
     this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
 
     if (this._box.getAttribute("resizable") == "true") {
@@ -885,6 +886,7 @@ class SubDialogManager {
     if (!this._dialogs.length) {
       // When opening the first dialog, show the dialog stack.
       this._dialogStack.hidden = false;
+      this._dialogStack.classList.remove("temporarilyHidden");
       this._topLevelPrevActiveElement = doc.activeElement;
 
       // Mark the top dialog according to the array insertion order.
@@ -924,12 +926,14 @@ class SubDialogManager {
   }
 
   /**
-   * Hides the dialog stack for a specific browser.
+   * Hides the dialog stack for a specific browser, without actually destroying
+   * frames for stuff within it.
+   *
    * @param aBrowser - The browser associated with the tab dialog.
    */
   hideDialog(aBrowser) {
     aBrowser.removeAttribute("tabDialogShowing");
-    this._dialogStack.hidden = true;
+    this._dialogStack.classList.add("temporarilyHidden");
   }
 
   /**
@@ -1004,6 +1008,7 @@ class SubDialogManager {
       this._topDialog._overlay.setAttribute("topmost", true);
       this._topDialog._addDialogEventListeners(false);
       this._dialogStack.hidden = false;
+      this._dialogStack.classList.remove("temporarilyHidden");
     } else {
       // We have closed the last dialog, do cleanup.
       this._topLevelPrevActiveElement.focus();