Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container, r?mikedeboer draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 18 Sep 2017 16:36:46 +0100
changeset 667014 aaafc2349e7a8984f8fbc4dba161b8d1e4e521b5
parent 666884 e4261f5b96ebfd63e7cb8af3035ff9fea90c74a5
child 667015 98440eca116a71d3dc942438fd4f5c17b0f151d7
push id80590
push userbmo:gijskruitbosch+bugs@gmail.com
push dateTue, 19 Sep 2017 15:05:12 +0000
reviewersmikedeboer
bugs1399517
milestone57.0a1
Bug 1399517 - CustomizableUI shouldn't try to insert items next to nodes that aren't in the right container, r?mikedeboer MozReview-Commit-ID: H9KY7YQwmlQ
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_insert_before_moved_node.js
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -1231,19 +1231,25 @@ var CustomizableUIInternal = {
     }
 
     let container = aAreaNode.customizationTarget;
     let placements = gPlacements.get(areaId);
     let nodeIndex = placements.indexOf(aNode.id);
 
     while (++nodeIndex < placements.length) {
       let nextNodeId = placements[nodeIndex];
-      let nextNode = container.getElementsByAttribute("id", nextNodeId).item(0);
-
-      if (nextNode) {
+      let nextNode = aNode.ownerDocument.getElementById(nextNodeId);
+      // If the next placed widget exists, and is a direct child of the
+      // container, or wrapped in a customize mode wrapper (toolbarpaletteitem)
+      // inside the container, insert beside it.
+      // We have to check the parent to avoid errors when the placement ids
+      // are for nodes that are no longer customizable.
+      if (nextNode && (nextNode.parentNode == container ||
+                       (nextNode.parentNode.localName == "toolbarpaletteitem" &&
+                        nextNode.parentNode.parentNode == container))) {
         return [container, nextNode];
       }
     }
 
     return [container, null];
   },
 
   insertWidgetBefore(aNode, aNextNode, aContainer, aArea) {
@@ -4558,30 +4564,46 @@ OverflowableToolbar.prototype = {
   findOverflowedInsertionPoints(aNode) {
     let newNodeCanOverflow = aNode.getAttribute("overflows") != "false";
     let areaId = this._toolbar.id;
     let placements = gPlacements.get(areaId);
     let nodeIndex = placements.indexOf(aNode.id);
     let nodeBeforeNewNodeIsOverflown = false;
 
     let loopIndex = -1;
+    // Loop through placements to find where to insert this item.
+    // As soon as we find an overflown widget, we will only
+    // insert in the overflow panel (this is why we check placements
+    // before the desired location for the new node). Once we pass
+    // the desired location of the widget, we look for placement ids
+    // that actually have DOM equivalents to insert before. If all
+    // else fails, we insert at the end of either the overflow list
+    // or the toolbar target.
     while (++loopIndex < placements.length) {
       let nextNodeId = placements[loopIndex];
       if (loopIndex > nodeIndex) {
-        if (newNodeCanOverflow && this._collapsed.has(nextNodeId)) {
-          let nextNode = this._list.getElementsByAttribute("id", nextNodeId).item(0);
-          if (nextNode) {
-            return [this._list, nextNode];
-          }
+        let nextNode = aNode.ownerDocument.getElementById(nextNodeId);
+        // If the node we're inserting can overflow, and the next node
+        // in the toolbar is overflown, we should insert this node
+        // in the overflow panel before it.
+        if (newNodeCanOverflow && this._collapsed.has(nextNodeId) &&
+            nextNode && nextNode.parentNode == this._list) {
+          return [this._list, nextNode];
         }
-        if (!nodeBeforeNewNodeIsOverflown || !newNodeCanOverflow) {
-          let nextNode = this._target.getElementsByAttribute("id", nextNodeId).item(0);
-          if (nextNode) {
-            return [this._target, nextNode];
-          }
+        // Otherwise (if either we can't overflow, or the previous node
+        // wasn't overflown), and the next node is in the toolbar itself,
+        // insert the node in the toolbar.
+        if ((!nodeBeforeNewNodeIsOverflown || !newNodeCanOverflow) && nextNode &&
+            (nextNode.parentNode == this._target ||
+             // Also check if the next node is in a customization wrapper
+             // (toolbarpaletteitem). We don't need to do this for the
+             // overflow case because overflow is disabled in customize mode.
+             (nextNode.parentNode.localName == "toolbarpaletteitem" &&
+              nextNode.parentNode.parentNode == this._target))) {
+          return [this._target, nextNode];
         }
       } else if (loopIndex < nodeIndex && this._collapsed.has(nextNodeId)) {
         nodeBeforeNewNodeIsOverflown = true;
       }
     }
 
     let containerForAppending = (this._collapsed.size && newNodeCanOverflow) ?
                                 this._list : this._target;
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -138,16 +138,17 @@ skip-if = os == "mac"
 [browser_allow_dragging_removable_false.js]
 [browser_bootstrapped_custom_toolbar.js]
 [browser_currentset_post_reset.js]
 [browser_customizemode_contextmenu_menubuttonstate.js]
 [browser_customizemode_dragspace.js]
 skip-if = os == "linux" # linux doesn't get drag space (no tabsintitlebar)
 [browser_customizemode_uidensity.js]
 [browser_exit_background_customize_mode.js]
+[browser_insert_before_moved_node.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]
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_insert_before_moved_node.js
@@ -0,0 +1,37 @@
+"use strict";
+
+/**
+ * Check inserting before a node that has moved from the toolbar into a
+ * non-customizable bit of the browser works.
+ */
+add_task(async function() {
+  for (let toolbar of ["nav-bar", "TabsToolbar"]) {
+    CustomizableUI.createWidget({id: "real-button", label: "test real button"});
+    CustomizableUI.addWidgetToArea("real-button", toolbar);
+    CustomizableUI.addWidgetToArea("moved-button-not-here", toolbar);
+    let placements = CustomizableUI.getWidgetIdsInArea(toolbar);
+    Assert.deepEqual(placements.slice(-2), ["real-button", "moved-button-not-here"],
+      "Should have correct placements");
+    let otherButton = document.createElement("toolbarbutton");
+    otherButton.id = "moved-button-not-here";
+    if (toolbar == "nav-bar") {
+      gURLBar.parentNode.appendChild(otherButton);
+    } else {
+      gBrowser.tabContainer.appendChild(otherButton);
+    }
+    CustomizableUI.destroyWidget("real-button");
+    CustomizableUI.createWidget({id: "real-button", label: "test real button"});
+
+    let button = document.getElementById("real-button");
+    ok(button, "Button should exist");
+    if (button) {
+      let expectedContainer = document.getElementById(toolbar).customizationTarget;
+      is(button.parentNode, expectedContainer, "Button should be in the toolbar");
+    }
+
+    CustomizableUI.destroyWidget("real-button");
+    otherButton.remove();
+    CustomizableUI.reset();
+  }
+});
+