Bug 1141031 - Fix in-content prefs dialogs overflowing. r=jaws, a=sledru
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 16 Apr 2015 19:51:04 +0100
changeset 258539 9117f9af554e
parent 258538 65cf03fc2bc9
child 258540 e4192150f53a
push id4691
push userryanvm@gmail.com
push date2015-04-20 17:16 +0000
treeherdermozilla-beta@72f1b4086067 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, sledru
bugs1141031
milestone38.0
Bug 1141031 - Fix in-content prefs dialogs overflowing. r=jaws, a=sledru
browser/components/preferences/in-content/subdialogs.js
browser/components/preferences/in-content/tests/browser_subdialogs.js
browser/components/preferences/in-content/tests/subdialog.xul
--- a/browser/components/preferences/in-content/subdialogs.js
+++ b/browser/components/preferences/in-content/subdialogs.js
@@ -175,51 +175,75 @@ let gSubDialog = {
     // 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);
 
     let groupBoxBody = document.getAnonymousElementByAttribute(this._box, "class", "groupbox-body");
+    // These are deduced from styles which we don't change, so it's safe to get them now:
     let boxVerticalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingTop);
     let boxHorizontalPadding = 2 * parseFloat(getComputedStyle(groupBoxBody).paddingLeft);
+    let boxHorizontalBorder = 2 * parseFloat(getComputedStyle(this._box).borderLeftWidth);
+    let boxVerticalBorder = 2 * parseFloat(getComputedStyle(this._box).borderTopWidth);
+
+    // The difference between the frame and box shouldn't change, either:
+    let boxRect = this._box.getBoundingClientRect();
+    let frameRect = this._frame.getBoundingClientRect();
+    let frameSizeDifference = (frameRect.top - boxRect.top) + (boxRect.bottom - frameRect.bottom);
+
+    // Then determine and set a bunch of width stuff:
     let frameMinWidth = docEl.style.width || docEl.scrollWidth + "px";
     let frameWidth = docEl.getAttribute("width") ? docEl.getAttribute("width") + "px" :
                      frameMinWidth;
+    this._frame.style.width = frameWidth;
+    this._box.style.minWidth = "calc(" +
+                               (boxHorizontalBorder + boxHorizontalPadding) +
+                               "px + " + frameMinWidth + ")";
+
+    // Now do the same but for the height. We need to do this afterwards because otherwise
+    // XUL assumes we'll optimize for height and gives us "wrong" values which then are no
+    // longer correct after we set the width:
     let frameMinHeight = docEl.style.height || docEl.scrollHeight + "px";
     let frameHeight = docEl.getAttribute("height") ? docEl.getAttribute("height") + "px" :
-                      frameMinHeight;
-    let boxVerticalBorder = 2 * parseFloat(getComputedStyle(this._box).borderTopWidth);
-    let boxHorizontalBorder = 2 * parseFloat(getComputedStyle(this._box).borderLeftWidth);
-
-    let frameRect = this._frame.getBoundingClientRect();
-    let boxRect = this._box.getBoundingClientRect();
-    let frameSizeDifference = (frameRect.top - boxRect.top) + (boxRect.bottom - frameRect.bottom);
+                                                     frameMinHeight;
 
     // Now check if the frame height we calculated is possible at this window size,
     // accounting for titlebar, padding/border and some spacing.
     let maxHeight = window.innerHeight - frameSizeDifference - 30;
-    if (frameHeight > maxHeight) {
-      // If not, we should probably let the dialog scroll:
-      frameHeight = maxHeight;
+    // Do this with a frame height in pixels...
+    let comparisonFrameHeight;
+    if (frameHeight.endsWith("em")) {
+      let fontSize = parseFloat(getComputedStyle(this._frame).fontSize);
+      comparisonFrameHeight = parseFloat(frameHeight, 10) * fontSize;
+    } else if (frameHeight.endsWith("px")) {
+      comparisonFrameHeight = parseFloat(frameHeight, 10);
+    } else {
+      Cu.reportError("This dialog (" + this._frame.contentWindow.location.href + ") " +
+                     "set a height in non-px-non-em units ('" + frameHeight + "'), " +
+                     "which is likely to lead to bad sizing in in-content preferences. " +
+                     "Please consider changing this.");
+      comparisonFrameHeight = parseFloat(frameHeight);
+    }
+
+    if (comparisonFrameHeight > maxHeight) {
+      // If the height is bigger than that of the window, we should let the contents scroll:
+      frameHeight = maxHeight + "px";
+      frameMinHeight = maxHeight + "px";
       let containers = this._frame.contentDocument.querySelectorAll('.largeDialogContainer');
       for (let container of containers) {
         container.classList.add("doScroll");
       }
     }
 
-    this._frame.style.width = frameWidth;
     this._frame.style.height = frameHeight;
     this._box.style.minHeight = "calc(" +
                                 (boxVerticalBorder + groupBoxTitleHeight + boxVerticalPadding) +
                                 "px + " + frameMinHeight + ")";
-    this._box.style.minWidth = "calc(" +
-                               (boxHorizontalBorder + boxHorizontalPadding) +
-                               "px + " + frameMinWidth + ")";
 
     this._overlay.style.visibility = "visible";
     this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
 
     this._resizeObserver = new MutationObserver(this._onResize);
     this._resizeObserver.observe(this._box, {attributes: true});
 
     this._trapFocus();
--- a/browser/components/preferences/in-content/tests/browser_subdialogs.js
+++ b/browser/components/preferences/in-content/tests/browser_subdialogs.js
@@ -153,41 +153,103 @@ let gTests = [{
   desc: "Check that width and height from the sub-dialog are used to size the <browser>",
   run: function* () {
     let deferredClose = Promise.defer();
     let dialogPromise = openAndLoadSubDialog(gDialogURL, null, null,
                                              (aEvent) => dialogClosingCallback(deferredClose, aEvent));
     let dialog = yield dialogPromise;
 
     ise(content.gSubDialog._frame.style.width, "32em", "Width should be set on the frame from the dialog");
-    ise(content.gSubDialog._frame.style.height, "40em", "Height should be set on the frame from the dialog");
+    ise(content.gSubDialog._frame.style.height, "5em", "Height should be set on the frame from the dialog");
 
     content.gSubDialog.close();
     yield deferredClose.promise;
   },
 },
 {
+  desc: "Check that a set width and content causing wrapping still lead to correct scrollHeight-implied height",
+  run: function* () {
+    let deferredClose = Promise.defer();
+    let dialogPromise = openAndLoadSubDialog(gDialogURL, null, null,
+                                             (aEvent) => dialogClosingCallback(deferredClose, aEvent));
+
+    let oldHeight;
+    content.addEventListener("DOMFrameContentLoaded", function frame2Loaded() {
+      content.removeEventListener("DOMFrameContentLoaded", frame2Loaded);
+      let doc = content.gSubDialog._frame.contentDocument;
+      oldHeight = doc.documentElement.scrollHeight;
+      doc.documentElement.style.removeProperty("height");
+      doc.getElementById("desc").textContent = `
+        Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque
+        laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi
+        architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas
+        sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione
+        laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi
+        architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas
+        sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione
+        laudantium, totam rem aperiam, eaque ipsa quae ab illo inventore veritatis et quasi
+        architecto beatae vitae dicta sunt explicabo. Nemo enim ipsam voluptatem quia voluptas
+        sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione
+        voluptatem sequi nesciunt.`
+      doc = null;
+    });
+
+    let dialog = yield dialogPromise;
+
+    ise(content.gSubDialog._frame.style.width, "32em", "Width should be set on the frame from the dialog");
+    let docEl = content.gSubDialog._frame.contentDocument.documentElement;
+    ok(docEl.scrollHeight > oldHeight, "Content height increased (from " + oldHeight + " to " + docEl.scrollHeight + ").");
+    ise(content.gSubDialog._frame.style.height, docEl.scrollHeight + "px", "Height on the frame should be higher now");
+
+    content.gSubDialog.close();
+    yield deferredClose.promise;
+  },
+},
+{
+  desc: "Check that a dialog that is too high gets cut down to size",
+  run: function* () {
+    let deferredClose = Promise.defer();
+    let dialogPromise = openAndLoadSubDialog(gDialogURL, null, null,
+                                             (aEvent) => dialogClosingCallback(deferredClose, aEvent));
+
+    content.addEventListener("DOMFrameContentLoaded", function frame3Loaded() {
+      content.removeEventListener("DOMFrameContentLoaded", frame3Loaded);
+      content.gSubDialog._frame.contentDocument.documentElement.style.height = '100000px';
+    });
+
+    let dialog = yield dialogPromise;
+
+    ise(content.gSubDialog._frame.style.width, "32em", "Width should be set on the frame from the dialog");
+    let newHeight = content.gSubDialog._frame.contentDocument.documentElement.scrollHeight;
+    ok(parseInt(content.gSubDialog._frame.style.height) < window.innerHeight,
+       "Height on the frame should be smaller than window's innerHeight");
+
+    content.gSubDialog.close();
+    yield deferredClose.promise;
+  }
+},
+{
   desc: "Check that scrollWidth and scrollHeight from the sub-dialog are used to size the <browser>",
   run: function* () {
     let deferredClose = Promise.defer();
     let dialogPromise = openAndLoadSubDialog(gDialogURL, null, null,
                                              (aEvent) => dialogClosingCallback(deferredClose, aEvent));
 
     content.addEventListener("DOMFrameContentLoaded", function frameLoaded() {
       content.removeEventListener("DOMFrameContentLoaded", frameLoaded);
-      content.gSubDialog._frame.contentDocument.documentElement.style.height = "";
-      content.gSubDialog._frame.contentDocument.documentElement.style.width = "";
+      content.gSubDialog._frame.contentDocument.documentElement.style.removeProperty("height");
+      content.gSubDialog._frame.contentDocument.documentElement.style.removeProperty("width");
     });
 
     let dialog = yield dialogPromise;
 
     ok(content.gSubDialog._frame.style.width.endsWith("px"),
-       "Width should be set to a px value of the scrollWidth from the dialog");
+       "Width (" + content.gSubDialog._frame.style.width + ") should be set to a px value of the scrollWidth from the dialog");
     ok(content.gSubDialog._frame.style.height.endsWith("px"),
-       "Height should be set to a px value of the scrollHeight from the dialog");
+       "Height (" + content.gSubDialog._frame.style.height + ") should be set to a px value of the scrollHeight from the dialog");
 
     gTeardownAfterClose = true;
     content.gSubDialog.close();
     yield deferredClose.promise;
   },
 }];
 
 function promiseDialogClosing(dialog) {
--- a/browser/components/preferences/in-content/tests/subdialog.xul
+++ b/browser/components/preferences/in-content/tests/subdialog.xul
@@ -2,26 +2,26 @@
 
 <!-- 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: 40em;"
+        title="Sample sub-dialog" style="width: 32em; height: 5em;"
         onload="document.getElementById('textbox').focus();"
         ondialogaccept="acceptSubdialog();">
   <script>
     function acceptSubdialog() {
       window.arguments[0].acceptCount++;
     }
   </script>
 
-  <description>A sample sub-dialog for testing</description>
+  <description id="desc">A sample sub-dialog for testing</description>
 
   <textbox id="textbox" value="Default text" />
 
   <separator class="thin"/>
 
   <button oncommand="close();" icon="close" label="Close" />
 
 </dialog>