[MD Bookmarks] Add a drag and drop indicator to bookmarks.

chrome/browser/resources/md_bookmarks/dnd_manager.js

Index: chrome/browser/resources/md_bookmarks/dnd_manager.js
diff --git a/chrome/browser/resources/md_bookmarks/dnd_manager.js b/chrome/browser/resources/md_bookmarks/dnd_manager.js
new file mode 100644
index 0000000000000000000000000000000000000000..dd2236121d227e3fdfe31cced20aa2663aeb906c
--- /dev/null
+++ b/chrome/browser/resources/md_bookmarks/dnd_manager.js
@@ -0,0 +1,513 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/** @typedef {{element: BookmarkElement, position: DropPosition}} */

[tsergeant, 2017/03/20 03:01:49]

You could put this, BookmarkElement and DragData into types.js to clean up this
file a bit and get them out of the build.

[calamity, 2017/03/22 05:29:52]

Did some. Couldn't do this one since it depends on DropPosition which is a
forrealz enum and can't be moved just into types.js.

+var DropDestination;
+
+cr.define('bookmarks', function() {
+  /** @const */
+  var ROOT_NODE_ID = '0';
+
+  /**
+   * @record
+   */
+  function BookmarkElement() {}
+
+  /** @type {string} */
+  BookmarkElement.itemId;
+
+  /** @return {HTMLElement} */
+  BookmarkElement.getDropTarget = function() {};
+
+  /**
+   * Enumeration of valid drop locations relative to an element. These are
+   * bit masks to allow combining multiple locations in a single value.
+   * @enum {number}
+   * @const
+   */
+  var DropPosition = {
+    NONE: 0,
+    ABOVE: 1,
+    ON: 2,
+    BELOW: 4,
+  };
+
+  /**
+   * @param {BookmarkElement} element
+   * @return {boolean}
+   */
+  function isBookmarkItem(element) {
+    return element.tagName == 'BOOKMARKS-ITEM';
+  }
+
+  /**
+   * @param {BookmarkElement} element
+   * @return {boolean}
+   */
+  function isBookmarkFolderNode(element) {
+    return element.tagName == 'BOOKMARKS-FOLDER-NODE';
+  }
+
+  /**
+   * @param {Array<!Element>|undefined} path
+   * @return {BookmarkElement}
+   */
+  function getBookmarkElement(path) {
+    if (!path)
+      return null;
+
+    for (var i = 0; i < path.length; i++) {
+      if (isBookmarkItem(path[i]) || isBookmarkFolderNode(path[i]))
+        return path[i];
+    }
+    return null;
+  }
+
+  /**
+   * @param {BookmarkElement} bookmarkElement
+   * @return {BookmarkNode}
+   */
+  function getBookmarkNode(bookmarkElement) {
+    return bookmarks.Store.getInstance().data.nodes[bookmarkElement.itemId];
+  }
+
+  /** @constructor */
+  function DragData() {
+    /** @type {Array<number>} */
+    this.elements = null;
+
+    /** @type {boolean} */
+    this.sameProfile = false;
+  }
+
+  /**
+   * @constructor

[tsergeant, 2017/03/20 03:01:50]

Maybe add a comment here explaining the purpose of this class? I was quite
confused about it until I realised that dragData was only set by the
bookmarkManagerPrivate listener.

[calamity, 2017/03/22 05:29:52]

Done.

+   */
+  function DragInfo() {
+    /** @type {DragData} */
+    this.dragData = null;
+  }
+
+  DragInfo.prototype = {
+    /** @param {DragData} newDragData */
+    handleChromeDragEnter: function(newDragData) {
+      this.dragData = newDragData;
+    },
+
+    clearDragData: function() {
+      this.dragData = null;
+    },
+
+    /** @return {boolean} */
+    isDragValid: function() {
+      return !!this.dragData;
+    },
+
+    /** @return {boolean} */
+    isSameProfile: function() {
+      return !!this.dragData && this.dragData.sameProfile;
+    },
+
+    /** @return {boolean} */
+    isDraggingFolders: function() {
+      return !!this.dragData && this.dragData.elements.some(function(node) {
+        return !node.url;
+      });
+    },
+
+    /** @return {boolean} */
+    isDraggingBookmark: function(bookmarkId) {
+      return !!this.dragData && this.dragData.elements.some(function(node) {
+        return node.id == bookmarkId;
+      });
+    },
+
+    /** @return {boolean} */
+    isDraggingChildBookmark: function(folderId) {
+      return !!this.dragData && this.dragData.elements.some(function(node) {
+        return node.parentId == folderId;
+      });
+    },
+
+    /** @return {boolean} */
+    isDraggingFolderToDescendant: function(itemId, nodes) {
+      var parentId = nodes[itemId].parentId;
+      var parents = {};
+      while (parentId) {
+        parents[parentId] = true;
+        parentId = nodes[parentId].parentId;
+      }
+
+      return !!this.dragData && this.dragData.elements.some(function(node) {
+        return parents[node.id];
+      });
+    },
+  };
+
+  /**
+   * @constructor
+   */
+  function DropIndicator() {
+    /**
+     * @type {number|null} Timer id used to help minimize flicker.
+     */
+    this.removeDropIndicatorTimer = null;
+
+    /**
+     * The element that had a style applied it to indicate the drop location.
+     * This is used to easily remove the style when necessary.
+     * @type {BookmarkElement|null}
+     */
+    this.lastIndicatorElement = null;
+
+    /**
+     * The style that was applied to indicate the drop location.
+     * @type {?string|null}
+     */
+    this.lastIndicatorClassName = null;
+  }
+
+  DropIndicator.prototype = {
+    /**
+     * Applies the drop indicator style on the target element and stores that
+     * information to easily remove the style in the future.
+     * @param {HTMLElement} indicatorElement
+     * @param {DropPosition} position
+     */
+    addDropIndicatorStyle: function(indicatorElement, position) {
+      var indicatorStyleName = position == DropPosition.ABOVE ?
+          'drag-above' :
+          position == DropPosition.BELOW ? 'drag-below' : 'drag-on';
+
+      this.lastIndicatorElement = indicatorElement;
+      this.lastIndicatorClassName = indicatorStyleName;
+
+      indicatorElement.classList.add(indicatorStyleName);
+    },
+
+    /**
+     * Clears the drop indicator style from the last element was the drop target
+     * so the drop indicator is no longer for that element.
+     */
+    removeDropIndicatorStyle: function() {
+      if (!this.lastIndicatorElement || !this.lastIndicatorClassName)
+        return;
+
+      this.lastIndicatorElement.classList.remove(this.lastIndicatorClassName);
+      this.lastIndicatorElement = null;
+      this.lastIndicatorClassName = null;
+    },
+
+    /**
+     * Displays the drop indicator on the current drop target to give the
+     * user feedback on where the drop will occur.
+     * @param {DropDestination} dropDest
+     */
+    update: function(dropDest) {
+      window.clearTimeout(this.removeDropIndicatorTimer);
+
+      var indicatorElement = dropDest.element.getDropTarget();
+      var position = dropDest.position;
+
+      this.removeDropIndicatorStyle();
+      this.addDropIndicatorStyle(indicatorElement, position);
+    },
+
+    /**
+     * Stop displaying the drop indicator.
+     */
+    finish: function() {
+      // The use of a timeout is in order to reduce flickering as we move
+      // between valid drop targets.
+      window.clearTimeout(this.removeDropIndicatorTimer);
+      this.removeDropIndicatorTimer = window.setTimeout(function() {
+        this.removeDropIndicatorStyle();
+      }.bind(this), 100);
+    }
+  };
+
+  Polymer({

[tsergeant, 2017/03/20 03:01:50]

I don't really think this needs to be a Polymer element. It doesn't do anything
Polymer-y: the only thing it makes use of is the lifecycle callbacks.

I'll leave it up to you, but this might be nicer as a regular JS class.

[calamity, 2017/03/22 05:29:52]

Done.

+    is: 'bookmarks-dnd-manager',
+
+    /** @private {bookmarks.DragInfo} */
+    dragInfo_: null,
+
+    /** @private {?DropDestination} */
+    dropDestination_: null,
+
+    /** @private {bookmarks.DropIndicator} */
+    dropIndicator_: null,
+
+    /** @private {Object<string, function(!Event)>} */
+    documentListeners_: null,
+
+    /** @override */
+    attached: function() {
+      this.dragInfo_ = new DragInfo();
+      this.dropIndicator_ = new DropIndicator();
+
+      this.documentListeners_ = {
+        'dragstart': this.onDragStart_.bind(this),
+        'dragenter': this.onDragEnter_.bind(this),
+        'dragover': this.onDragOver_.bind(this),
+        'dragleave': this.onDragLeave_.bind(this),
+        'drop': this.onDrop_.bind(this),
+        'dragend': this.clearDragData_.bind(this),
+        'mouseup': this.clearDragData_.bind(this),
+        // TODO(calamity): Add touch support.
+      };
+      for (var event in this.documentListeners_)
+        document.addEventListener(event, this.documentListeners_[event]);
+
+      chrome.bookmarkManagerPrivate.onDragEnter.addListener(
+          this.dragInfo_.handleChromeDragEnter.bind(this.dragInfo_));
+      chrome.bookmarkManagerPrivate.onDragLeave.addListener(
+          this.clearDragData_.bind(this));
+      chrome.bookmarkManagerPrivate.onDrop.addListener(
+          this.clearDragData_.bind(this));
+    },
+
+    /** @override */
+    detached: function() {
+      for (var event in this.documentListeners_)
+        document.removeEventListener(event, this.documentListeners_[event]);
+    },
+
+    /** @private */
+    onDragLeave_: function() {
+      this.dropIndicator_.finish();
+    },
+
+    /**
+     * @private
+     * @param {!Event} e
+     */
+    onDrop_: function(e) {
+      if (this.dropDestination_) {
+        e.preventDefault();
+      }
+      this.dropDestination_ = null;
+      this.dropIndicator_.finish();
+    },
+
+    /** @private */
+    clearDragData_: function() {
+      this.dragInfo_.clearDragData();
+      this.dropDestination_ = null;
+      this.dropIndicator_.finish();
+    },
+
+    /**
+     * @private
+     * @param {Event} e
+     */
+    onDragStart_: function(e) {
+      var dragElement = getBookmarkElement(e.path);
+      if (!dragElement)
+        return;
+
+      // Determine the selected bookmarks.
+      var state = bookmarks.Store.getInstance().data;
+      var selectedItems = Object.keys(state.selection.items);
+      var draggedNodes =

[tsergeant, 2017/03/20 03:01:50]

This logic doesn't seem right:

1) If you drag a folder node while items are selected, you'll drag the selected
items instead of the folder

2) If you select some items then drag a different list item, you'll drag the
selection rather than the drag target.

[calamity, 2017/03/22 05:29:52]

Good point. Fixed.

+          selectedItems.length ? selectedItems : [dragElement.itemId];
+
+      if (!draggedNodes.length)
+        return;
+
+      e.preventDefault();
+
+      // If we are dragging a single link, we can do the *Link* effect.
+      // Otherwise, we only allow copy and move.
+      e.dataTransfer.effectAllowed =
+          draggedNodes.length == 1 && state.nodes[draggedNodes[0]].url ?
+          'copyMoveLink' :

[tsergeant, 2017/03/20 03:01:50]

I think 'copyMoveLink' is...bogus?

It's not in the MDN docs, and doesn't appear anywhere else in the codebase:

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/effectAllowed
https://cs.chromium.org/search/?q=copyMoveLink&ssfr=1&type=cs

[calamity, 2017/03/22 05:29:52]

Lol.

+          'copyMove';
+
+      // TODO(calamity): account for touch.
+      chrome.bookmarkManagerPrivate.startDrag(draggedNodes, false);
+    },
+
+    /**
+     * @private
+     * @param {Event} e
+     */
+    onDragEnter_: function(e) {
+      e.preventDefault();
+    },
+
+    /**
+     * @private
+     * @param {Event} e
+     */
+    onDragOver_: function(e) {
+      // Allow normal DND on text inputs.
+      if (e.path[0].tagName == 'INPUT')
+        return;
+
+      // The default operation is to allow dropping links etc to do
+      // navigation. We never want to do that for the bookmark manager.
+      e.preventDefault();
+
+      // Set to none. This will get set to something if we can do the drop.
+      e.dataTransfer.dropEffect = 'none';
+
+      if (!this.dragInfo_.isDragValid())
+        return;
+
+      var overElement = getBookmarkElement(e.path);
+      if (!overElement)
+        return;
+
+      // TODO(calamity): open folders on hover.
+
+      // Now we know that we can drop. Determine if we will drop above, on or
+      // below based on mouse position etc.
+      this.dropDestination_ =
+          this.calculateDropDestination_(e.clientY, overElement);
+      if (!this.dropDestination_) {
+        e.dataTransfer.dropEffect = 'none';
+        return;
+      }
+
+      e.dataTransfer.dropEffect =
+          this.dragInfo_.isSameProfile() ? 'move' : 'copy';
+      this.dropIndicator_.update(this.dropDestination_);
+    },
+
+    /**
+     * This function determines where the drop will occur.
+     * @param {number} elementClientY
+     * @param {!BookmarkElement} overElement
+     * @return {?DropDestination} If no valid drop position is found, null,
+     *   otherwise:
+     *       element - The target element that will receive the drop.
+     *       position - A |DropPosition| relative to the |element|.
+     */
+    calculateDropDestination_: function(elementClientY, overElement) {
+      var validDropPositions = this.calculateValidDropPositions_(overElement);
+      if (validDropPositions == DropPosition.NONE)
+        return null;
+
+      var above = validDropPositions & DropPosition.ABOVE;
+      var below = validDropPositions & DropPosition.BELOW;
+      var on = validDropPositions & DropPosition.ON;
+      var rect = overElement.getDropTarget().getBoundingClientRect();
+      var yRatio = (elementClientY - rect.top) / rect.height;
+
+      if (above && (yRatio <= .25 || yRatio <= .5 && (!below || !on)))
+        return {element: overElement, position: DropPosition.ABOVE};
+
+      if (below && (yRatio > .75 || yRatio > .5 && (!above || !on)))
+        return {element: overElement, position: DropPosition.BELOW};
+
+      if (on)
+        return {element: overElement, position: DropPosition.ON};
+
+      return null;
+    },
+
+    /**
+     * Determines the valid drop positions for the given target element.
+     * @param {!BookmarkElement} overElement The element that we are currently
+     *     dragging over.
+     * @return {DropPosition} An bit field enumeration of valid drop locations.
+     */
+    calculateValidDropPositions_: function(overElement) {
+      var dragInfo = this.dragInfo_;
+      var state = bookmarks.Store.getInstance().data;
+
+      // Drags aren't allowed onto the search result list.
+      if (isBookmarkItem(overElement) &&
+          bookmarks.util.isShowingSearch(state)) {
+        return DropPosition.NONE;
+      }
+
+      // Drags of a bookmark onto itself or of a folder into its children aren't
+      // allowed.
+      if (dragInfo.isSameProfile() &&
+          (dragInfo.isDraggingBookmark(overElement.itemId) ||
+           dragInfo.isDraggingFolderToDescendant(
+               overElement.itemId, state.nodes))) {
+        return DropPosition.NONE;
+      }
+
+      var validDropPositions = this.calculateDropAboveBelow(overElement);
+      if (this.canDropOn(overElement))
+        validDropPositions |= DropPosition.ON;
+
+      return validDropPositions;
+    },
+
+    /**
+     * @param {BookmarkElement} overElement
+     * @return {DropPosition}
+     */
+    calculateDropAboveBelow: function(overElement) {

[tsergeant, 2017/03/20 03:01:49]

I've noticed a couple of (minor) behavioral differences. Not sure why they are
different and whether they're actually bugs, but you should check anyway.

If I have a tree like

Bookmarks Bar
-> a
   -> b
      -> c

And I grab 'c', I am able to drop c:

- Below 'b'
- Onto 'a'

Neither of which are possible in the old bookmark manager.

[calamity, 2017/03/22 05:29:52]

Case 1 is a bug here, Case 2 is a bug with the old bookmark manager...

It uses the bounding rect of the entire node, including all it's children so the
ratio is bogus. Don't know if we want to explicitly cause the same behavior.

+      var dragInfo = this.dragInfo_;
+      var state = bookmarks.Store.getInstance().data;
+
+      // We cannot drop between Bookmarks bar and Other bookmarks.
+      if (getBookmarkNode(overElement).parentId == ROOT_NODE_ID)

[tsergeant, 2017/03/20 03:01:49]

Maybe extract this into a util function like isRootNode(), which can be used
here and in folder_node

(it would probably also be handy in reducers)

[calamity, 2017/03/22 05:29:52]

Moved the constant to be part of bookmarks.util. Having a method seems weird
since the id is what we should check.

+        return DropPosition.NONE;
+
+      var isOverFolderNode = isBookmarkFolderNode(overElement);
+
+      // We can only drop between items in the tree if we have any folders.
+      if (isOverFolderNode && !dragInfo.isDraggingFolders())
+        return DropPosition.NONE;
+
+      var resultPositions = DropPosition.NONE;
+
+      // Cannot drop above if the item above is already in the drag source.
+      var previousElem = overElement.previousElementSibling;
+      if (!dragInfo.isSameProfile() || !previousElem ||
+          !dragInfo.isDraggingBookmark(previousElem.itemId)) {
+        resultPositions |= DropPosition.ABOVE;
+      }
+
+      // Don't allow dropping below an expanded sidebar folder item since it is
+      // confusing to the user anyway.
+      if (isOverFolderNode && state.closedFolders[overElement.itemId])
+        return resultPositions;
+
+      // Cannot drop below if the item below is already in the drag source.
+      var nextElement = overElement.nextElementSibling;
+
+      // The template element sits past the end of the last bookmark element.
+      if (!isBookmarkItem(nextElement) && !isBookmarkFolderNode(nextElement))
+        nextElement = null;
+
+      if (!dragInfo.isSameProfile() || !nextElement ||
+          !dragInfo.isDraggingBookmark(nextElement.itemId)) {
+        resultPositions |= DropPosition.BELOW;
+      }
+
+      return resultPositions;
+    },
+
+    /**
+     * Determine whether we can drop the dragged items on the drop target.
+     * @param {!BookmarkElement} overElement The element that we are currently
+     *     dragging over.
+     * @return {boolean} Whether we can drop the dragged items on the drop
+     *     target.
+     */
+    canDropOn: function(overElement) {
+      // We can only drop on a folder.
+      if (getBookmarkNode(overElement).url)
+        return false;
+
+      if (!this.dragInfo_.isSameProfile())
+        return true;
+
+      return !this.dragInfo_.isDraggingChildBookmark(overElement.itemId)
+    },
+  });
+
+  return {
+    DragInfo: DragInfo,
+    DropIndicator: DropIndicator,
+  };
+});