Bug 890140 - Fix placoholder code so there aren't extra rows created. r=Gijs

browser/components/customizableui/src/CustomizeMode.jsm

@@ -1143,13 +1143,12 @@ CustomizeMode.prototype = {
         aPlaceholder.style.removeProperty("width");
       }
 
-      let placeholders = Array.slice(panelContents.getElementsByClassName(
-        kPlaceholderClass));
+      let placeholders = Array.slice(panelContents.getElementsByClassName(kPlaceholderClass));
 
-      let toContract = placeholders.shift();
-      if (isPlaceholderAtEnd(toContract))
-        toContract = null;
       let toExpand = placeholders.shift();
+      let toContract = placeholders.shift();
+      if (toContract && isPlaceholderAtEnd(toContract))
+        toContract = null;
       // Seek to find hidden placeholders first to use for the expand transition.
       while (toExpand.getAttribute("hidden") != "true" && placeholders.length)
         toExpand = placeholders.shift();
@@ -1297,14 +1296,25 @@ CustomizeMode.prototype = {
     this._removePanelCustomizationPlaceholders();
     let doc = this.document;
     let contents = this.panelUIContents;
-    let visibleWideItems = contents.querySelectorAll("toolbarpaletteitem:not([hidden]) > .panel-wide-item");
+    const kWidePanelItemClass = "panel-wide-item";
+    let visibleWideItems = contents.querySelectorAll("toolbarpaletteitem:not([hidden]) > ." + kWidePanelItemClass);
     let visibleChildren = contents.querySelectorAll("toolbarpaletteitem:not([hidden])");
     // TODO(bug 885578): Still doesn't handle a hole when there is a wide
     //                   widget located at the bottom of the panel.
-    let hangingItems = (visibleChildren.length - visibleWideItems.length) % kColumnsInMenuPanel;
-    let newPlaceholders = kColumnsInMenuPanel;
-    let visiblePlaceholders = kColumnsInMenuPanel - hangingItems;
-    while (newPlaceholders--) {
+    let narrowItemsAfterWideItem = 0;
+    let node = contents.lastChild;
+    while (node && !node.classList.contains(kWidePanelItemClass) &&
+           (!node.firstChild || !node.firstChild.classList.contains(kWidePanelItemClass))) {
+      if (!node.hidden) {
+        narrowItemsAfterWideItem++;
+      }
+      node = node.previousSibling;
+    }
+
+    let orphanedItems = narrowItemsAfterWideItem % kColumnsInMenuPanel;
+    let placeholders = kColumnsInMenuPanel - orphanedItems;
+
+    while (placeholders--) {
       let placeholder = doc.createElement("toolbarpaletteitem");
       placeholder.classList.add(kPlaceholderClass);
       //XXXjaws The toolbarbutton child here is only necessary to get
@@ -1312,8 +1322,6 @@ CustomizeMode.prototype = {
       let placeholderChild = doc.createElement("toolbarbutton");
       placeholderChild.classList.add(kPlaceholderClass + "-child");
       placeholder.appendChild(placeholderChild);
-      // Always have at least 1 placeholder visible.
-      placeholder.setAttribute("hidden", --visiblePlaceholders < 0);
       contents.appendChild(placeholder);
     }
   },

browser/components/customizableui/test/browser.ini

@@ -3,16 +3,17 @@ support-files =
   head.js
 
 [browser_873501_handle_specials.js]
+[browser_876926_customize_mode_wrapping.js]
 [browser_877006_missing_view.js]
 [browser_877178_unregisterArea.js]
 [browser_877447_skip_missing_ids.js]
 [browser_878452_drag_to_panel.js]
-[browser_876926_customize_mode_wrapping.js]
 [browser_880382_drag_wide_widgets_in_panel.js]
 [browser_885530_showInPrivateBrowsing.js]
 [browser_886323_buildArea_removable_nodes.js]
 [browser_887438_currentset_shim.js]
 [browser_888817_currentset_updating.js]
+[browser_890140_orphaned_placeholders.js]
 [browser_890262_destroyWidget_after_add_to_panel.js]
 [browser_892955_isWidgetRemovable_for_removed_widgets.js]
 [browser_892956_destroyWidget_defaultPlacements.js]

browser/components/customizableui/test/browser_890140_orphaned_placeholders.js

@@ -0,0 +1,135 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+let gTests = [
+  {
+    desc: "One orphaned item should have two placeholders next to it.",
+    setup: startCustomizing,
+    run: function() {
+      let btn = document.getElementById("developer-button");
+      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
+      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
+
+      let placementsAfterAppend = placements.concat(["developer-button"]);
+      simulateItemDrag(btn, panel);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterAppend);
+      ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
+      is(getVisiblePlaceholderCount(panel), 2, "Should only have 2 visible placeholders before exiting");
+
+      yield endCustomizing();
+      yield startCustomizing();
+      is(getVisiblePlaceholderCount(panel), 2, "Should only have 2 visible placeholders after re-entering");
+
+      let palette = document.getElementById("customization-palette");
+      simulateItemDrag(btn, palette);
+      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
+    },
+  },
+  {
+    desc: "Two orphaned items should have one placeholder next to them (case 1).",
+    setup: startCustomizing,
+    run: function() {
+      let btn = document.getElementById("developer-button");
+      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
+      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
+
+      let placementsAfterAppend = placements.concat(["developer-button", "sync-button"]);
+      simulateItemDrag(btn, panel);
+      btn = document.getElementById("sync-button");
+      simulateItemDrag(btn, panel);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterAppend);
+      ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
+      is(getVisiblePlaceholderCount(panel), 1, "Should only have 1 visible placeholders before exiting");
+
+      yield endCustomizing();
+      yield startCustomizing();
+      is(getVisiblePlaceholderCount(panel), 1, "Should only have 1 visible placeholders after re-entering");
+
+      let palette = document.getElementById("customization-palette");
+      simulateItemDrag(btn, palette);
+      btn = document.getElementById("developer-button");
+      simulateItemDrag(btn, palette);
+      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
+    },
+  },
+  {
+    desc: "Two orphaned items should have one placeholder next to them (case 2).",
+    setup: startCustomizing,
+    run: function() {
+      let btn = document.getElementById("add-ons-button");
+      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
+      let palette = document.getElementById("customization-palette");
+      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
+
+      let placementsAfterAppend = placements.filter(p => p != btn.id);
+      simulateItemDrag(btn, palette);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterAppend);
+      ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
+      is(getVisiblePlaceholderCount(panel), 1, "Should only have 1 visible placeholders before exiting");
+
+      yield endCustomizing();
+      yield startCustomizing();
+      is(getVisiblePlaceholderCount(panel), 1, "Should only have 1 visible placeholders after re-entering");
+
+      simulateItemDrag(btn, panel);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placements);
+      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
+    },
+  },
+  {
+    desc: "A wide widget at the bottom of the panel should have three placeholders after it.",
+    setup: startCustomizing,
+    run: function() {
+      let btn = document.getElementById("edit-controls");
+      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
+      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
+
+      let placementsAfterAppend = placements.concat([placements.shift()]);
+      simulateItemDrag(btn, panel);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterAppend);
+      ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
+      is(getVisiblePlaceholderCount(panel), 3, "Should have 3 visible placeholders before exiting");
+
+      yield endCustomizing();
+      yield startCustomizing();
+      is(getVisiblePlaceholderCount(panel), 3, "Should have 3 visible placeholders after re-entering");
+
+      let zoomControls = document.getElementById("zoom-controls");
+      simulateItemDrag(btn, zoomControls);
+      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
+    },
+  },
+  {
+    desc: "The default placements should have three placeholders at the bottom.",
+    setup: startCustomizing,
+    run: function() {
+      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
+      ok(CustomizableUI.inDefaultState, "Should be in default state.");
+      is(getVisiblePlaceholderCount(panel), 3, "Should have 3 visible placeholders before exiting");
+
+      yield endCustomizing();
+      yield startCustomizing();
+      is(getVisiblePlaceholderCount(panel), 3, "Should have 3 visible placeholders after re-entering");
+
+      ok(CustomizableUI.inDefaultState, "Should still be in default state.");
+    },
+  },
+];
+
+function asyncCleanup() {
+  yield endCustomizing();
+  Services.prefs.clearUserPref("browser.uiCustomization.skipSourceNodeCheck");
+  yield resetCustomization();
+}
+
+function test() {
+  Services.prefs.setBoolPref("browser.uiCustomization.skipSourceNodeCheck", true);
+  waitForExplicitFinish();
+  runTests(gTests, asyncCleanup);
+}
+
+function getVisiblePlaceholderCount(aPanel) {
+  let visiblePlaceholders = aPanel.querySelectorAll(".panel-customization-placeholder:not([hidden=true])");
+  return visiblePlaceholders.length;
+}