Bug 1396423 - make it possible to drop items in what visually looks like the palette but isn't, r=mikedeboer
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 29 Sep 2017 15:40:48 +0100
changeset 383800 a9af2009e4069ae9e4f26001f2e9fdde0bbcdc3e
parent 383799 1bac4ad2bddb0517896ba78b407fb1301a55eb0f
child 383801 fc55e32ba087bd92f68db2dd34e779b4ff1ca304
push id52397
push usergijskruitbosch@gmail.com
push dateFri, 29 Sep 2017 20:04:59 +0000
treeherderautoland@a9af2009e406 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1396423
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1396423 - make it possible to drop items in what visually looks like the palette but isn't, r=mikedeboer MozReview-Commit-ID: AqBwn9ovTjT
browser/components/customizableui/CustomizeMode.jsm
browser/components/customizableui/test/browser.ini
browser/components/customizableui/test/browser_drag_outside_palette.js
--- a/browser/components/customizableui/CustomizeMode.jsm
+++ b/browser/components/customizableui/CustomizeMode.jsm
@@ -26,16 +26,18 @@ const kDownloadAutohidePanelId = "downlo
 const kDownloadAutoHidePref = "browser.download.autohideButton";
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource:///modules/CustomizableUI.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/AddonManager.jsm");
 Cu.import("resource://gre/modules/AppConstants.jsm");
 
+Cu.importGlobalProperties(["CSS"]);
+
 XPCOMUtils.defineLazyModuleGetter(this, "DragPositionManager",
                                   "resource:///modules/DragPositionManager.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BrowserUITelemetry",
                                   "resource:///modules/BrowserUITelemetry.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BrowserUtils",
                                   "resource://gre/modules/BrowserUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
                                   "resource://gre/modules/LightweightThemeManager.jsm");
@@ -323,17 +325,17 @@ CustomizeMode.prototype = {
       Services.obs.addObserver(this, "lightweight-theme-window-updated");
 
       // Let everybody in this window know that we're about to customize.
       CustomizableUI.dispatchToolboxEvent("customizationstarting", {}, window);
 
       await this._wrapToolbarItems();
       this.populatePalette();
 
-      this._addDragHandlers(this.visiblePalette);
+      this._setupPaletteDragging();
 
       window.gNavToolbox.addEventListener("toolbarvisibilitychange", this);
 
       this._updateResetButton();
       this._updateUndoResetButton();
 
       this._skipSourceNodeCheck = Services.prefs.getPrefType(kSkipSourceNodePref) == Ci.nsIPrefBranch.PREF_BOOL &&
                                   Services.prefs.getBoolPref(kSkipSourceNodePref);
@@ -433,18 +435,17 @@ CustomizeMode.prototype = {
       }
       let browser = document.getElementById("browser");
       browser.parentNode.selectedPanel = browser;
       let customizer = document.getElementById("customization-container");
       customizer.hidden = true;
 
       window.gNavToolbox.removeEventListener("toolbarvisibilitychange", this);
 
-      DragPositionManager.stop();
-      this._removeDragHandlers(this.visiblePalette);
+      this._teardownPaletteDragging();
 
       await this._unwrapToolbarItems();
 
       if (this._changed) {
         // XXXmconley: At first, it seems strange to also persist the old way with
         //             currentset - but this might actually be useful for switching
         //             to old builds. We might want to keep this around for a little
         //             bit.
@@ -1505,16 +1506,52 @@ CustomizeMode.prototype = {
         }
         break;
       case "unload":
         this.uninit();
         break;
     }
   },
 
+  /**
+   * We handle dragover/drop on the outer palette separately
+   * to avoid overlap with other drag/drop handlers.
+   */
+  _setupPaletteDragging() {
+    this._addDragHandlers(this.visiblePalette);
+
+    this.paletteDragHandler = (aEvent) => {
+      let originalTarget = aEvent.originalTarget;
+      if (this._isUnwantedDragDrop(aEvent) ||
+          this.visiblePalette.contains(originalTarget) ||
+          this.document.getElementById("customization-panelHolder").contains(originalTarget)) {
+        return;
+      }
+      // We have a dragover/drop on the palette.
+      if (aEvent.type == "dragover") {
+        this._onDragOver(aEvent, this.visiblePalette);
+      } else {
+        this._onDragDrop(aEvent, this.visiblePalette);
+      }
+    };
+    let contentContainer = this.document.getElementById("customization-content-container");
+    contentContainer.addEventListener("dragover", this.paletteDragHandler, true);
+    contentContainer.addEventListener("drop", this.paletteDragHandler, true);
+  },
+
+  _teardownPaletteDragging() {
+    DragPositionManager.stop();
+    this._removeDragHandlers(this.visiblePalette);
+
+    let contentContainer = this.document.getElementById("customization-content-container");
+    contentContainer.removeEventListener("dragover", this.paletteDragHandler, true);
+    contentContainer.removeEventListener("drop", this.paletteDragHandler, true);
+    delete this.paletteDragHandler;
+  },
+
   observe(aSubject, aTopic, aData) {
     switch (aTopic) {
       case "nsPref:changed":
         this._updateResetButton();
         this._updateUndoResetButton();
         if (AppConstants.CAN_DRAW_IN_TITLEBAR) {
           this._updateTitlebarCheckbox();
           this._updateDragSpaceCheckbox();
@@ -1648,17 +1685,17 @@ CustomizeMode.prototype = {
         }
       }
       this._initializeDragAfterMove = null;
       this.window.clearTimeout(this._dragInitializeTimeout);
     };
     this._dragInitializeTimeout = this.window.setTimeout(this._initializeDragAfterMove, 0);
   },
 
-  _onDragOver(aEvent) {
+  _onDragOver(aEvent, aOverrideTarget) {
     if (this._isUnwantedDragDrop(aEvent)) {
       return;
     }
     if (this._initializeDragAfterMove) {
       this._initializeDragAfterMove();
     }
 
     __dumpDragData(aEvent);
@@ -1667,17 +1704,17 @@ CustomizeMode.prototype = {
     let documentId = document.documentElement.id;
     if (!aEvent.dataTransfer.mozTypesAt(0)) {
       return;
     }
 
     let draggedItemId =
       aEvent.dataTransfer.mozGetDataAt(kDragDataTypePrefix + documentId, 0);
     let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
-    let targetArea = this._getCustomizableParent(aEvent.currentTarget);
+    let targetArea = this._getCustomizableParent(aOverrideTarget || aEvent.currentTarget);
     let originArea = this._getCustomizableParent(draggedWrapper);
 
     // Do nothing if the target or origin are not customizable.
     if (!targetArea || !originArea) {
       return;
     }
 
     // Do nothing if the widget is not allowed to be removed.
@@ -1755,26 +1792,26 @@ CustomizeMode.prototype = {
       }
       this._dragOverItem = dragOverItem;
     }
 
     aEvent.preventDefault();
     aEvent.stopPropagation();
   },
 
-  _onDragDrop(aEvent) {
+  _onDragDrop(aEvent, aOverrideTarget) {
     if (this._isUnwantedDragDrop(aEvent)) {
       return;
     }
 
     __dumpDragData(aEvent);
     this._initializeDragAfterMove = null;
     this.window.clearTimeout(this._dragInitializeTimeout);
 
-    let targetArea = this._getCustomizableParent(aEvent.currentTarget);
+    let targetArea = this._getCustomizableParent(aOverrideTarget || aEvent.currentTarget);
     let document = aEvent.target.ownerDocument;
     let documentId = document.documentElement.id;
     let draggedItemId =
       aEvent.dataTransfer.mozGetDataAt(kDragDataTypePrefix + documentId, 0);
     let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
     let originArea = this._getCustomizableParent(draggedWrapper);
     if (this._dragSizeMap) {
       this._dragSizeMap = new WeakMap();
@@ -2177,77 +2214,59 @@ CustomizeMode.prototype = {
     } else {
       aDraggedItem.parentNode.hidden = true;
     }
     return size;
   },
 
   _getCustomizableParent(aElement) {
     if (aElement) {
-      // Deal with drag/drop on the padding of the panel in photon.
+      // Deal with drag/drop on the padding of the panel.
       let containingPanelHolder = aElement.closest("#customization-panelHolder");
       if (containingPanelHolder) {
         return containingPanelHolder.firstChild;
       }
     }
 
     let areas = CustomizableUI.areas;
     areas.push(kPaletteId);
-    while (aElement) {
-      if (areas.indexOf(aElement.id) != -1) {
-        return aElement;
-      }
-      aElement = aElement.parentNode;
-    }
-
-    return null;
+    return aElement.closest(areas.map(a => "#" + CSS.escape(a)).join(","));
   },
 
   _getDragOverNode(aEvent, aAreaElement, aAreaType, aDraggedItemId) {
     let expectedParent = aAreaElement.customizationTarget || aAreaElement;
+    if (!expectedParent.contains(aEvent.target)) {
+      return expectedParent;
+    }
     // Our tests are stupid. Cope:
     if (!aEvent.clientX && !aEvent.clientY) {
       return aEvent.target;
     }
     // Offset the drag event's position with the offset to the center of
     // the thing we're dragging
     let dragX = aEvent.clientX - this._dragOffset.x;
     let dragY = aEvent.clientY - this._dragOffset.y;
 
     // Ensure this is within the container
     let boundsContainer = expectedParent;
-    // NB: because the panel UI itself is inside a scrolling container, we need
-    // to use the parent bounds (otherwise, if the panel UI is scrolled down,
-    // the numbers we get are in window coordinates which leads to various kinds
-    // of weirdness)
-    if (boundsContainer == this.panelUIContents) {
-      boundsContainer = boundsContainer.parentNode;
-    }
-    let bounds = boundsContainer.getBoundingClientRect();
+    let bounds = this._dwu.getBoundsWithoutFlushing(boundsContainer);
     dragX = Math.min(bounds.right, Math.max(dragX, bounds.left));
     dragY = Math.min(bounds.bottom, Math.max(dragY, bounds.top));
 
     let targetNode;
     if (aAreaType == "toolbar" || aAreaType == "menu-panel") {
       targetNode = aAreaElement.ownerDocument.elementFromPoint(dragX, dragY);
       while (targetNode && targetNode.parentNode != expectedParent) {
         targetNode = targetNode.parentNode;
       }
     } else {
       let positionManager = DragPositionManager.getManagerForArea(aAreaElement);
       // Make it relative to the container:
       dragX -= bounds.left;
-      // NB: but if we're in the panel UI, we need to use the actual panel
-      // contents instead of the scrolling container to determine our origin
-      // offset against:
-      if (expectedParent == this.panelUIContents) {
-        dragY -= this.panelUIContents.getBoundingClientRect().top;
-      } else {
-        dragY -= bounds.top;
-      }
+      dragY -= bounds.top;
       // Find the closest node:
       targetNode = positionManager.find(aAreaElement, dragX, dragY, aDraggedItemId);
     }
     return targetNode || aEvent.target;
   },
 
   _onMouseDown(aEvent) {
     log.debug("_onMouseDown");
--- a/browser/components/customizableui/test/browser.ini
+++ b/browser/components/customizableui/test/browser.ini
@@ -137,16 +137,17 @@ skip-if = os == "mac"
 [browser_1161838_inserted_new_default_buttons.js]
 [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_drag_outside_palette.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
new file mode 100644
--- /dev/null
+++ b/browser/components/customizableui/test/browser_drag_outside_palette.js
@@ -0,0 +1,42 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+/**
+ * Check that moving items from the toolbar or panel to the palette by
+ * dropping on the panel container (not inside the visible panel) works.
+ */
+add_task(async function() {
+  await startCustomizing();
+  let panelContainer = document.getElementById("customization-panel-container");
+  // Try dragging an item from the navbar:
+  let homeButton = document.getElementById("home-button");
+  let oldNavbarPlacements = CustomizableUI.getWidgetIdsInArea("nav-bar");
+  simulateItemDrag(homeButton, panelContainer);
+  assertAreaPlacements(CustomizableUI.AREA_NAVBAR,
+    oldNavbarPlacements.filter(w => w != "home-button"));
+  ok(homeButton.closest("#customization-palette"), "Button should be in the palette");
+
+  // Put it in the panel and try again from there:
+  let panelHolder = document.getElementById("customization-panelHolder");
+  simulateItemDrag(homeButton, panelHolder);
+  assertAreaPlacements(CustomizableUI.AREA_FIXED_OVERFLOW_PANEL,
+    ["home-button"]);
+
+  simulateItemDrag(homeButton, panelContainer);
+  assertAreaPlacements(CustomizableUI.AREA_FIXED_OVERFLOW_PANEL, []);
+
+  ok(homeButton.closest("#customization-palette"), "Button should be in the palette");
+
+  // Check we can't move non-removable items like this:
+  let urlbar = document.getElementById("urlbar-container");
+  simulateItemDrag(urlbar, panelContainer);
+  assertAreaPlacements(CustomizableUI.AREA_NAVBAR,
+    oldNavbarPlacements.filter(w => w != "home-button"));
+});
+
+registerCleanupFunction(async function() {
+  await gCustomizeMode.reset();
+  await endCustomizing();
+});