Bug 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner. r=Gijs,Paolo
authorMike de Boer <mdeboer@mozilla.com>
Tue, 30 May 2017 14:21:59 +0200
changeset 409430 db14c0e392d03d81a229bc2e68325f27d6cf0e41
parent 409429 d1deb3611dfceb9ce82e8c295a581e05b466e104
child 409431 9fc7556f538645eed760f3cb37c34a98aa28ed14
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, Paolo
bugs1364738
milestone55.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 1364738 - Fix up multi-line labels inside panelviews that have wrapped around and align elements in the banner. r=Gijs,Paolo I updated, extended and refined Paolo's descriptionHeightWorkaround method to support multi-line toolbar button labels. Made the app menu use that method to ensure no scrollbars appear. Also updated the styling of the banner to have icon and label align correctly with those of the other buttons inside the panelview. MozReview-Commit-ID: IzbahG0kyTu
browser/components/customizableui/PanelMultiView.jsm
browser/components/customizableui/content/panelUI.inc.xul
browser/themes/linux/customizableui/panelUI.css
browser/themes/osx/customizableui/panelUI.css
browser/themes/shared/customizableui/panelUI.inc.css
browser/themes/windows/customizableui/panelUI.css
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -212,16 +212,21 @@ this.PanelMultiView = class {
       this.__currentSubView = panel;
     return panel;
   }
   get _keyNavigationMap() {
     if (!this.__keyNavigationMap)
       this.__keyNavigationMap = new Map();
     return this.__keyNavigationMap;
   }
+  get _multiLineElementsMap() {
+    if (!this.__multiLineElementsMap)
+      this.__multiLineElementsMap = new WeakMap();
+    return this.__multiLineElementsMap;
+  }
 
   constructor(xulNode, testMode = false) {
     this.node = xulNode;
     // If `testMode` is `true`, the consumer is only interested in accessing the
     // methods of this instance. (E.g. in unit tests.)
     if (testMode)
       return;
 
@@ -239,25 +244,25 @@ this.PanelMultiView = class {
       document.getAnonymousElementByAttribute(this.node, "anonid", "mainViewContainer");
     this._subViews =
       document.getAnonymousElementByAttribute(this.node, "anonid", "subViews");
     this._viewStack =
       document.getAnonymousElementByAttribute(this.node, "anonid", "viewStack");
 
     this._panel.addEventListener("popupshowing", this);
     this._panel.addEventListener("popuphidden", this);
+    this._panel.addEventListener("popupshown", this);
     if (this.panelViews) {
       let cs = window.getComputedStyle(document.documentElement);
       // Set CSS-determined attributes now to prevent a layout flush when we do
       // it when transitioning between panels.
       this._dir = cs.direction;
       this.setMainView(this.panelViews.currentView);
       this.showMainView();
     } else {
-      this._panel.addEventListener("popupshown", this);
       this._clickCapturer.addEventListener("click", this);
 
       this._mainViewContainer.setAttribute("panelid", this._panel.id);
 
       if (this._mainView) {
         this.setMainView(this._mainView);
       }
     }
@@ -312,29 +317,30 @@ this.PanelMultiView = class {
    * @param  {panelview} view View to check, defaults to the currently active view.
    * @return {Boolean}
    */
   _canGoBack(view = this._currentSubView) {
     return !!view.getAttribute("title");
   }
 
   setMainView(aNewMainView) {
+    if (this._mainView) {
+      if (!this.panelViews)
+        this._subViews.appendChild(this._mainView);
+      this._mainView.removeAttribute("mainview");
+    }
+    this._mainViewId = aNewMainView.id;
+    aNewMainView.setAttribute("mainview", "true");
     if (this.panelViews) {
       // If the new main view is not yet in the zeroth position, make sure it's
       // inserted there.
       if (aNewMainView.parentNode != this._viewStack && this._viewStack.firstChild != aNewMainView) {
         this._viewStack.insertBefore(aNewMainView, this._viewStack.firstChild);
       }
     } else {
-      if (this._mainView) {
-        this._subViews.appendChild(this._mainView);
-        this._mainView.removeAttribute("mainview");
-      }
-      this._mainViewId = aNewMainView.id;
-      aNewMainView.setAttribute("mainview", "true");
       this._mainViewContainer.appendChild(aNewMainView);
     }
   }
 
   showMainView() {
     if (this.panelViews) {
       this.showSubView(this._mainViewId);
     } else {
@@ -416,17 +422,17 @@ this.PanelMultiView = class {
         let custWidget = CustomizableWidgets.find(widget => widget.viewId == viewNode.id);
         if (custWidget) {
           if (custWidget.onInit)
             custWidget.onInit(aAnchor);
           custWidget.onViewShowing({ target: aAnchor, detail });
         }
       }
       viewNode.setAttribute("current", true);
-      if (playTransition && this.panelViews)
+      if (this.panelViews && this._mainViewWidth)
         viewNode.style.maxWidth = viewNode.style.minWidth = this._mainViewWidth + "px";
 
       let evt = new window.CustomEvent("ViewShowing", { bubbles: true, cancelable: true, detail });
       viewNode.dispatchEvent(evt);
 
       let cancel = evt.defaultPrevented;
       if (detail.blockers.size) {
         try {
@@ -438,38 +444,42 @@ this.PanelMultiView = class {
         }
       }
 
       if (cancel) {
         return;
       }
 
       this._currentSubView = viewNode;
+      if (this.panelViews) {
+        this.node.setAttribute("viewtype", "subview");
+        if (!playTransition)
+          this.descriptionHeightWorkaround(viewNode);
+      }
 
       // Now we have to transition the panel. There are a few parts to this:
       //
       // 1) The main view content gets shifted so that the center of the anchor
       //    node is at the left-most edge of the panel.
       // 2) The subview deck slides in so that it takes up almost all of the
       //    panel.
       // 3) If the subview is taller then the main panel contents, then the panel
       //    must grow to meet that new height. Otherwise, it must shrink.
       //
       // All three of these actions make use of CSS transformations, so they
       // should all occur simultaneously.
       if (this.panelViews && playTransition) {
-        this.node.setAttribute("viewtype", "subview");
-
         // Sliding the next subview in means that the previous panelview stays
         // where it is and the active panelview slides in from the left in LTR
         // mode, right in RTL mode.
         let onTransitionEnd = () => {
           evt = new window.CustomEvent("ViewHiding", { bubbles: true, cancelable: true });
           previousViewNode.dispatchEvent(evt);
           previousViewNode.removeAttribute("current");
+          this.descriptionHeightWorkaround(viewNode);
         };
 
         // There's absolutely no need to show off our epic animation skillz when
         // the panel's not even open.
         if (this._panel.state != "open") {
           onTransitionEnd();
           return;
         }
@@ -550,17 +560,17 @@ this.PanelMultiView = class {
 
             // Myeah, panel layout auto-resizing is a funky thing. We'll wait
             // another few milliseconds to remove the width and height 'fixtures',
             // to be sure we don't flicker annoyingly.
             // NB: HACK! Bug 1363756 is there to fix this.
             window.setTimeout(() => {
               // Only remove the height when the view is larger than the main
               // view, otherwise it'll snap back to its own height.
-              if (viewRect.height > this._mainViewHeight)
+              if (viewNode == this._mainView || viewRect.height > this._mainViewHeight)
                 this._viewContainer.style.removeProperty("height");
               this._viewContainer.style.removeProperty("width");
             }, 500);
 
             // Take another breather, just like before, to wait for the 'current'
             // attribute removal to take effect. This prevents a flicker.
             // The cleanup we do doesn't affect the display anymore, so we're not
             // too fussed about the timing here.
@@ -730,42 +740,45 @@ this.PanelMultiView = class {
           this.window.addEventListener("keydown", this);
           this._panel.addEventListener("mousemove", this);
         }
         break;
       case "popupshown":
         // Now that the main view is visible, we can check the height of the
         // description elements it contains.
         this.descriptionHeightWorkaround();
-        // Now that the panel has opened, we can compute the distance from its
-        // anchor to the available margin of the screen, based on whether the
-        // panel actually opened towards the top or the bottom. We use this to
-        // limit its maximum height, which is relevant when opening a subview.
-        let maxHeight;
-        if (this._panel.alignmentPosition.startsWith("before_")) {
-          maxHeight = this._panel.getOuterScreenRect().bottom -
-                      this.window.screen.availTop;
-        } else {
-          maxHeight = this.window.screen.availTop +
-                      this.window.screen.availHeight -
-                      this._panel.getOuterScreenRect().top;
+
+        if (!this.panelViews) {
+          // Now that the panel has opened, we can compute the distance from its
+          // anchor to the available margin of the screen, based on whether the
+          // panel actually opened towards the top or the bottom. We use this to
+          // limit its maximum height, which is relevant when opening a subview.
+          let maxHeight;
+          if (this._panel.alignmentPosition.startsWith("before_")) {
+            maxHeight = this._panel.getOuterScreenRect().bottom -
+                        this.window.screen.availTop;
+          } else {
+            maxHeight = this.window.screen.availTop +
+                        this.window.screen.availHeight -
+                        this._panel.getOuterScreenRect().top;
+          }
+          // To go from the maximum height of the panel to the maximum height of
+          // the view stack, we start by subtracting the height of the arrow box.
+          // We don't need to trigger a new layout because this does not change.
+          let arrowBox = this.document.getAnonymousElementByAttribute(
+                                          this._panel, "anonid", "arrowbox");
+          maxHeight -= this._dwu.getBoundsWithoutFlushing(arrowBox).height;
+          // We subtract a fixed margin to account for variable borders. We don't
+          // try to measure this accurately so it's faster, we don't depend on
+          // the arrowpanel structure, and we don't hit rounding errors. Instead,
+          // we use a value that is much greater than the typical borders and
+          // makes sense visually.
+          const EXTRA_MARGIN_PX = 8;
+          this._viewStack.style.maxHeight = (maxHeight - EXTRA_MARGIN_PX) + "px";
         }
-        // To go from the maximum height of the panel to the maximum height of
-        // the view stack, we start by subtracting the height of the arrow box.
-        // We don't need to trigger a new layout because this does not change.
-        let arrowBox = this.document.getAnonymousElementByAttribute(
-                                        this._panel, "anonid", "arrowbox");
-        maxHeight -= this._dwu.getBoundsWithoutFlushing(arrowBox).height;
-        // We subtract a fixed margin to account for variable borders. We don't
-        // try to measure this accurately so it's faster, we don't depend on
-        // the arrowpanel structure, and we don't hit rounding errors. Instead,
-        // we use a value that is much greater than the typical borders and
-        // makes sense visually.
-        const EXTRA_MARGIN_PX = 8;
-        this._viewStack.style.maxHeight = (maxHeight - EXTRA_MARGIN_PX) + "px";
         break;
       case "popuphidden":
         this.node.removeAttribute("panelopen");
         this.showMainView();
         if (this.panelViews) {
           this.window.removeEventListener("keydown", this);
           this._panel.removeEventListener("mousemove", this);
           this._resetKeyNavigation();
@@ -911,49 +924,61 @@ this.PanelMultiView = class {
       let bounds = dwu.getBoundsWithoutFlushing(button);
       return bounds.width > 0 && bounds.height > 0;
     });
   }
 
   /**
    * If the main view or a subview contains wrapping elements, the attribute
    * "descriptionheightworkaround" should be set on the view to force all the
-   * "description" elements to a fixed height. If the attribute is set and the
-   * visibility, contents, or width of any of these elements changes, this
-   * function should be called to refresh the calculated heights.
+   * "description" or wrapping toolbarbutton elements to a fixed height.
+   * If the attribute is set and the visibility, contents, or width of any of
+   * these elements changes, this function should be called to refresh the
+   * calculated heights.
    *
-   * @note While both "label" and "description" elements may contain wrapping
-   *       text, only "description" elements are used that way in panels.
+   * This may trigger a synchronous layout.
    *
    * @param viewNode
    *        Indicates the node to scan for descendant elements. This is the main
    *        view if omitted.
    */
   descriptionHeightWorkaround(viewNode = this._mainView) {
     if (!this.node.hasAttribute("descriptionheightworkaround")) {
       // This view does not require the workaround.
       return;
     }
 
     // We batch DOM changes together in order to reduce synchronous layouts.
     // First we reset any change we may have made previously. The first time
     // this is called, and in the best case scenario, this has no effect.
     let items = [];
-    for (let element of viewNode.getElementsByTagName("description")) {
+    for (let element of viewNode.querySelectorAll(
+         "description:not([hidden]):not([value]),toolbarbutton[wrap]:not([hidden])")) {
+      // Take the label for toolbarbuttons; it only exists on those elements.
+      element = element.labelElement || element;
+
+      let bounds = this._dwu.getBoundsWithoutFlushing(element);
+      let previous = this._multiLineElementsMap.get(element);
+      // Only remove the 'height' property, which will cause a layout flush, when
+      // absolutely necessary.
+      if (!bounds.width || !bounds.height ||
+          (previous && element.textContent == previous.textContent &&
+                       bounds.width == previous.bounds.width)) {
+        continue;
+      }
+
       element.style.removeProperty("height");
       items.push({ element });
     }
+
     // We now read the computed style to store the height of any element that
     // may contain wrapping text, which will be zero if the element is hidden.
-    // This might trigger a synchronous layout, but if this function was called
-    // from a _transitionHeight callback and there are no description elements
-    // visible, then _transitionHeight will not trigger a layout again.
     for (let item of items) {
-      item.height = item.element.getBoundingClientRect().height;
+      item.bounds = item.element.getBoundingClientRect();
     }
+
     // Now we can make all the necessary DOM changes at once.
-    for (let item of items) {
-      if (item.height) {
-        item.element.style.height = item.height + "px";
-      }
+    for (let { element, bounds } of items) {
+      this._multiLineElementsMap.set(element, { bounds, textContent: element.textContent });
+      element.style.height = bounds.height + "px";
     }
   }
 }
--- a/browser/components/customizableui/content/panelUI.inc.xul
+++ b/browser/components/customizableui/content/panelUI.inc.xul
@@ -498,17 +498,17 @@
 <panel id="appMenu-popup"
        class="cui-widget-panel"
        role="group"
        type="arrow"
        hidden="true"
        flip="slide"
        position="bottomcenter topright"
        noautofocus="true">
-  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView">
+  <photonpanelmultiview id="appMenu-multiView" mainViewId="appMenu-mainView" descriptionheightworkaround="true">
     <panelview id="appMenu-mainView" class="PanelUI-subView">
       <vbox class="panel-subview-body">
         <vbox id="appMenu-addon-banners"/>
         <toolbarbutton class="panel-banner-item"
                        label-update-available="&updateAvailable.panelUI.label;"
                        label-update-manual="&updateManual.panelUI.label;"
                        label-update-restart="&updateRestart.panelUI.label2;"
                        oncommand="PanelUI._onBannerItemSelected(event)"
--- a/browser/themes/linux/customizableui/panelUI.css
+++ b/browser/themes/linux/customizableui/panelUI.css
@@ -95,14 +95,15 @@ menu.subviewbutton > .menu-right:-moz-lo
 
 .subviewradio > .radio-label-box {
   -moz-appearance: none;
 }
 
 /* START photon adjustments */
 
 photonpanelmultiview .subviewbutton > .toolbarbutton-text,
-photonpanelmultiview .subviewbutton > .toolbarbutton-icon {
+photonpanelmultiview .subviewbutton > .toolbarbutton-icon,
+photonpanelmultiview .panel-banner-item > .toolbarbutton-multiline-text {
   padding: 0;
   margin: 0;
 }
 
 /* END photon adjustments */
--- a/browser/themes/osx/customizableui/panelUI.css
+++ b/browser/themes/osx/customizableui/panelUI.css
@@ -20,17 +20,18 @@
 
 .subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) > .toolbarbutton-text {
   margin: 2px 6px !important; /* !important for overriding toolbarbutton.css */
 }
 
 /* START photon adjustments */
 
 photonpanelmultiview .subviewbutton > .toolbarbutton-text,
-photonpanelmultiview .subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) > .toolbarbutton-text {
+photonpanelmultiview .subviewbutton:-moz-any([image],[targetURI],.cui-withicon, .bookmark-item) > .toolbarbutton-text,
+photonpanelmultiview .panel-banner-item > .toolbarbutton-multiline-text {
   margin: 0 !important;  /* !important for overriding the rules above. */
 }
 
 /* END photon adjustments */
 
 .restoreallitem > .toolbarbutton-icon {
   display: none;
 }
--- a/browser/themes/shared/customizableui/panelUI.inc.css
+++ b/browser/themes/shared/customizableui/panelUI.inc.css
@@ -1279,17 +1279,18 @@ photonpanelmultiview .PanelUI-subView .s
   padding: 4px 12px;
 }
 
 photonpanelmultiview .subviewbutton:focus {
   outline: 0;
 }
 
 photonpanelmultiview .subviewbutton-iconic:not(.subviewbutton-back) > .toolbarbutton-text,
-photonpanelmultiview .subviewbutton[checked="true"] > .toolbarbutton-text {
+photonpanelmultiview .subviewbutton[checked="true"] > .toolbarbutton-text,
+photonpanelmultiview .panel-banner-item > .toolbarbutton-multiline-text {
   padding-inline-start: 8px; /* See '.subviewbutton-iconic > .toolbarbutton-text' rule above. */
 }
 
 photonpanelmultiview .subviewbutton:not(.subviewbutton-iconic):not([checked="true"]) > .toolbarbutton-text {
   padding-inline-start: 24px; /* This is 16px for the icon + 8px for the padding as defined above. */
 }
 
 photonpanelmultiview .PanelUI-subView .panel-subview-footer {
@@ -1332,16 +1333,21 @@ photonpanelmultiview .PanelUI-subView .t
   min-width: auto;
   padding: 4px;
 }
 
 photonpanelmultiview .PanelUI-subView .toolbaritem-combined-buttons > .subviewbutton > .toolbarbutton-text {
   display: none;
 }
 
+photonpanelmultiview .panel-banner-item::after {
+  margin-inline-end: 14px;
+  margin-inline-start: 10px;
+}
+
 /* END photon adjustments */
 
 panelview .toolbarbutton-1,
 .widget-overflow-list > .toolbarbutton-1:not(:first-child),
 .widget-overflow-list > toolbaritem:not(:first-child) {
   margin-top: 6px;
 }
 
--- a/browser/themes/windows/customizableui/panelUI.css
+++ b/browser/themes/windows/customizableui/panelUI.css
@@ -143,14 +143,15 @@ menu.subviewbutton > .menu-right:-moz-lo
   #zoom-controls@inAnyPanel@ > toolbarbutton {
     border-radius: 0;
   }
 }
 
 /* START photon adjustments */
 
 photonpanelmultiview .subviewbutton > .toolbarbutton-text,
-photonpanelmultiview .subviewbutton > .toolbarbutton-icon {
+photonpanelmultiview .subviewbutton > .toolbarbutton-icon,
+photonpanelmultiview .panel-banner-item > .toolbarbutton-multiline-text {
   padding: 0;
   margin: 0;
 }
 
 /* END photon adjustments */