Bug 996635 - fix removing widgets from outside their area, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 12 May 2014 21:47:17 +0100
changeset 183599 aee3e89fa990a785ce7acc3416efabacca0a09eb
parent 183598 0f54154e533267bf893337b2f4f6bf91d372ca20
child 183600 ece88e3d2ebf882c87991e279065211d3c69f478
push id6842
push usergijskruitbosch@gmail.com
push dateSat, 17 May 2014 08:27:00 +0000
treeherderfx-team@aee3e89fa990 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs996635
milestone32.0a1
Bug 996635 - fix removing widgets from outside their area, r=jaws
browser/components/customizableui/src/CustomizableUI.jsm
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_996635_remove_non_widgets.js
--- a/browser/components/customizableui/src/CustomizableUI.jsm
+++ b/browser/components/customizableui/src/CustomizableUI.jsm
@@ -767,25 +767,26 @@ let CustomizableUIInternal = {
 
     for (let areaNode of areaNodes) {
       let window = areaNode.ownerDocument.defaultView;
       if (!showInPrivateBrowsing &&
           PrivateBrowsingUtils.isWindowPrivate(window)) {
         continue;
       }
 
+      let container = areaNode.customizationTarget;
       let widgetNode = window.document.getElementById(aWidgetId);
-      if (!widgetNode) {
+      if (widgetNode && isOverflowable) {
+        container = areaNode.overflowable.getContainerFor(widgetNode);
+      }
+
+      if (!widgetNode || !container.contains(widgetNode)) {
         INFO("Widget not found, unable to remove");
         continue;
       }
-      let container = areaNode.customizationTarget;
-      if (isOverflowable) {
-        container = areaNode.overflowable.getContainerFor(widgetNode);
-      }
 
       this.notifyListeners("onWidgetBeforeDOMChange", widgetNode, null, container, true);
 
       // We remove location attributes here to make sure they're gone too when a
       // widget is removed from a toolbar to the palette. See bug 930950.
       this.removeLocationAttributes(widgetNode);
       // We also need to remove the panel context menu if it's there:
       this.ensureButtonContextMenu(widgetNode);
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -103,12 +103,13 @@ skip-if = os == "linux"
 [browser_987177_destroyWidget_xul.js]
 [browser_987177_xul_wrapper_updating.js]
 [browser_987492_window_api.js]
 [browser_987640_charEncoding.js]
 [browser_992747_toggle_noncustomizable_toolbar.js]
 [browser_993322_widget_notoolbar.js]
 [browser_995164_registerArea_during_customize_mode.js]
 [browser_996364_registerArea_different_properties.js]
+[browser_996635_remove_non_widgets.js]
 [browser_1003588_no_specials_in_panel.js]
 [browser_1008559_anchor_undo_restore.js]
 [browser_bootstrapped_custom_toolbar.js]
 [browser_panel_toggle.js]
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_996635_remove_non_widgets.js
@@ -0,0 +1,43 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// NB: This is testing what happens if something that /isn't/ a customizable
+// widget gets used in CustomizableUI APIs. Don't use this as an example of
+// what should happen in a "normal" case or how you should use the API.
+function test() {
+  // First create a button that isn't customizable, and add it in the nav-bar,
+  // but not in the customizable part of it (the customization target) but
+  // next to the main (hamburger) menu button.
+  const buttonID = "Test-non-widget-non-removable-button";
+  let btn = document.createElement("toolbarbutton");
+  btn.id = buttonID;
+  btn.label = "Hi";
+  btn.setAttribute("style", "width: 20px; height: 20px; background-color: red");
+  document.getElementById("nav-bar").appendChild(btn);
+  registerCleanupFunction(function() {
+    btn.remove();
+  });
+
+  // Now try to add this non-customizable button to the tabstrip. This will
+  // update the internal bookkeeping (ie placements) information, but shouldn't
+  // move the node.
+  CustomizableUI.addWidgetToArea(buttonID, CustomizableUI.AREA_TABSTRIP);
+  let placement = CustomizableUI.getPlacementOfWidget(buttonID);
+  // Check our bookkeeping
+  ok(placement, "Button should be placed");
+  is(placement && placement.area, CustomizableUI.AREA_TABSTRIP, "Should be placed on tabstrip.");
+  // Check we didn't move the node.
+  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");
+
+  // Now remove the node again. This should remove the bookkeeping, but again
+  // not affect the actual node.
+  CustomizableUI.removeWidgetFromArea(buttonID);
+  placement = CustomizableUI.getPlacementOfWidget(buttonID);
+  // Check our bookkeeping:
+  ok(!placement, "Button should no longer have a placement.");
+  // Check our node.
+  is(btn.parentNode && btn.parentNode.id, "nav-bar", "Actual button should still be on navbar.");
+}
+