Bug 1199496 - New bookmarks dialog width is increased at each invocation. r=ttaubert a=ritu
☠☠ backed out by 441f51977790 ☠ ☠
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 02 Sep 2015 16:41:05 +0200
changeset 289130 7ba5523ffb28d1124978af627d921b9c0a956717
parent 289129 ef52c4718d2b394145536b1069bfa39d000ce202
child 289131 84c39fd559177869054b7af18caa4d264ac54cad
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert, ritu
bugs1199496
milestone42.0a2
Bug 1199496 - New bookmarks dialog width is increased at each invocation. r=ttaubert a=ritu
browser/components/places/PlacesUIUtils.jsm
browser/components/places/content/bookmarkProperties.js
browser/components/places/content/editBookmarkOverlay.xul
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -462,31 +462,27 @@ this.PlacesUIUtils = {
    *        Owner window for the new dialog.
    *
    * @see documentation at the top of bookmarkProperties.js
    * @return true if any transaction has been performed, false otherwise.
    */
   showBookmarkDialog:
   function PUIU_showBookmarkDialog(aInfo, aParentWindow) {
     // Preserve size attributes differently based on the fact the dialog has
-    // a folder picker or not.  If the picker is visible, the dialog should
-    // be resizable since it may not show enough content for the folders
-    // hierarchy.
+    // a folder picker or not, since it needs more horizontal space than the
+    // other controls.
     let hasFolderPicker = !("hiddenRows" in aInfo) ||
                           aInfo.hiddenRows.indexOf("folderPicker") == -1;
-    // Use a different chrome url, since this allows to persist different sizes,
-    // based on resizability of the dialog.
+    // Use a different chrome url to persist different sizes.
     let dialogURL = hasFolderPicker ?
                     "chrome://browser/content/places/bookmarkProperties2.xul" :
                     "chrome://browser/content/places/bookmarkProperties.xul";
 
-    let features =
-      "centerscreen,chrome,modal,resizable=" + (hasFolderPicker ? "yes" : "no");
-
-    aParentWindow.openDialog(dialogURL, "",  features, aInfo);
+    let features = "centerscreen,chrome,modal,resizable=yes";
+    aParentWindow.openDialog(dialogURL, "", features, aInfo);
     return ("performed" in aInfo && aInfo.performed);
   },
 
   _getTopBrowserWin: function PUIU__getTopBrowserWin() {
     return RecentWindow.getMostRecentBrowserWindow();
   },
 
   /**
--- a/browser/components/places/content/bookmarkProperties.js
+++ b/browser/components/places/content/bookmarkProperties.js
@@ -67,16 +67,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 
 const BOOKMARK_ITEM = 0;
 const BOOKMARK_FOLDER = 1;
 const LIVEMARK_CONTAINER = 2;
 
 const ACTION_EDIT = 0;
 const ACTION_ADD = 1;
 
+let elementsHeight = new Map();
+
 var BookmarkPropertiesPanel = {
 
   /** UI Text Strings */
   __strings: null,
   get _strings() {
     if (!this.__strings) {
       this.__strings = document.getElementById("stringBundle");
     }
@@ -266,16 +268,53 @@ var BookmarkPropertiesPanel = {
    */
   onDialogLoad: Task.async(function* () {
     this._determineItemInfo();
 
     document.title = this._getDialogTitle();
     var acceptButton = document.documentElement.getButton("accept");
     acceptButton.label = this._getAcceptLabel();
 
+    // Do not use sizeToContent, otherwise, due to bug 90276, the dialog will
+    // grow at every opening.
+    // Since elements can be uncollapsed asynchronously, we must observe their
+    // mutations and resize the dialog using a cached element size.
+    this._height = window.outerHeight;
+    this._mutationObserver = new MutationObserver(mutations => {
+      for (let mutation of mutations) {
+        let target = mutation.target;
+        let id = target.id;
+        if (!/^editBMPanel_.*(Row|Checkbox)$/.test(id))
+          continue;
+
+        let collapsed = target.getAttribute("collapsed") === "true";
+        let wasCollapsed = mutation.oldValue === "true";
+        if (collapsed == wasCollapsed)
+          continue;
+
+        if (collapsed) {
+          this._height -= elementsHeight.get(id);
+          elementsHeight.delete(id);
+        } else {
+          elementsHeight.set(id, target.boxObject.height);
+          this._height += elementsHeight.get(id);
+        }
+        window.resizeTo(window.outerWidth, this._height);
+      }
+    });
+
+    this._mutationObserver.observe(document,
+                                   { subtree: true,
+                                     attributeOldValue: true,
+                                     attributeFilter: ["collapsed"] });
+
+    // Some controls are flexible and we want to update their cached size when
+    // the dialog is resized.
+    window.addEventListener("resize", this);
+
     this._beginBatch();
 
     switch (this._action) {
       case ACTION_EDIT:
         gEditItemOverlay.initPanel({ node: this._node
                                    , hiddenRows: this._hiddenRows });
         acceptButton.disabled = gEditItemOverlay.readOnly;
         break;
@@ -295,81 +334,46 @@ var BookmarkPropertiesPanel = {
 
         // if this is an uri related dialog disable accept button until
         // the user fills an uri value.
         if (this._itemType == BOOKMARK_ITEM)
           acceptButton.disabled = !this._inputIsValid();
         break;
     }
 
-    // Adjust the dialog size to the changes done by initPanel. This is necessary because
-    // initPanel, which shows and hides elements, may run after some async work was done
-    // here - i.e. after the DOM load event was processed.
-    window.sizeToContent();
-
-    // When collapsible elements change their collapsed attribute we must
-    // resize the dialog.
-    // sizeToContent is not usable due to bug 90276, so we'll use resizeTo
-    // instead and cache the element size. See WSucks in the legacy
-    // UI code (addBookmark2.js).
-    if (!this._element("tagsRow").collapsed) {
-      this._element("tagsSelectorRow")
-          .addEventListener("DOMAttrModified", this, false);
-    }
-    if (!this._element("folderRow").collapsed) {
-      this._element("folderTreeRow")
-          .addEventListener("DOMAttrModified", this, false);
-    }
-
     if (!gEditItemOverlay.readOnly) {
       // Listen on uri fields to enable accept button if input is valid
       if (this._itemType == BOOKMARK_ITEM) {
         this._element("locationField")
             .addEventListener("input", this, false);
         if (this._isAddKeywordDialog) {
           this._element("keywordField")
               .addEventListener("input", this, false);
         }
       }
     }
-
-    window.sizeToContent();
   }),
 
   // nsIDOMEventListener
-  _elementsHeight: [],
   handleEvent: function BPP_handleEvent(aEvent) {
     var target = aEvent.target;
     switch (aEvent.type) {
       case "input":
         if (target.id == "editBMPanel_locationField" ||
             target.id == "editBMPanel_keywordField") {
           // Check uri fields to enable accept button if input is valid
           document.documentElement
                   .getButton("accept").disabled = !this._inputIsValid();
         }
         break;
-
-      case "DOMAttrModified":
-        // this is called when collapsing a node, but also its direct children,
-        // we only need to resize when the original node changes.
-        if ((target.id == "editBMPanel_tagsSelectorRow" ||
-             target.id == "editBMPanel_folderTreeRow") &&
-            aEvent.attrName == "collapsed" &&
-            target == aEvent.originalTarget) {
-          var id = target.id;
-          var newHeight = window.outerHeight;
-          if (aEvent.newValue) // is collapsed
-            newHeight -= this._elementsHeight[id];
-          else {
-            this._elementsHeight[id] = target.boxObject.height;
-            newHeight += this._elementsHeight[id];
-          }
-
-          window.resizeTo(window.outerWidth, newHeight);
+      case "resize":
+        for (let [id, oldHeight] of elementsHeight) {
+          let newHeight = document.getElementById(id).boxObject.height;
+          this._height += - oldHeight + newHeight;
+          elementsHeight.set(id, newHeight);
         }
         break;
     }
   },
 
 	// Hack for implementing batched-Undo around the editBookmarkOverlay
 	// instant-apply code. For all the details see the comment above beginBatch
 	// in browser-places.js
@@ -413,22 +417,23 @@ var BookmarkPropertiesPanel = {
   },
 
   _element: function BPP__element(aID) {
     return document.getElementById("editBMPanel_" + aID);
   },
 
   onDialogUnload() {
     // gEditItemOverlay does not exist anymore here, so don't rely on it.
+    this._mutationObserver.disconnect();
+    delete this._mutationObserver;
+
+    window.removeEventListener("resize", this);
+
     // Calling removeEventListener with arguments which do not identify any
     // currently registered EventListener on the EventTarget has no effect.
-    this._element("tagsSelectorRow")
-        .removeEventListener("DOMAttrModified", this, false);
-    this._element("folderTreeRow")
-        .removeEventListener("DOMAttrModified", this, false);
     this._element("locationField")
         .removeEventListener("input", this, false);
   },
 
   onDialogAccept() {
     // We must blur current focused element to save its changes correctly
     document.commandDispatcher.focusedElement.blur();
     // The order here is important! We have to uninit the panel first, otherwise
--- a/browser/components/places/content/editBookmarkOverlay.xul
+++ b/browser/components/places/content/editBookmarkOverlay.xul
@@ -19,36 +19,42 @@
     </hbox>
 
     <grid id="editBookmarkPanelGrid" flex="1">
       <columns id="editBMPanel_columns">
         <column id="editBMPanel_labelColumn" />
         <column flex="1" id="editBMPanel_editColumn" />
       </columns>
       <rows id="editBMPanel_rows">
-        <row align="center" id="editBMPanel_nameRow">
+        <row id="editBMPanel_nameRow"
+             align="center"
+             collapsed="true">
           <label value="&editBookmarkOverlay.name.label;"
                  class="editBMPanel_rowLabel"
                  accesskey="&editBookmarkOverlay.name.accesskey;"
                  control="editBMPanel_namePicker"/>
           <textbox id="editBMPanel_namePicker"
                    onchange="gEditItemOverlay.onNamePickerChange();"/>
         </row>
 
-        <row align="center" id="editBMPanel_locationRow">
+        <row id="editBMPanel_locationRow"
+             align="center"
+             collapsed="true">
           <label value="&editBookmarkOverlay.location.label;"
                  class="editBMPanel_rowLabel"
                  accesskey="&editBookmarkOverlay.location.accesskey;"
                  control="editBMPanel_locationField"/>
           <textbox id="editBMPanel_locationField"
                    class="uri-element"
                    onchange="gEditItemOverlay.onLocationFieldChange();"/>
         </row>
 
-        <row align="center" id="editBMPanel_folderRow">
+        <row id="editBMPanel_folderRow"
+             align="center"
+             collapsed="true">
           <label value="&editBookmarkOverlay.folder.label;"
                  class="editBMPanel_rowLabel"
                  control="editBMPanel_folderMenuList"/>
           <hbox flex="1" align="center">
             <menulist id="editBMPanel_folderMenuList"
                       class="folder-icon"
                       flex="1"
                       oncommand="gEditItemOverlay.onFolderMenuListCommand(event);">
@@ -71,17 +77,19 @@
                     class="expander-down"
                     tooltiptext="&editBookmarkOverlay.foldersExpanderDown.tooltip;"
                     tooltiptextdown="&editBookmarkOverlay.foldersExpanderDown.tooltip;"
                     tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;"
                     oncommand="gEditItemOverlay.toggleFolderTreeVisibility();"/>
           </hbox>
         </row>
 
-        <row id="editBMPanel_folderTreeRow" collapsed="true" flex="1">
+        <row id="editBMPanel_folderTreeRow"
+             collapsed="true"
+             flex="1">
           <spacer/>
           <vbox flex="1">
             <tree id="editBMPanel_folderTree"
                   flex="1"
                   class="placesTree"
                   type="places"
                   height="150"
                   minheight="150"
@@ -98,17 +106,19 @@
               <button label="&editBookmarkOverlay.newFolderButton.label;"
                       id="editBMPanel_newFolderButton"
                       accesskey="&editBookmarkOverlay.newFolderButton.accesskey;"
                       oncommand="gEditItemOverlay.newFolder().catch(Components.utils.reportError);"/>
             </hbox>
           </vbox>
         </row>
 
-        <row align="center" id="editBMPanel_tagsRow">
+        <row id="editBMPanel_tagsRow"
+             align="center"
+             collapsed="true">
           <label value="&editBookmarkOverlay.tags.label;"
                  class="editBMPanel_rowLabel"
                  accesskey="&editBookmarkOverlay.tags.accesskey;"
                  control="editBMPanel_tagsField"/>
           <hbox flex="1" align="center">
             <textbox id="editBMPanel_tagsField"
                      type="autocomplete"
                      class="padded"
@@ -131,41 +141,45 @@
         <row id="editBMPanel_tagsSelectorRow"
              align="center"
              collapsed="true">
           <spacer/>
           <listbox id="editBMPanel_tagsSelector"
                    height="150"/>
         </row>
 
-        <row align="center" id="editBMPanel_keywordRow">
+        <row id="editBMPanel_keywordRow"
+             align="center"
+             collapsed="true">
           <observes element="additionalInfoBroadcaster" attribute="hidden"/>
           <label value="&editBookmarkOverlay.keyword.label;"
                  class="editBMPanel_rowLabel"
                  accesskey="&editBookmarkOverlay.keyword.accesskey;"
                  control="editBMPanel_keywordField"/>
           <textbox id="editBMPanel_keywordField"
                    onchange="gEditItemOverlay.onKeywordFieldChange();"/>
         </row>
 
-        <row id="editBMPanel_descriptionRow">
+        <row id="editBMPanel_descriptionRow"
+             collapsed="true">
           <observes element="additionalInfoBroadcaster" attribute="hidden"/>
           <label value="&editBookmarkOverlay.description.label;"
                  class="editBMPanel_rowLabel"
                  accesskey="&editBookmarkOverlay.description.accesskey;"
                  control="editBMPanel_descriptionField"/>
           <textbox id="editBMPanel_descriptionField"
                    multiline="true"
                    rows="4"
                    onchange="gEditItemOverlay.onDescriptionFieldChange();"/>
         </row>
       </rows>
     </grid>
 
     <checkbox id="editBMPanel_loadInSidebarCheckbox"
+              collapsed="true"
               label="&editBookmarkOverlay.loadInSidebar.label;"
               accesskey="&editBookmarkOverlay.loadInSidebar.accesskey;"
               oncommand="gEditItemOverlay.onLoadInSidebarCheckboxCommand();">
       <observes element="additionalInfoBroadcaster" attribute="hidden"/>
     </checkbox>
 
     <!-- If the ids are changing or additional fields are being added, be sure
          to sync the values in places.js -->