Bug 1366813 - add a flexible space item in customize mode in photon, r=mossop
☠☠ backed out by e336727983ae ☠ ☠
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 10 Jul 2017 23:34:12 +0100
changeset 368273 051284a3564e0a798fea06a7dde26300ac2288a6
parent 368272 064f2ad7ca3bcd37df0ca60dd870a722ca96c756
child 368274 85332a69ae0cbad6556541883960c517b1b86a03
push id46264
push usergijskruitbosch@gmail.com
push dateTue, 11 Jul 2017 17:23:24 +0000
treeherderautoland@051284a3564e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmossop
bugs1366813
milestone56.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 1366813 - add a flexible space item in customize mode in photon, r=mossop MozReview-Commit-ID: 4k4VdBa4HCt
browser/base/content/browser.css
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/CustomizeMode.jsm
browser/components/customizableui/test/browser_1003588_no_specials_in_panel.js
browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
browser/themes/shared/customizableui/customizeMode.inc.css
browser/themes/shared/customizableui/panelUI.inc.css
--- a/browser/base/content/browser.css
+++ b/browser/base/content/browser.css
@@ -263,16 +263,20 @@ window:not([chromehidden~="toolbar"]) #n
 #titlebar-buttonbox {
   -moz-appearance: -moz-window-button-box;
 }
 
 #personal-bookmarks {
   -moz-window-dragging: inherit;
 }
 
+toolbarpaletteitem {
+  -moz-window-dragging: no-drag;
+}
+
 %ifdef XP_MACOSX
 #titlebar-fullscreen-button {
   -moz-appearance: -moz-mac-fullscreen-button;
 }
 
 /* Fullscreen and caption buttons don't move with RTL on OS X so override the automatic ordering. */
 #titlebar-secondary-buttonbox:-moz-locale-dir(ltr),
 #titlebar-buttonbox-container:-moz-locale-dir(rtl),
@@ -743,16 +747,28 @@ html|input.urlbar-input[textoverflow]:no
 #identity-box.verifiedIdentity > #identity-icon-labels > #identity-icon-label {
   margin-inline-end: 0.25em !important;
 }
 
 #main-window[customizing] :-moz-any(#urlbar, .searchbar-textbox) > .autocomplete-textbox-container > .textbox-input-box {
   visibility: hidden;
 }
 
+/* Flexible spacer sizing (matching url bar) */
+toolbarpaletteitem[place=toolbar][id^=wrapper-customizableui-special-spring],
+toolbarspring {
+  -moz-box-flex: 1;
+  min-width: 40px;
+}
+
+#nav-bar toolbarpaletteitem[id^=wrapper-customizableui-special-spring],
+#nav-bar toolbarspring {
+  -moz-box-flex: 80;
+}
+
 /* ::::: Unified Back-/Forward Button ::::: */
 .unified-nav-current {
   font-weight: bold;
 }
 
 .bookmark-item > label {
   /* ensure we use the direction of the bookmarks label instead of the
    * browser locale */
@@ -1354,16 +1370,17 @@ toolbarpaletteitem[place="palette"] {
 
 #main-window[photon-structure] toolbarpaletteitem[place="palette"] {
   -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem-palette-wrapping-label");
   width: 7em;
   /* icon (16) + margin (9 + 12) + 3 lines of text: */
   height: calc(39px + 3em);
   margin-inline-end: 24px;
   overflow: visible;
+  vertical-align: top;
 }
 
 toolbarpaletteitem[place="palette"][hidden] {
   display: none;
 }
 
 #customization-palette .toolbarpaletteitem-box {
   -moz-box-pack: center;
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -514,16 +514,18 @@ var CustomizableUIInternal = {
       if (!widget.wrapper) {
         widget.wrapper = new WidgetGroupWrapper(widget);
         gGroupWrapperCache.set(aWidgetId, widget.wrapper);
       }
       return widget.wrapper;
     }
 
     // PROVIDER_SPECIAL gets treated the same as PROVIDER_XUL.
+    // XXXgijs: this causes bugs in code that depends on widgetWrapper.provider
+    // giving an accurate answer... filed as bug 1379821
     let wrapper = new XULWidgetGroupWrapper(aWidgetId);
     gGroupWrapperCache.set(aWidgetId, wrapper);
     return wrapper;
   },
 
   registerArea(aName, aProperties, aInternalCaller) {
     if (typeof aName != "string" || !/^[a-z0-9-_]{1,}$/i.test(aName)) {
       throw new Error("Invalid area name");
@@ -1286,19 +1288,16 @@ var CustomizableUIInternal = {
     }
     return aId;
   },
 
   createSpecialWidget(aId, aDocument) {
     let nodeName = "toolbar" + aId.match(/spring|spacer|separator/)[0];
     let node = aDocument.createElementNS(kNSXUL, nodeName);
     node.id = this.ensureSpecialWidgetId(aId);
-    if (nodeName == "toolbarspring") {
-      node.flex = 1;
-    }
     return node;
   },
 
   /* Find a XUL-provided widget in a window. Don't try to use this
    * for an API-provided widget or a special widget.
    */
   findWidgetInWindow(aId, aWindow) {
     if (!gBuildWindows.has(aWindow)) {
@@ -2726,28 +2725,28 @@ var CustomizableUIInternal = {
     // Otherwise this is either a special widget, which is always removable, or
     // an API widget which has already been removed from gPalette. Returning true
     // here allows us to then remove its ID from any placements where it might
     // still occur.
     return true;
   },
 
   canWidgetMoveToArea(aWidgetId, aArea) {
+    // Special widgets can't move to the menu panel.
+    if (this.isSpecialWidget(aWidgetId) && gAreas.has(aArea) &&
+        gAreas.get(aArea).get("type") == CustomizableUI.TYPE_MENU_PANEL) {
+      return false;
+    }
     let placement = this.getPlacementOfWidget(aWidgetId);
-    if (placement && placement.area != aArea) {
-      // Special widgets can't move to the menu panel.
-      if (this.isSpecialWidget(aWidgetId) && gAreas.has(aArea) &&
-          gAreas.get(aArea).get("type") == CustomizableUI.TYPE_MENU_PANEL) {
-        return false;
-      }
-      // For everything else, just return whether the widget is removable.
-      return this.isWidgetRemovable(aWidgetId);
-    }
-
-    return true;
+    // Items in the palette can move, and items can move within their area:
+    if (!placement || placement.area == aArea) {
+      return true;
+    }
+    // For everything else, just return whether the widget is removable.
+    return this.isWidgetRemovable(aWidgetId);
   },
 
   ensureWidgetPlacedInWindow(aWidgetId, aWindow) {
     let placement = this.getPlacementOfWidget(aWidgetId);
     if (!placement) {
       return false;
     }
     let areaNodes = gBuildAreas.get(placement.area);
@@ -3810,16 +3809,25 @@ this.CustomizableUI = {
 
   /**
    * Check if a toolbar is builtin or not.
    * @param aToolbarId the ID of the toolbar you want to check
    */
   isBuiltinToolbar(aToolbarId) {
     return CustomizableUIInternal._builtinToolbars.has(aToolbarId);
   },
+
+  /**
+   * Create an instance of a spring, spacer or separator.
+   * @param aId       the type of special widget (spring, spacer or separator)
+   * @param aDocument the document in which to create it.
+   */
+  createSpecialWidget(aId, aDocument) {
+    return CustomizableUIInternal.createSpecialWidget(aId, aDocument);
+  },
 };
 Object.freeze(this.CustomizableUI);
 Object.freeze(this.CustomizableUI.windows);
 
 /**
  * All external consumers of widgets are really interacting with these wrappers
  * which provide a common interface.
  */
--- a/browser/components/customizableui/CustomizeMode.jsm
+++ b/browser/components/customizableui/CustomizeMode.jsm
@@ -30,16 +30,20 @@ Cu.import("resource://gre/modules/AppCon
 XPCOMUtils.defineLazyModuleGetter(this, "DragPositionManager",
                                   "resource:///modules/DragPositionManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BrowserUITelemetry",
                                   "resource:///modules/BrowserUITelemetry.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
                                   "resource://gre/modules/LightweightThemeManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
                                   "resource:///modules/sessionstore/SessionStore.jsm");
+XPCOMUtils.defineLazyGetter(this, "gWidgetsBundle", function() {
+  const kUrl = "chrome://browser/locale/customizableui/customizableWidgets.properties";
+  return Services.strings.createBundle(kUrl);
+});
 
 XPCOMUtils.defineLazyPreferenceGetter(this, "gPhotonStructure",
   "browser.photon.structure.enabled", false);
 
 let gDebug;
 XPCOMUtils.defineLazyGetter(this, "log", () => {
   let scope = {};
   Cu.import("resource://gre/modules/Console.jsm", scope);
@@ -838,16 +842,21 @@ CustomizeMode.prototype = {
       for (let widget of unusedWidgets) {
         let paletteItem = this.makePaletteItem(widget, "palette");
         if (!paletteItem) {
           continue;
         }
         fragment.appendChild(paletteItem);
       }
 
+      if (gPhotonStructure) {
+        let flexSpace = CustomizableUI.createSpecialWidget("spring", this.document);
+        fragment.appendChild(this.wrapToolbarItem(flexSpace, "palette"));
+      }
+
       this.visiblePalette.appendChild(fragment);
       this._stowedPalette = this.window.gNavToolbox.palette;
       this.window.gNavToolbox.palette = this.visiblePalette;
     } catch (ex) {
       log.error(ex);
     }
   },
 
@@ -873,30 +882,28 @@ CustomizeMode.prototype = {
 
   depopulatePalette() {
     return (async () => {
       this.visiblePalette.hidden = true;
       let paletteChild = this.visiblePalette.firstChild;
       let nextChild;
       while (paletteChild) {
         nextChild = paletteChild.nextElementSibling;
-        let provider = CustomizableUI.getWidget(paletteChild.id).provider;
-        if (provider == CustomizableUI.PROVIDER_XUL) {
+        let itemId = paletteChild.firstChild.id;
+        if (CustomizableUI.isSpecialWidget(itemId)) {
+          this.visiblePalette.removeChild(paletteChild);
+        } else {
+          // XXXunf Currently this doesn't destroy the (now unused) node in the
+          //       API provider case. It would be good to do so, but we need to
+          //       keep strong refs to it in CustomizableUI (can't iterate of
+          //       WeakMaps), and there's the question of what behavior
+          //       wrappers should have if consumers keep hold of them.
           let unwrappedPaletteItem =
             await this.deferredUnwrapToolbarItem(paletteChild);
           this._stowedPalette.appendChild(unwrappedPaletteItem);
-        } else if (provider == CustomizableUI.PROVIDER_API) {
-          // XXXunf Currently this doesn't destroy the (now unused) node. It would
-          //       be good to do so, but we need to keep strong refs to it in
-          //       CustomizableUI (can't iterate of WeakMaps), and there's the
-          //       question of what behavior wrappers should have if consumers
-          //       keep hold of them.
-          // widget.destroyInstance(widgetNode);
-        } else if (provider == CustomizableUI.PROVIDER_SPECIAL) {
-          this.visiblePalette.removeChild(paletteChild);
         }
 
         paletteChild = nextChild;
       }
       this.visiblePalette.hidden = false;
       this.window.gNavToolbox.palette = this._stowedPalette;
     })().catch(log.error);
   },
@@ -1022,16 +1029,20 @@ CustomizeMode.prototype = {
     }
 
     // Only add listeners for newly created wrappers:
     if (!aIsUpdate) {
       wrapper.addEventListener("mousedown", this);
       wrapper.addEventListener("mouseup", this);
     }
 
+    if (CustomizableUI.isSpecialWidget(aNode.id)) {
+      wrapper.setAttribute("title", gWidgetsBundle.GetStringFromName(aNode.nodeName + ".label"));
+    }
+
     return wrapper;
   },
 
   deferredUnwrapToolbarItem(aWrapper) {
     return new Promise(resolve => {
       dispatchFunction(() => {
         let item = null;
         try {
@@ -2054,16 +2065,21 @@ CustomizeMode.prototype = {
                             itemForPlacement.id;
       placement = CustomizableUI.getPlacementOfWidget(targetNodeId);
     }
     if (!placement) {
       log.debug("Could not get a position for " + aTargetNode.nodeName + "#" + aTargetNode.id + "." + aTargetNode.className);
     }
     let position = placement ? placement.position : null;
 
+    // Force creating a new spacer/spring/separator if dragging from the palette
+    if (CustomizableUI.isSpecialWidget(aDraggedItemId) && aOriginArea.id == kPaletteId) {
+      aDraggedItemId = aDraggedItemId.match(/^customizableui-special-(spring|spacer|separator)/)[1];
+    }
+
     // Is the target area the same as the origin? Since we've already handled
     // the possibility that the target is the customization palette, we know
     // that the widget is moving within a customizable area.
     if (aTargetArea == aOriginArea) {
       CustomizableUI.moveWidgetWithinArea(aDraggedItemId, position);
     } else {
       CustomizableUI.addWidgetToArea(aDraggedItemId, aTargetArea.id, position);
     }
--- a/browser/components/customizableui/test/browser_1003588_no_specials_in_panel.js
+++ b/browser/components/customizableui/test/browser_1003588_no_specials_in_panel.js
@@ -56,19 +56,18 @@ add_task(async function checkAddingToToo
   let oldNumberOfItems = previousPlacements.length;
   if (getAreaWidgetIds(area).length != oldNumberOfItems) {
     CustomizableUI.reset();
   }
 });
 
 
 add_task(async function checkDragging() {
-  await SpecialPowers.pushPrefEnv({set: [["browser.photon.structure.enabled", false]]});
   let startArea = CustomizableUI.AREA_NAVBAR;
-  let targetArea = CustomizableUI.AREA_PANEL;
+  let targetArea = gPhotonStructure ? CustomizableUI.AREA_FIXED_OVERFLOW_PANEL : CustomizableUI.AREA_PANEL;
   let startingToolbarPlacements = getAreaWidgetIds(startArea);
   let startingTargetPlacements = getAreaWidgetIds(targetArea);
 
   CustomizableUI.addWidgetToArea("separator", startArea);
   CustomizableUI.addWidgetToArea("spring", startArea);
   CustomizableUI.addWidgetToArea("spacer", startArea);
 
   let placementsWithSpecials = getAreaWidgetIds(startArea);
@@ -76,32 +75,39 @@ add_task(async function checkDragging() 
   for (let id of placementsWithSpecials) {
     if (CustomizableUI.isSpecialWidget(id)) {
       elementsToMove.push(id);
     }
   }
   is(elementsToMove.length, 3, "Should have 3 elements to try and drag.");
 
   await startCustomizing();
+  let existingSpecial = null;
+  if (gPhotonStructure) {
+    existingSpecial = gCustomizeMode.visiblePalette.querySelector("toolbarspring");
+    ok(existingSpecial, "Should have a flexible space in the palette by default in photon");
+  }
   for (let id of elementsToMove) {
     simulateItemDragAndEnd(document.getElementById(id), PanelUI.contents);
   }
 
   assertAreaPlacements(startArea, placementsWithSpecials);
   assertAreaPlacements(targetArea, startingTargetPlacements);
 
   for (let id of elementsToMove) {
     simulateItemDrag(document.getElementById(id), gCustomizeMode.visiblePalette);
   }
 
   assertAreaPlacements(startArea, startingToolbarPlacements);
   assertAreaPlacements(targetArea, startingTargetPlacements);
 
-  ok(!gCustomizeMode.visiblePalette.querySelector("toolbarspring,toolbarseparator,toolbarspacer"),
-     "No specials should make it to the palette alive.");
+  let allSpecials = gCustomizeMode.visiblePalette.querySelectorAll("toolbarspring,toolbarseparator,toolbarspacer");
+  allSpecials = [...allSpecials].filter(special => special != existingSpecial);
+  ok(!allSpecials.length,
+     "No (new) specials should make it to the palette alive.");
   await endCustomizing();
 });
 
 
 add_task(async function asyncCleanup() {
   await endCustomizing();
   CustomizableUI.reset();
 });
--- a/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
+++ b/browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ -104,8 +104,12 @@ panic-button.tooltiptext = Forget about 
 # LOCALIZATION NOTE(devtools-webide-button.label, devtools-webide-button.tooltiptext):
 # widget is only visible after WebIDE has been started once (Tools > Web Developers > WebIDE)
 # %S is the keyboard shortcut
 devtools-webide-button2.label = WebIDE
 devtools-webide-button2.tooltiptext = Open WebIDE (%S)
 
 e10s-button.label = New Non-e10s Window
 e10s-button.tooltiptext = Open a new Non-e10s Window
+
+toolbarspring.label = Flexible Space
+toolbarseparator.label = Separator
+toolbarspacer.label = Space
--- a/browser/themes/shared/customizableui/customizeMode.inc.css
+++ b/browser/themes/shared/customizableui/customizeMode.inc.css
@@ -366,16 +366,31 @@ toolbarpaletteitem[place="toolbar"]:not(
   outline: 1px dotted;
   -moz-outline-radius: 2.5px;
 }
 
 toolbarpaletteitem[place="toolbar"]:not([mousedown="true"]):-moz-focusring {
   outline-offset: -5px;
 }
 
+toolbarpaletteitem[place=palette] > toolbarspring {
+  width: 7em;
+  min-width: 7em;
+  outline: 1px solid GrayText;
+  outline-offset: -8px;
+  height: 37px;
+}
+
+toolbarpaletteitem[place=toolbar] > toolbarspring {
+  outline: 1px solid GrayText;
+  outline-offset: -2px;
+  margin-top: 5px;
+  margin-bottom: 5px;
+}
+
 :root:not([photon-structure]) #wrapper-edit-controls[place="palette"] > #edit-controls > toolbarbutton,
 :root:not([photon-structure]) #wrapper-edit-controls[place="palette"] > #edit-controls > separator,
 :root:not([photon-structure]) #wrapper-zoom-controls[place="palette"] > #zoom-controls > toolbarbutton,
 :root:not([photon-structure]) #wrapper-zoom-controls[place="palette"] > #zoom-controls > separator {
   margin-top: 20px;
 }
 
 :root:not([photon-structure]) #wrapper-edit-controls[place="palette"] > #edit-controls > toolbarbutton,
--- a/browser/themes/shared/customizableui/panelUI.inc.css
+++ b/browser/themes/shared/customizableui/panelUI.inc.css
@@ -1598,16 +1598,17 @@ toolbarpaletteitem[place="palette"] > .t
 #search-container[cui-areatype="menu-panel"] {
   margin-top: 6px;
   margin-bottom: 6px;
 }
 
 toolbarpaletteitem[place="palette"] > #search-container {
   min-width: 7em;
   width: 7em;
+  min-height: 37px;
 }
 
 .toolbaritem-combined-buttons@inAnyPanel@ {
   background-color: transparent;
   border-radius: 2px;
   border: 1px solid;
   border-color: transparent;
   border-bottom-color: var(--panel-separator-color);