Bug 1393574 - fix flexible spacers not being removable in some circumstances, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 06 Sep 2017 10:02:44 +0100
changeset 428745 48ed0961198671541c2a91a6d6cdf18758a6ace0
parent 428744 4f17343164b66db561be6371496c544f0c202a2d
child 428746 7efa30ab3893c3eb67e99ca115200c7e403bf941
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1393574, 1393661
milestone57.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 1393574 - fix flexible spacers not being removable in some circumstances, r=jaws The goal of this patch is to ensure that: - in default placements, specials have no unique ids - in actual placements as stored by CUI, they do - we reset the counter for those unique ids on reset. - we re-number specials when building an area (like on startup, or when resetting), ensuring that the actual nodes always match the placements for a given area. - we force saves after resetting, to ensure that the gNewElementCount is always persisted correctly. This last part will also fix bug 1393661 MozReview-Commit-ID: HAS5J5ZSgB5
browser/base/content/browser.xul
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/content/toolbar.xml
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_1096763_seen_widgets_post_reset.js
browser/components/customizableui/test/browser_currentset_post_reset.js
browser/components/customizableui/test/browser_remove_customized_specials.js
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -779,16 +779,17 @@
                        ondragover="homeButtonObserver.onDragOver(event)"
                        ondragenter="homeButtonObserver.onDragOver(event)"
                        ondrop="homeButtonObserver.onDrop(event)"
                        ondragexit="homeButtonObserver.onDragExit(event)"
                        key="goHome"
                        onclick="BrowserGoHome(event);"
                        cui-areatype="toolbar"
                        aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
+        <toolbarspring cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>
         <toolbaritem id="urlbar-container" flex="400" persist="width"
                      removable="false"
                      class="chromeclass-location" overflows="false">
             <textbox id="urlbar" flex="1"
                      placeholder="&urlbar.placeholder2;"
                      type="autocomplete"
                      autocompletesearch="unifiedcomplete"
                      autocompletesearchparam="enable-actions"
@@ -932,16 +933,17 @@
         </toolbaritem>
 
         <toolbaritem id="search-container" title="&searchItem.title;"
                      align="center" class="chromeclass-toolbar-additional panel-wide-item"
                      cui-areatype="toolbar"
                      flex="100" persist="width" removable="true">
           <searchbar id="searchbar" flex="1"/>
         </toolbaritem>
+        <toolbarspring cui-areatype="toolbar" class="chromeclass-toolbar-additional"/>
 
         <!-- This is a placeholder for the Downloads Indicator.  It is visible
              during the customization of the toolbar, in the palette, and before
              the Downloads Indicator overlay is loaded. -->
         <toolbarbutton id="downloads-button"
                        class="toolbarbutton-1 chromeclass-toolbar-additional badged-button"
                        key="key_openDownloads"
                        oncommand="DownloadsIndicatorView.onCommand(event);"
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -527,23 +527,17 @@ var CustomizableUIInternal = {
     let areaIsKnown = gAreas.has(aName);
     let props = areaIsKnown ? gAreas.get(aName) : new Map();
     const kImmutableProperties = new Set(["type", "legacy", "overflowable"]);
     for (let key in aProperties) {
       if (areaIsKnown && kImmutableProperties.has(key) &&
           props.get(key) != aProperties[key]) {
         throw new Error("An area cannot change the property for '" + key + "'");
       }
-      // XXXgijs for special items, we need to make sure they have an appropriate ID
-      // so we aren't perpetually in a non-default state:
-      if (key == "defaultPlacements" && Array.isArray(aProperties[key])) {
-        props.set(key, aProperties[key].map(x => this.isSpecialWidget(x) ? this.ensureSpecialWidgetId(x) : x ));
-      } else {
-        props.set(key, aProperties[key]);
-      }
+      props.set(key, aProperties[key]);
     }
     // Default to a toolbar:
     if (!props.has("type")) {
       props.set("type", CustomizableUI.TYPE_TOOLBAR);
     }
     if (props.get("type") == CustomizableUI.TYPE_TOOLBAR) {
       // Check aProperties instead of props because this check is only interested
       // in the passed arguments, not the state of a potentially pre-existing area.
@@ -735,18 +729,22 @@ var CustomizableUIInternal = {
     try {
       let currentNode = container.firstChild;
       let placementsToRemove = new Set();
       for (let id of aPlacements) {
         while (currentNode && currentNode.getAttribute("skipintoolbarset") == "true") {
           currentNode = currentNode.nextSibling;
         }
 
-        if (currentNode &&
-            (currentNode.id == id || this.matchingSpecials(id, currentNode.id))) {
+        // Fix ids for specials and continue, for correctly placed specials.
+        if (currentNode && (!currentNode.id || CustomizableUI.isSpecialWidget(currentNode)) &&
+            this.matchingSpecials(id, currentNode)) {
+          currentNode.id = id;
+        }
+        if (currentNode && currentNode.id == id) {
           currentNode = currentNode.nextSibling;
           continue;
         }
 
         if (this.isSpecialWidget(id) && areaIsPanel) {
           placementsToRemove.add(id);
           continue;
         }
@@ -796,19 +794,20 @@ var CustomizableUIInternal = {
         let node = container.lastChild;
         while (node && node != limit) {
           let previousSibling = node.previousSibling;
           // Nodes opt-in to removability. If they're removable, and we haven't
           // seen them in the placements array, then we toss them into the palette
           // if one exists. If no palette exists, we just remove the node. If the
           // node is not removable, we leave it where it is. However, we can only
           // safely touch elements that have an ID - both because we depend on
-          // IDs, and because such elements are not intended to be widgets
-          // (eg, titlebar-placeholder elements).
-          if (node.id && node.getAttribute("skipintoolbarset") != "true") {
+          // IDs (or are specials), and because such elements are not intended to
+          // be widgets (eg, titlebar-placeholder elements).
+          if ((node.id || this.isSpecialWidget(node)) &&
+              node.getAttribute("skipintoolbarset") != "true") {
             if (this.isWidgetRemovable(node)) {
               if (palette && !this.isSpecialWidget(node.id)) {
                 palette.appendChild(node);
                 this.removeLocationAttributes(node);
               } else {
                 container.removeChild(node);
               }
             } else {
@@ -1248,24 +1247,41 @@ var CustomizableUIInternal = {
     if (!node) {
       return false;
     }
     let win = e.view;
     let panels = gPanelsForWindow.get(win);
     return !!panels && panels.has(node);
   },
 
+  _getSpecialIdForNode(aNode) {
+    if (typeof aNode == "object" && aNode.localName) {
+      if (aNode.id) {
+        return aNode.id
+      }
+      if (aNode.localName.startsWith("toolbar")) {
+        return aNode.localName.substring(7);
+      }
+      return "";
+    }
+    return aNode;
+  },
+
   isSpecialWidget(aId) {
+    aId = this._getSpecialIdForNode(aId);
     return (aId.startsWith(kSpecialWidgetPfx) ||
             aId.startsWith("separator") ||
             aId.startsWith("spring") ||
             aId.startsWith("spacer"));
   },
 
   matchingSpecials(aId1, aId2) {
+    aId1 = this._getSpecialIdForNode(aId1);
+    aId2 = this._getSpecialIdForNode(aId2);
+
     return this.isSpecialWidget(aId1) &&
            this.isSpecialWidget(aId2) &&
            aId1.match(/spring|spacer|separator/)[0] == aId2.match(/spring|spacer|separator/)[0];
   },
 
   ensureSpecialWidgetId(aId) {
     let nodeType = aId.match(/spring|spacer|separator/)[0];
     // If the ID we were passed isn't a generated one, generate one now:
@@ -2562,39 +2578,42 @@ var CustomizableUIInternal = {
     // was reset above.
     this._rebuildRegisteredAreas();
 
     for (let [widgetId, widget] of gPalette) {
       if (widget.source == CustomizableUI.SOURCE_EXTERNAL) {
         gSeenWidgets.add(widgetId);
       }
     }
-    if (gSeenWidgets.size) {
+    if (gSeenWidgets.size || gNewElementCount) {
       gDirty = true;
+      this.saveState();
     }
 
     gResetting = false;
   },
 
   _resetUIState() {
     try {
       gUIStateBeforeReset.drawInTitlebar = Services.prefs.getBoolPref(kPrefDrawInTitlebar);
       gUIStateBeforeReset.uiCustomizationState = Services.prefs.getCharPref(kPrefCustomizationState);
       gUIStateBeforeReset.uiDensity = Services.prefs.getIntPref(kPrefUIDensity);
       gUIStateBeforeReset.autoTouchMode = Services.prefs.getBoolPref(kPrefAutoTouchMode);
       gUIStateBeforeReset.currentTheme = LightweightThemeManager.currentTheme;
+      gUIStateBeforeReset.newElementCount = gNewElementCount;
     } catch (e) { }
 
     this._resetExtraToolbars();
 
     Services.prefs.clearUserPref(kPrefCustomizationState);
     Services.prefs.clearUserPref(kPrefDrawInTitlebar);
     Services.prefs.clearUserPref(kPrefUIDensity);
     Services.prefs.clearUserPref(kPrefAutoTouchMode);
     LightweightThemeManager.currentTheme = null;
+    gNewElementCount = 0;
     log.debug("State reset");
 
     // Reset placements to make restoring default placements possible.
     gPlacements = new Map();
     gDirtyAreaCache = new Set();
     gSeenWidgets = new Set();
     // Clear the saved state to ensure that defaults will be used.
     gSavedState = null;
@@ -2656,16 +2675,17 @@ var CustomizableUIInternal = {
     }
     gUndoResetting = true;
 
     let uiCustomizationState = gUIStateBeforeReset.uiCustomizationState;
     let drawInTitlebar = gUIStateBeforeReset.drawInTitlebar;
     let currentTheme = gUIStateBeforeReset.currentTheme;
     let uiDensity = gUIStateBeforeReset.uiDensity;
     let autoTouchMode = gUIStateBeforeReset.autoTouchMode;
+    gNewElementCount = gUIStateBeforeReset.newElementCount;
 
     // Need to clear the previous state before setting the prefs
     // because pref observers may check if there is a previous UI state.
     this._clearPreviousUIState();
 
     Services.prefs.setCharPref(kPrefCustomizationState, uiCustomizationState);
     Services.prefs.setBoolPref(kPrefDrawInTitlebar, drawInTitlebar);
     Services.prefs.setIntPref(kPrefUIDensity, uiDensity);
@@ -2700,17 +2720,22 @@ var CustomizableUIInternal = {
    * @return {Boolean} whether the widget is removable
    */
   isWidgetRemovable(aWidget) {
     let widgetId;
     let widgetNode;
     if (typeof aWidget == "string") {
       widgetId = aWidget;
     } else {
-      widgetId = aWidget.id;
+      if (!aWidget.id &&
+          !["toolbarspring", "toolbarspacer", "toolbarseparator"].includes(aWidget.nodeName)) {
+        throw new Error("No nodes without ids that aren't special widgets should ever come into contact with CUI");
+      }
+      // Use "spring" / "spacer" / "separator" for special widgets without ids
+      widgetId = aWidget.id || aWidget.nodeName.substring(7 /* "toolbar".length */);
       widgetNode = aWidget;
     }
     let provider = this.getWidgetProvider(widgetId);
 
     if (provider == CustomizableUI.PROVIDER_API) {
       return gPalette.get(widgetId).removable;
     }
 
--- a/browser/components/customizableui/content/toolbar.xml
+++ b/browser/components/customizableui/content/toolbar.xml
@@ -192,17 +192,17 @@
             for (let node of overflowList.children) {
               let realNode = node.localName == "toolbarpaletteitem" ? node.firstChild : node;
               if (realNode.getAttribute("skipintoolbarset") != "true") {
                 currentWidgets.add(realNode.id);
               }
             }
           }
           let orderedPlacements = CustomizableUI.getWidgetIdsInArea(this.id);
-          return orderedPlacements.filter((x) => currentWidgets.has(x)).join(",");
+          return orderedPlacements.filter(w => currentWidgets.has(w)).join(",");
         ]]></getter>
         <setter><![CDATA[
           // Get list of new and old ids:
           let newVal = (val || "").split(",").filter(x => x);
           let oldIds = CustomizableUI.getWidgetIdsInArea(this.id);
 
           // Get a list of items only in the new list
           let newIds = newVal.filter(id => oldIds.indexOf(id) == -1);
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -132,29 +132,31 @@ skip-if = os == "linux" # crashing on Li
 tags = fullscreen
 skip-if = os == "mac"
 [browser_1087303_button_preferences.js]
 [browser_1089591_still_customizable_after_reset.js]
 [browser_1096763_seen_widgets_post_reset.js]
 [browser_1161838_inserted_new_default_buttons.js]
 [browser_allow_dragging_removable_false.js]
 [browser_bootstrapped_custom_toolbar.js]
+[browser_currentset_post_reset.js]
 [browser_customizemode_contextmenu_menubuttonstate.js]
 [browser_customizemode_uidensity.js]
 [browser_exit_background_customize_mode.js]
 [browser_overflow_use_subviews.js]
 [browser_panel_keyboard_navigation.js]
 [browser_panel_toggle.js]
 [browser_panelUINotifications.js]
 [browser_panelUINotifications_fullscreen.js]
 tags = fullscreen
 skip-if = os == "mac"
 [browser_panelUINotifications_fullscreen_noAutoHideToolbar.js]
 tags = fullscreen
 [browser_panelUINotifications_multiWindow.js]
+[browser_remove_customized_specials.js]
 [browser_switch_to_customize_mode.js]
 [browser_synced_tabs_menu.js]
 [browser_backfwd_enabled_post_customize.js]
 [browser_check_tooltips_in_navbar.js]
 [browser_editcontrols_update.js]
 subsuite = clipboard
 [browser_customization_context_menus.js]
 [browser_open_from_popup.js]
--- a/browser/components/customizableui/test/browser_1096763_seen_widgets_post_reset.js
+++ b/browser/components/customizableui/test/browser_1096763_seen_widgets_post_reset.js
@@ -9,17 +9,16 @@ add_task(async function() {
     defaultArea: CustomizableUI.AREA_NAVBAR
   });
 
   const kPrefCustomizationState = "browser.uiCustomization.state";
   let bsPass = Cu.import("resource:///modules/CustomizableUI.jsm", {});
   ok(bsPass.gSeenWidgets.has(BUTTONID), "Widget should be seen after createWidget is called.");
   CustomizableUI.reset();
   ok(bsPass.gSeenWidgets.has(BUTTONID), "Widget should still be seen after reset.");
-  ok(!Services.prefs.prefHasUserValue(kPrefCustomizationState), "Pref shouldn't be set right now, because that'd break undo.");
   CustomizableUI.addWidgetToArea(BUTTONID, CustomizableUI.AREA_NAVBAR);
   gCustomizeMode.removeFromArea(document.getElementById(BUTTONID));
   let hasUserValue = Services.prefs.prefHasUserValue(kPrefCustomizationState);
   ok(hasUserValue, "Pref should be set right now.");
   if (hasUserValue) {
     let seenArray = JSON.parse(Services.prefs.getCharPref(kPrefCustomizationState)).seen;
     isnot(seenArray.indexOf(BUTTONID), -1, "Widget should be in saved 'seen' list.");
   }
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_currentset_post_reset.js
@@ -0,0 +1,25 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+function checkSpacers() {
+  let navbarWidgets = CustomizableUI.getWidgetIdsInArea("nav-bar");
+  let currentSetWidgets = document.getElementById("nav-bar").currentSet.split(",");
+  navbarWidgets = navbarWidgets.filter(w => CustomizableUI.isSpecialWidget(w));
+  currentSetWidgets = currentSetWidgets.filter(w => CustomizableUI.isSpecialWidget(w));
+  Assert.deepEqual(navbarWidgets, currentSetWidgets, "Should have the same 'special' widgets in currentset and placements");
+}
+
+/**
+ * Check that after a reset, the currentset property correctly deals with flexible spacers.
+ */
+add_task(async function() {
+  await startCustomizing();
+  checkSpacers();
+
+  CustomizableUI.addWidgetToArea("spring", "nav-bar", 4 /* Insert before the last extant spacer */);
+  await gCustomizeMode.reset();
+  checkSpacers();
+  await endCustomizing();
+});
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_remove_customized_specials.js
@@ -0,0 +1,28 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Check that after a reset, we can still drag special nodes in customize mode
+ */
+add_task(async function() {
+  await startCustomizing();
+  CustomizableUI.addWidgetToArea("spring", "nav-bar", 5);
+  await gCustomizeMode.reset();
+  let springs = document.querySelectorAll("#nav-bar toolbarspring");
+  let lastSpring = springs[springs.length - 1];
+  let expectedPlacements = CustomizableUI.getWidgetIdsInArea("nav-bar");
+  info("Placements before drag: " + expectedPlacements.join(","));
+  let lastItem = document.getElementById(expectedPlacements[expectedPlacements.length - 1]);
+  simulateItemDrag(lastSpring, lastItem);
+  expectedPlacements.splice(expectedPlacements.indexOf(lastSpring.id), 1);
+  expectedPlacements.splice(expectedPlacements.length - 1, 0, lastSpring.id);
+  let actualPlacements = CustomizableUI.getWidgetIdsInArea("nav-bar");
+  // Log these separately because Assert.deepEqual truncates the stringified versions...
+  info("Actual placements: " + actualPlacements.join(","));
+  info("Expected placements: " + expectedPlacements.join(","));
+  Assert.deepEqual(expectedPlacements, actualPlacements, "Should be able to move spring");
+  await gCustomizeMode.reset();
+  await endCustomizing();
+});