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
--- 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();
+ }
+});
+