Make downloads list keyboard shortcuts more consistent.

ui/webui/resources/js/cr/ui/focus_row.js

Index: ui/webui/resources/js/cr/ui/focus_row.js
diff --git a/ui/webui/resources/js/cr/ui/focus_row.js b/ui/webui/resources/js/cr/ui/focus_row.js
index 26c4c5567e2b673f24b19574c2b35afe6c69263f..5a8426141aa455be595e5319e9a008b8fd07a30f 100644
--- a/ui/webui/resources/js/cr/ui/focus_row.js
+++ b/ui/webui/resources/js/cr/ui/focus_row.js
@@ -26,25 +26,21 @@ cr.define('cr.ui', function() {
    * changes to a node inside |this.boundary_|. If opt_boundary isn't
    * specified, any focus change deactivates the row.
    *
-   * @param {!Array.<!Element>|!NodeList} items Elements to track focus of.
    * @param {Node=} opt_boundary Focus events are ignored outside of this node.
    * @param {FocusRow.Delegate=} opt_delegate A delegate to handle key events.
    * @param {FocusRow.Observer=} opt_observer An observer that's notified if
    *     this focus row is added to or removed from the focus order.
    * @constructor
    */
-  function FocusRow(items, opt_boundary, opt_delegate, opt_observer) {
-    /** @type {!Array.<!Element>} */
-    this.items = Array.prototype.slice.call(items);
-    assert(this.items.length > 0);
-
+  function FocusRow(opt_boundary, opt_delegate, opt_observer) {
     /** @type {!Node} */
     this.boundary_ = opt_boundary || document;
 
     /** @private {cr.ui.FocusRow.Delegate|undefined} */
     this.delegate_ = opt_delegate;
 
-    /** @private {cr.ui.FocusRow.Observer|undefined} */
+    /** @private {cr.ui.FocusRow.Observer} */
+    assert(opt_observer);
     this.observer_ = opt_observer;
 
     /** @private {!EventTracker} */
@@ -52,19 +48,11 @@ cr.define('cr.ui', function() {
     this.eventTracker_.add(cr.doc, 'focusin', this.onFocusin_.bind(this));
     this.eventTracker_.add(cr.doc, 'keydown', this.onKeydown_.bind(this));
 
-    this.items.forEach(function(item) {
-      if (item != document.activeElement)
-        item.tabIndex = -1;
-
-      this.eventTracker_.add(item, 'mousedown', this.onMousedown_.bind(this));
-    }, this);
+    /** @type {Array<string>} */
+    this.elementIds = [];
 
-    /**
-     * The index that should be actively participating in the page tab order.
-     * @type {number}
-     * @private
-     */
-    this.activeIndex_ = this.items.indexOf(document.activeElement);
+    /** @private {Element} */
+    this.rowElement_ = null;
   }
 
   /** @interface */
@@ -72,8 +60,8 @@ cr.define('cr.ui', function() {
 
   FocusRow.Delegate.prototype = {
     /**
-     * Called when a key is pressed while an item in |this.items| is focused. If
-     * |e|'s default is prevented, further processing is skipped.
+     * Called when a key is pressed while an item in |this.getItems()| is
+     * focused. If |e|'s default is prevented, further processing is skipped.
      * @param {cr.ui.FocusRow} row The row that detected a keydown.
      * @param {Event} e
      * @return {boolean} Whether the event was handled.
@@ -103,43 +91,208 @@ cr.define('cr.ui', function() {
      * @param {cr.ui.FocusRow} row The row removed from the focus order.
      */
     onDeactivate: assertNotReached,
+
+    /**
+     * Called when adding rowItems to the FocusRow to determine the element that
+     * represents the row. This should return the same element regardless of the
+     * rowItem it it called on for a specific row.
+     * @param {Element} rowItem The item to find a row element for.
+     * @return {Element} |rowItem|'s row element.
+     */
+    getRowElement: assertNotReached,
+
+    /**
+     * Called whenever there is a change in rowElement focus and the elementId
+     * is not in the FocusRow.
+     * @param {cr.ui.FocusRow} row The row that is being focused.
+     * @param {string} expectedId The id that was not found.
+     * @return {string} The id in |row| that should be focused.
+     */
+    onElementIdMiss: assertNotReached,

[Evan Stade, 2015/01/15 23:38:09]

this name is super confusing to me. It should be something like
getElementIdForMissingElementId.

Also, I think you're overloading Id here, which leads to lots of confusion. I'd
recommend not using "ID" for anything except the HTML attribute.

[hcarmona, 2015/01/16 21:39:06]

Done
Makes sense. Changed the "elementID" concept to "element code". I think "code"
doesn't have the context of "ID" where an exact match is expected. I was also
thinking about "tag" or "token", but I think both of those words also have other
meanings where "code" doesn't. What do you think?

   };
 
   FocusRow.prototype = {
-    get activeIndex() {
-      return this.activeIndex_;
+    /**
+     * @param {Element} element The element whose id is needed.
+     * @return {?string} |element|'s elementId. null if element is not in this
+     *     FocusRow.
+     */
+    getElementId: function(element) {
+      if (!this.rowElement_.contains(element))
+        return null;
+      return element.getAttribute("focus-row-element-id");
     },
-    set activeIndex(index) {
-      var wasActive = this.items[this.activeIndex_];
-      if (wasActive)
-        wasActive.tabIndex = -1;
 
-      this.items.forEach(function(item) { assert(item.tabIndex == -1); });
-      this.activeIndex_ = index;
+    /**
+     * @param {string} elementId
+     * @return {?Element} The element in this FocusRow with elementId. null if
+     *     not in this FocusRow.
+     */
+    getElement: function (elementId) {

[Evan Stade, 2015/01/15 23:38:09]

nit: extra space

[hcarmona, 2015/01/16 21:39:06]

Done.

+      var element = this.rowElement_.querySelector('[focus-row-element-id="' +
+                                                   elementId + '"]');
 
-      if (this.items[index])
-        this.items[index].tabIndex = 0;
+      // Special case when the column is the row.
+      if (!element && elementId &&
+          this.rowElement_.getAttribute('focus-row-element-id') == elementId)
+        return this.rowElement_;
 
-      if (!this.observer_)
+      return element;
+    },
+
+    /**
+     * @return {Element} The row element that contains all focusable row items.
+     */
+    getRowElement: function() {
+      return this.rowElement_;
+    },
+
+    /**
+     * @return {[Element]} An array with all row items in this row. Empty array
+     *     if nothing is focusable.
+     */
+    getItems: function() {
+      assert(this.rowElement_);
+      var items = this.rowElement_.querySelectorAll('[focus-row-element-id]');
+
+      // Special case when the column is the row.
+      if (items.length == 0 &&
+          this.rowElement_.hasAttribute('focus-row-element-id'))
+        return [ this.rowElement_ ];
+
+      return items;
+    },
+
+    /**
+     * Add an element to this FocusRow with the given elementId. No-op if either
+     * |element| or |elementId| is not provided.
+     * @param {Element} element The element that should be added.
+     * @param {string} elementId The elementId that should be used to find
+     *     similar elements in the FocusRow. This MUST be unique for each row.
+     */
+    setFocusableElementId: function(element, elementId) {
+      if (!element || !elementId)
         return;
 
-      var isActive = index >= 0 && index < this.items.length;
-      if (isActive == !!wasActive)
+      assert(this.elementIds.indexOf(elementId) == -1);
+      element.setAttribute('focus-row-element-id', elementId);
+      this.elementIds.push(elementId);
+      this.eventTracker_.add(element, 'mousedown',
+                             this.onMousedown_.bind(this));
+
+      if (!this.rowElement_)
+        this.rowElement_ = this.observer_.getRowElement(element);
+      else
+        assert(this.rowElement_ == this.observer_.getRowElement(element));
+    },
+
+    /**
+     * @param {bool} focused Whether the initial focus for this row is enabled
+     * or disabled.
+     */
+    setInitialFocus: function(focused) {
+      if (focused)
+        this.onFocusIdChange(this.elementIds[0]);
+      else
+        this.enableRowTab(false);
+    },
+
+    /**
+     * Called when focus changes to activate/deactivate the row. Focus is
+     * removed from the row when |elementId| is not in the FocusRow.
+     * @param {string} elementId The elementId that has focus.
+    */
+    onFocusIdChange: function(elementId) {
+      var element = this.getElement(elementId);
+      var rowClasses = this.rowElement_.classList;
+      var wasActive = rowClasses.contains('focus-row-active');
+
+      if (element) {
+        // Verify that the focus hasn't changed. This allows the FocusGrid to go
+        // back to the same focused element on a miss.
+        this.focusChanged_ = this.focusChanged_ ||
+                             elementId != this.lastFocusedElementId_;
+        // Keep track of the last elementId that was focused.
+        this.lastFocusedElementId_ = elementId;
+
+        this.enableRowTab(true);
+      } else if (wasActive)
+        this.enableRowTab(false);
+
+      // Only send events if the active state is different for the row.
+      if (!!element == wasActive)
         return;
 
-      if (isActive)
+      if (element) {
+        rowClasses.add('focus-row-active');
         this.observer_.onActivate(this);
-      else
+      } else {
+        rowClasses.remove('focus-row-active');
         this.observer_.onDeactivate(this);
+      }
+    },
+
+    /**
+     * Enables/disables the tabIndex of the focusable elements in the FocusRow.
+     * tabIndex can be set properly.
+     * @param {bool} allow True if tab is allowed for this row.
+     */
+    enableRowTab: function(allow) {

[dmazzoni, 2015/01/15 19:38:19]

I'd call this something like makeRowFocusable or at least enableRowTabIndex -
"tab" isn't clear

[hcarmona, 2015/01/16 21:39:06]

Done.

+      var items = this.getItems();
+      for (var i = 0; i < items.length; ++i)
+        items[i].tabIndex = allow ? 0 : -1;
     },
 
     /**
-     * Focuses the item at |index|.
-     * @param {number} index An index to focus. Must be between 0 and
-     *     this.items.length - 1.
+     * Will choose an appropriate element to focus.
+     * @param {string} elementId The element id that should be focused.
+     * @return {Element} A focusable element that best matches |elementId|.

[Evan Stade, 2015/01/15 23:38:09]

I am confused how you could "best match" an ID. IDs are generally used for exact
matches.

[hcarmona, 2015/01/16 21:39:06]

Done.

      */
-    focusIndex: function(index) {
-      this.items[index].focus();
+    getFocusableElement: function(elementId) {
+      if (!elementId)
+        return null;
+
+      /** Priority for focus is:
+       *   1. Focusable element with same elementId
+       *   2. Let the delegate decide what should be focused
+       *   3. Focus the first focusable element
+       */
+      return this.getElement(elementId) ||
+             this.getElement(this.observer_.onElementIdMiss(this, elementId)) ||
+             this.getElement(this.elementIds[0]);

[dmazzoni, 2015/01/15 19:38:19]

Just checking, this works fine on an empty list / grid?

[hcarmona, 2015/01/16 21:39:06]

An empty grid would have no rows, so it should be skipped in the tab order.
Which is good.

However, if a row has no focus-able columns this won't work. Right now, this
code is built with the assumption that a FocusRow has at least one focusable
element. I've updated the documentation at the top of this file to reflect this.

+    },
+
+    /**
+     * Called to set focus to a given row item based on the elementId. Will
+     * choose an appropriate element if |elementId| is not in the FocusRow.
+     * @param {string} elementId The element that should be focused.
+     */
+    setFocusId: function(elementId) {
+      var element = this.getFocusableElement(elementId);
+      if (element)
+        element.focus();
+    },
+
+    /** @private {string} */
+    lastFocusedElementId_: null,
+
+    /** @private {bool} */
+    focusChanged_: false,
+
+    /**
+     * Will reset the private focusChanged_ variable so that a change in focus
+     * can be tracked.
+     */
+    trackFocus: function() {
+      this.focusChanged_ = false;
+    },
+
+    /**
+     * @return {bool} Whether the column focus changed in this row since it was
+     * last asked to trackFocus().
+     */
+    focusChanged: function() {
+      return this.focusChanged_;
     },
 
     /** Call this to clean up event handling before dereferencing. */
@@ -153,7 +306,7 @@ cr.define('cr.ui', function() {
      */
     onFocusin_: function(e) {
       if (this.boundary_.contains(assertInstanceof(e.target, Node)))
-        this.activeIndex = this.items.indexOf(e.target);
+        this.onFocusIdChange(this.getElementId(e.target));
     },
 
     /**
@@ -161,29 +314,30 @@ cr.define('cr.ui', function() {
      * @private
      */
     onKeydown_: function(e) {
-      var item = this.items.indexOf(e.target);
-      if (item < 0)
+      if (!this.rowElement_.contains(e.target))
         return;
 
       if (this.delegate_ && this.delegate_.onKeydown(this, e))
         return;
 
+      var focusId = this.getElementId(e.target);
+      var elementIndex = this.elementIds.indexOf(focusId);
       var index = -1;
 
       if (e.keyIdentifier == 'Left')
-        index = item + (isRTL() ? 1 : -1);
+        index = elementIndex + (isRTL() ? 1 : -1);
       else if (e.keyIdentifier == 'Right')
-        index = item + (isRTL() ? -1 : 1);
+        index = elementIndex + (isRTL() ? -1 : 1);
       else if (e.keyIdentifier == 'Home')
         index = 0;
       else if (e.keyIdentifier == 'End')
-        index = this.items.length - 1;
+        index = this.elementIds.length - 1;
 
-      if (!this.items[index])
-        return;
-
-      this.focusIndex(index);
-      e.preventDefault();
+      focusId = this.elementIds[index];
+      if (focusId) {
+        this.setFocusId(focusId);
+        e.preventDefault();
+      }
     },
 
     /**
@@ -191,11 +345,18 @@ cr.define('cr.ui', function() {
      * @private
      */
     onMousedown_: function(e) {
+      if (!this.rowElement_.contains(e.target))
+        return;
+
       if (this.delegate_ && this.delegate_.onMousedown(this, e))
         return;
 
-      if (!e.button)
-        this.activeIndex = this.items.indexOf(e.currentTarget);
+      // Only accept the left mouse click.
+      if (!e.button) {
+        // Focus this row if the target is one of the elements in this row.
+        this.onFocusIdChange(this.getElementId(e.target));
+        e.preventDefault();
+      }
     },
   };