MD Bookmarks: Scroll and select items that are added to the main list

chrome/browser/resources/md_bookmarks/list.js

Index: chrome/browser/resources/md_bookmarks/list.js
diff --git a/chrome/browser/resources/md_bookmarks/list.js b/chrome/browser/resources/md_bookmarks/list.js
index 7d772a19766b1a8682de44eb493a6f8998e90082..80dd1f5c6e9fd32e7948565617da2a3c30ba5c9c 100644
--- a/chrome/browser/resources/md_bookmarks/list.js
+++ b/chrome/browser/resources/md_bookmarks/list.js
@@ -64,8 +64,19 @@ Polymer({
     });
     this.updateFromStore();
 
+    this.boundOnItemKeydown_ = this.onItemKeydown_.bind(this);
+    this.boundOnHighlightItems_ = this.onHighlightItems_.bind(this);
+
     this.$.bookmarksCard.addEventListener(
-        'keydown', this.onItemKeydown_.bind(this), true);
+        'keydown', this.boundOnItemKeydown_, true);
+    document.addEventListener('highlight-items', this.boundOnHighlightItems_);
+  },
+
+  detached: function() {
+    this.$.bookmarksCard.removeEventListener(
+        'keydown', this.boundOnItemKeydown_, true);
+    document.removeEventListener(
+        'highlight-items', this.boundOnHighlightItems_);
   },
 
   /** @return {HTMLElement} */
@@ -108,6 +119,20 @@ Polymer({
     this.scrollTop = 0;
   },
 
+  /**
+   * Scroll the list so that |itemId| is visible, if it is not already.
+   * @param {string} itemId
+   * @private
+   */
+  scrollToId_: function(itemId) {
+    var index = this.displayedIds_.indexOf(itemId);
+    var list = this.$.bookmarksCard;
+    if (index >= 0 && index < list.firstVisibleIndex ||
+        index > list.lastVisibleIndex) {
+      list.scrollToIndex(index);
+    }
+  },
+
   /** @private */
   emptyListMessage_: function() {
     var emptyListMessage = this.searchTerm_ ? 'noSearchResults' : 'emptyList';
@@ -137,12 +162,33 @@ Polymer({
    * @private
    */
   onOpenItemMenu_: function(e) {
-    var index = this.displayedIds_.indexOf(
-        /** @type {BookmarksItemElement} */ (e.path[0]).itemId);
-    var list = this.$.bookmarksCard;
     // If the item is not visible, scroll to it before rendering the menu.
-    if (index < list.firstVisibleIndex || index > list.lastVisibleIndex)
-      list.scrollToIndex(index);
+    this.scrollToId_(/** @type {BookmarksItemElement} */ (e.path[0]).itemId);
+  },
+
+  /**
+   * Highlight a list of items by selecting them, scrolling them into view and
+   * focusing the first item.
+   * @param {Event} e
+   * @private
+   */
+  onHighlightItems_: function(e) {
+    var toHighlight = /** @type {!Array<string>} */
+        (e.detail.filter((item) => this.displayedIds_.indexOf(item) != -1));

[calamity, 2017/07/14 04:14:39]

Comment plx. What are the cases where this comes up?

[tsergeant, 2017/07/14 06:39:20]

This is just being defensive, but I think it's a reasonable check to have: This
array accepts any list of IDs, and the current caller (ApiListener) doesn't make
any exact guarantees that the creations it sees are the ones we care about.

[calamity, 2017/07/17 05:09:49]

Acknowledged.

+
+    assert(toHighlight.length > 0);
+    var leadId = toHighlight[0];
+    this.dispatch(bookmarks.actions.updateAnchor(leadId));
+    this.dispatch(
+        bookmarks.actions.selectAll(toHighlight, this.getState(), leadId));

[calamity, 2017/07/14 04:14:39]

So a batch of operations from a move isn't _guaranteed_ to be a single batch.
Shouldn't this be additive in case the highlight event fires early? Or will
everything be totally borked in that case anyway?

[calamity, 2017/07/14 04:14:39]

leadId is selected as the anchor here and in the previous statement.

[tsergeant, 2017/07/14 06:39:19]

Yeah, this is the thing that was hard about this CL. Without having callbacks
from the APIs to definitively tell when an operation is complete, we just can't
be certain when the batch of create calls is finished.

Making this additive wouldn't necessarily be sufficient, because we still need
to decide when to turn off trackUpdatedItems in ApiListener.

One alternative solution is to be more like the old bookmark manager, which sets
a timeout for 300ms and highlights everything that was created and moved during
that time. The problem there is that our processing time for big batches could
potentially be more than 300ms on a slow device anyway.

In short, I don't really know what the best solution is, but this seems to work
well while I've been testing it.

[tsergeant, 2017/07/14 06:39:20]

Done.

[calamity, 2017/07/17 05:09:49]

Can we add a completion callback to Drop and Paste to guarantee completion of
batched operations?

[tsergeant, 2017/07/18 01:08:54]

Done.

+
+    // Allow iron-list time to render changes to the list.

[calamity, 2017/07/14 04:14:39]

What changes need to be rendered here? The adds/removes from the batchUpdate? Is
this guaranteed to happen in a single task?

[tsergeant, 2017/07/14 06:39:20]

Yeah, additions to the list are what's important. iron-list gives no formal
guarantees on when things will be done rendering. But in the current version, it
renders with microtask timing. In the latest version with Polymer 2.0, it will
render with animation frame timing, welp:
https://github.com/PolymerElements/iron-list/blob/master/iron-list.html#L1104

+    this.async(function() {
+      this.scrollToId_(leadId);
+      var leadIndex = this.displayedIds_.indexOf(leadId);
+      assert(leadIndex != -1);
+      this.$.bookmarksCard.focusItem(leadIndex);

[calamity, 2017/07/14 04:14:39]

tangent: I would love to rename bookmarksCard to like, ironList or something.

[tsergeant, 2017/07/14 06:39:19]

Renamed to list.

+    });
   },
 
   /**