[NTP] Allow reordering of apps via drag and drop.

chrome/browser/resources/ntp/draganddrop.js

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Aaron Boodman, 2011/01/22 23:42:57]

Seems like this file should be called drag_drop_controller.js.

[jstritar, 2011/01/24 01:00:42]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// The delegate interface:
+//   dragContainer -->
+//         element containing the draggable items
+//
+//   transitionsDuration -->
+//         length of time of transitions in ms
+//
+//   dragItem -->
+//         get / set property containing the item being dragged
+//
+//   dimensions -->
+//         property that specifies the dimensions of the items
+//         The object should contain the following properties:
+//           itemHeight, itemWidth, marginWidth, marginHeight, borderWidth

[Aaron Boodman, 2011/01/22 23:42:57]

Why does this class care about marginWidth/height borderWidth/height? Why not
just the overall dimensions?

[jstritar, 2011/01/24 01:00:42]

I removed the dependency on the dimensions.

+//
+//   getItem(e) -->
+//         get's the item that is under the mouse event |e|
+//
+//   canDropOn(position) -->
+//         returns true if the position index is a valid place to drop an item
+//
+//   setDragPlaceholder(position) -->
+//         tells the delegate that the dragged item is currently above
+//         |position|
+//
+//   saveDrag() -->
+//         tells the delegate that the drag is done. move the item to the
+//         position last specified by setDragPlaceholder. (e.g., commit changes)
+//
+
+function DragAndDropController(delegate) {
+  this.delegate = delegate;

[Aaron Boodman, 2011/01/22 23:42:57]

this.delegate_ ?

[jstritar, 2011/01/24 01:00:42]

Done.

+
+  this.installHandlers_();
+}
+
+DragAndDropController.prototype = {
+  startX_: 0,
+  startY_: 0,
+  startScreenX_: 0,
+  startScreenY_: 0,
+
+  installHandlers_: function() {
+    var el = this.delegate.dragContainer;
+    el.addEventListener('dragstart', this.handleDragStart_.bind(this));
+    el.addEventListener('dragenter', this.handleDragEnter_.bind(this));
+    el.addEventListener('dragover', this.handleDragOver_.bind(this));
+    el.addEventListener('dragleave', this.handleDragLeave_.bind(this));
+    el.addEventListener('drop', this.handleDrop_.bind(this));
+    el.addEventListener('dragend', this.handleDragEnd_.bind(this));
+    el.addEventListener('drag', this.handleDrag_.bind(this));
+    el.addEventListener('mousedown', this.handleMouseDown_.bind(this));
+  },
+
+  isDragging: function() {
+    return !!this.delegate.dragItem;

[Aaron Boodman, 2011/01/22 23:42:57]

Nit: prefer Boolean(this.delegate.dragItem) or this.delegate.dragItem != null
for clarity.

[jstritar, 2011/01/24 01:00:42]

I realized I wasn't using this method, so I yanked it out.

+  },
+
+  getIndexAt_: function(e) {

[Aaron Boodman, 2011/01/22 23:42:57]

Design nit: I think that knowledge of layout should be centralized in the
delegate. By having getIndexAt_ in this class, you are baking in the assumption
that each drag position can be mapped to an index, and that the index is all the
information that the delegate will need in order to decide what to do in
canDropOn().

I think it would be more elegant to put getIndexAt_() in apps, and just send the
coordinate in canDropOn(). Leave it up to the delegate to decide what to do with
it.

I might be missing something that makes this complicated though.

[jstritar, 2011/01/24 01:00:42]

Great idea-- I think that's what felt off about the delegate / controller
interface. I now pass the coordinates into the canDropOn method and its working
a lot nicer. 

+    var del = this.delegate;

[Aaron Boodman, 2011/01/22 23:42:57]

Careful with your abbreviations again. Since delegate is only used twice in this
class, this.delegate_ both times is fine IMO.

[jstritar, 2011/01/24 01:00:42]

Done. Cleaned this up throughout the class.

+    var el = del.dragContainer;

[Aaron Boodman, 2011/01/22 23:42:57]

This variable is only used once, might as well just have it inline at that time.

[jstritar, 2011/01/24 01:00:42]

Done.

+    var rect = el.getBoundingClientRect();
+    var d = del.dimensions;
+    var availableWidth = el.offsetWidth;
+
+    var clientX = e.clientX + window.scrollX - rect.left;
+    var clientY = e.clientY + window.scrollY - rect.top;
+
+    var w = d.itemWidth + 2 * d.borderWidth + 2 * d.marginWidth;
+    var h = d.itemHeight + 2 * d.borderWidth + 2 * d.marginHeight;
+
+    var rtl = isRtl();
+    if (rtl)
+      clientX = availableWidth - clientX;
+
+    var row = Math.floor(clientY / h);
+    var col = Math.floor(clientX / w);
+    var index = Math.floor(availableWidth / w) * row + col;
+
+    return index;
+  },
+
+  // Listen to mousedown to get the relative position of the cursor when
+  // starting drag and drop.
+  handleMouseDown_: function(e) {
+    var item = this.delegate.getItem(e);
+    if (item) {

[Aaron Boodman, 2011/01/22 23:42:57]

Invert check and return early. Multiple places in this file.

[jstritar, 2011/01/24 01:00:42]

Done.

+      this.startX_ = item.offsetLeft;
+      this.startY_ = item.offsetTop;
+      this.startScreenX_ = e.screenX;
+      this.startScreenY_ = e.screenY;
+
+      // We don't want to focus the item on mousedown. However, to prevent
+      // focus one has to call preventDefault but this also prevents the drag
+      // and drop (sigh) so we only prevent it when the user is not doing a
+      // left mouse button drag.
+      if (e.button != 0) // LEFT
+        e.preventDefault();
+    }
+  },
+
+  handleDragStart_: function(e) {
+    var item = this.delegate.getItem(e);
+    if (item) {
+      // Don't set data since HTML5 does not allow setting the name for
+      // url-list. Instead, we just rely on the dragging of link behavior.
+      this.delegate.dragItem = item;
+      item.classList.add('dragging');
+      item.style.zIndex = 2;
+
+      e.dataTransfer.effectAllowed = 'copyLinkMove';
+    }
+  },
+
+  handleDragEnter_: function(e) {
+    if (this.delegate.canDropOn(this.getIndexAt_(e)))
+      e.preventDefault();
+  },
+
+  handleDragOver_: function(e) {
+    var del = this.delegate;
+    var position = this.getIndexAt_(e);
+    if (del.canDropOn(position)) {
+      del.setDragPlaceholder(position);
+      e.preventDefault();
+      e.dataTransfer.dropEffect = 'move';
+    }
+  },
+
+  handleDragLeave_: function(e) {
+    if (this.delegate.canDropOn(this.getIndexAt_(e)))
+      e.preventDefault();
+  },
+
+  handleDrop_: function(e) {
+    var del = this.delegate;
+    var dragItem = del.dragItem;
+    if (dragItem) {
+      dragItem.classList.remove('dragging');
+      del.dragItem = null;
+      del.invalidate();

[Aaron Boodman, 2011/01/22 23:42:57]

Seems like these two methods should be del.saveDrag().

[jstritar, 2011/01/24 01:00:42]

Done.

+      del.layout();
+
+      setTimeout(function() {
+        dragItem.style.pointerEvents = '';
+        dragItem.style.zIndex = '';
+      }, this.delegate.transitionsDuration + 10);
+    }
+  },
+
+  handleDragEnd_: function(e) {
+    return this.handleDrop_(e);
+  },
+
+  handleDrag_: function(e) {
+    // Moves the drag item making sure that it is not displayed outside the
+    // browser viewport.
+    var del = this.delegate;
+    var dragItem = del.dragItem;
+    var rect = del.dragContainer.getBoundingClientRect();
+
+    var x = this.startX_ + e.screenX - this.startScreenX_;
+    var y = this.startY_ + e.screenY - this.startScreenY_;
+
+    // The position of the item is relative to #apps so we need to
+    // subtract that when calculating the allowed position.
+    x = Math.max(x, -rect.left);
+    x = Math.min(x, document.body.clientWidth - rect.left -
+                    dragItem.offsetWidth - 2);
+
+    // The shadow is 2px
+    y = Math.max(-rect.top, y);
+    y = Math.min(y, document.body.clientHeight - rect.top -
+                    dragItem.offsetHeight - 2);
+
+    // Override right in case of RTL.
+    dragItem.style.right = 'auto';

[Aaron Boodman, 2011/01/22 23:42:57]

Don't think this line is needed.

[jstritar, 2011/01/24 01:00:42]

Done.

+    dragItem.style.left = x + 'px';
+    dragItem.style.top = y + 'px';
+    dragItem.style.zIndex = 2;
+  }
+};