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

chrome/browser/resources/md_bookmarks/list.js

 // Copyright 2016 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.
 
 Polymer({
   is: 'bookmarks-list',
 
   behaviors: [
     bookmarks.StoreClient,
   ],
@@ skipping 46 matching lines @@
       return bookmarks.util.getDisplayedList(state);
     });
     this.watch('searchTerm_', function(state) {
       return state.search.term;
     });
     this.watch('selectedFolder_', function(state) {
       return state.selectedFolder;
     });
     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} */
   getDropTarget: function() {
     return this.$.message;
   },
 
   /**
    * Updates `displayedList_` using splices to be equivalent to `newValue`. This
    * allows the iron-list to delete sublists of items which preserves scroll and
@@ skipping 22 matching lines @@
         ].concat(additions));
       }.bind(this));
     }
   },
 
   /** @private */
   onDisplayedListSourceChange_: function() {
     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';
     return loadTimeData.getString(emptyListMessage);
   },
 
   /** @private */
   isEmptyList_: function() {
     return this.displayedList_.length == 0;
   },
 
   /** @private */
   deselectItems_: function() {
     this.dispatch(bookmarks.actions.deselectItems());
   },
 
   /**
    * @param{HTMLElement} el
    * @private
    */
   getIndexForItemElement_: function(el) {
     return this.$.bookmarksCard.modelForElement(el).index;
   },
 
   /**
    * @param {Event} e
    * @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.

+    });
+  },
+
+  /**
    * @param {KeyboardEvent} e
    * @private
    */
   onItemKeydown_: function(e) {
     var handled = true;
     var list = this.$.bookmarksCard;
     var focusMoved = false;
     var focusedIndex =
         this.getIndexForItemElement_(/** @type {HTMLElement} */ (e.target));
     var oldFocusedIndex = focusedIndex;
@@ skipping 54 matching lines @@
 
     if (!handled) {
       handled = bookmarks.CommandManager.getInstance().handleKeyEvent(
           e, this.getState().selection.items);
     }
 
     if (handled)
       e.stopPropagation();
   },
 });