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 607384 051284a3564e0a798fea06a7dde26300ac2288a6
parent 607383 064f2ad7ca3bcd37df0ca60dd870a722ca96c756
child 607385 85332a69ae0cbad6556541883960c517b1b86a03
push id67985
push userbmo:emilio+bugs@crisal.io
push dateWed, 12 Jul 2017 08:36:44 +0000
reviewersmossop
bugs1366813
milestone56.0a1
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);