Bug 996635 - fix removing widgets from outside their area, r=jaws
☠☠ backed out by 00a3ea4089ce ☠ ☠
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 12 May 2014 21:47:17 +0100
changeset 183217 b32eb8beb7eba63b85b6b4f4b6c58f50b9c9369d
parent 183216 7ce6c7d52d6fd69b37cbac240672f1f4b201c617
child 183218 d65835b006b49688caf8ec6843665a66f5ee5581
push id6818
push usergijskruitbosch@gmail.com
push dateThu, 15 May 2014 22:17:24 +0000
treeherderfx-team@b32eb8beb7eb [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,22 +767,23 @@ 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 || !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.
--- 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.");
+}
+