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..2b5eff005acb2d2e55ceb4ade32d76f8ee2fb5b7 100644
--- a/ui/webui/resources/js/cr/ui/focus_row.js
+++ b/ui/webui/resources/js/cr/ui/focus_row.js
@@ -11,11 +11,13 @@ cr.define('cr.ui', function() {
    *
    * One could create a FocusRow by doing:
    *
-   *   new cr.ui.FocusRow([checkboxEl, labelEl, buttonEl])
+   *   var focusRow = new cr.ui.FocusRow(rowBoundary, rowEl);
    *
-   * if there are references to each node or querying them from the DOM like so:
+   *   focusRow.setFocusableElement(checkboxEl);
+   *   focusRow.setFocusableElement(labelEl);
+   *   focusRow.setFocusableElement(buttonEl);
    *
-   *   new cr.ui.FocusRow(dialog.querySelectorAll('list input[type=checkbox]'))
+   *   focusRow.setInitialFocusability(true);
    *
    * Pressing left cycles backward and pressing right cycles forward in item
    * order. Pressing Home goes to the beginning of the list and End goes to the
@@ -23,48 +25,31 @@ cr.define('cr.ui', function() {
    *
    * If an item in this row is focused, it'll stay active (accessible via tab).
    * If no items in this row are focused, the row can stay active until focus
-   * changes to a node inside |this.boundary_|. If opt_boundary isn't
-   * specified, any focus change deactivates the row.
+   * changes to a node inside |this.boundary_|. If |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 {Node} boundary Focus events are ignored outside of this node.
+   * @param {Element} rowElement The element representing this row.
    * @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(boundary, rowElement, opt_delegate) {
     /** @type {!Node} */
-    this.boundary_ = opt_boundary || document;
+    this.boundary_ = boundary || document;
 
-    /** @private {cr.ui.FocusRow.Delegate|undefined} */
-    this.delegate_ = opt_delegate;
+    /** @type {Element} */
+    this.rowElement_ = rowElement;
 
-    /** @private {cr.ui.FocusRow.Observer|undefined} */
-    this.observer_ = opt_observer;
+    /** @type {cr.ui.FocusRow.Delegate|undefined} */
+    this.delegate = opt_delegate;
 
     /** @private {!EventTracker} */
     this.eventTracker_ = new EventTracker;
     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);
-
-    /**
-     * The index that should be actively participating in the page tab order.
-     * @type {number}
-     * @private
-     */
-    this.activeIndex_ = this.items.indexOf(document.activeElement);
+    /** @type {Array<Element>} */
+    this.rowElements = [];

[Evan Stade, 2015/01/22 16:55:11]

I can't figure out what this is or what this.rowElement is. Is this the elements
within the row, and rowElement is the row itself? Confusing... needs better
names. rowElement_ should be equivalent to |this| (via decorate)

[hcarmona, 2015/01/24 01:24:19]

|this| is now what rowElement_ used to be.

Renamed |rowElements| to |focusableElements| to make the name more clear.

   }
 
   /** @interface */
@@ -72,8 +57,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.rowElements| 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.
@@ -88,58 +73,111 @@ cr.define('cr.ui', function() {
     onMousedown: assertNotReached,
   };
 
-  /** @interface */
-  FocusRow.Observer = function() {};
-
-  FocusRow.Observer.prototype = {
+  FocusRow.prototype = {
     /**
      * Called when the row is activated (added to the focus order).
-     * @param {cr.ui.FocusRow} row The row added to the focus order.
      */
-    onActivate: assertNotReached,
+    onActivate: function() {},
 
     /**
      * Called when the row is deactivated (removed from the focus order).
-     * @param {cr.ui.FocusRow} row The row removed from the focus order.
      */
-    onDeactivate: assertNotReached,
-  };
+    onDeactivate: function() {},
 
-  FocusRow.prototype = {
-    get activeIndex() {
-      return this.activeIndex_;
-    },
-    set activeIndex(index) {
-      var wasActive = this.items[this.activeIndex_];
-      if (wasActive)
-        wasActive.tabIndex = -1;
+    /**
+     * Find the element that best matches |sampleElement|. This function should
+     * NOT return null or undefined.
+     * @param {Element} sampleElement An element that needs an equivalent
+     *     element in this row.
+     * @return {!Element} The element that best matches sampleElement.
+     */
+    getEquivalentElement: assertNotReached,
 
-      this.items.forEach(function(item) { assert(item.tabIndex == -1); });
-      this.activeIndex_ = index;
+    /**
+     * Return the only element matching the query or null if not found.
+     * @param {string} query The query to find the element in this FocusRow.
+     * @return {Element} Either null or the element matching the query.
+     */
+    getMatch: function(query) {
+      var elements = this.rowElement_.querySelectorAll(query);
 
-      if (this.items[index])
-        this.items[index].tabIndex = 0;
+      // There should be exactly 0 or 1 matches.
+      assert(elements.length == 0 || elements.length == 1);
 
-      if (!this.observer_)
-        return;
+      return elements[0] || null;
+    },
 
-      var isActive = index >= 0 && index < this.items.length;
-      if (isActive == !!wasActive)
+    /**
+     * Add an element to this FocusRow. No-op if |element| is not provided.
+     * @param {Element} element The element that should be added.
+     */
+    setFocusableElement: function(element) {

[Evan Stade, 2015/01/22 16:55:11]

addFocusableElement

[hcarmona, 2015/01/24 01:24:19]

Done.

+      if (!element)
         return;
 
-      if (isActive)
-        this.observer_.onActivate(this);
-      else
-        this.observer_.onDeactivate(this);
+      assert(this.rowElements.indexOf(element) == -1);
+      assert(this.contains(element));
+
+      this.rowElements.push(element);
+      this.eventTracker_.add(element, 'mousedown',
+                             this.onMousedown_.bind(this));
+    },
+
+    /**
+     * Called when focus changes to activate/deactivate the row. Focus is
+     * removed from the row when |element| is not in the FocusRow.
+     * @param {Element} element The element that has focus. null if focus should
+     *     be removed.
+     * @private
+    */
+    onFocusChange_: function(element) {
+      var isActive = this.contains(element);
+      var wasActive = this.rowElement_.classList.contains('focus-row-active');
+
+      // Only send events if the active state is different for the row.
+      if (isActive != wasActive)
+        this.makeRowFocusable(isActive);
+    },
+
+    /**
+     * 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.
+     */
+    makeRowFocusable: function(allow) {

[Evan Stade, 2015/01/22 16:55:11]

nit: more like makeRowActive

[hcarmona, 2015/01/24 01:24:19]

Done.

+      this.rowElements.forEach(function(element) {
+        element.tabIndex = allow ? 0 : -1;
+      });
+
+      if (allow) {
+        this.rowElement_.classList.add('focus-row-active');
+        this.onActivate(this);

[Evan Stade, 2015/01/22 16:55:10]

I don't think you need to pass |this|

[hcarmona, 2015/01/24 01:24:19]

Done.

+      }

[Evan Stade, 2015/01/22 16:55:11]

nit: } else {

[hcarmona, 2015/01/24 01:24:19]

Done.

+      else {
+        this.rowElement_.classList.remove('focus-row-active');
+        this.onDeactivate(this);

[Evan Stade, 2015/01/22 16:55:10]

can you combine Activate and Deactivate? Then this code is:

this.rowElement_.classList.toggle('focus-row-active', allow);
this.onActiveStateChanged();

[hcarmona, 2015/01/24 01:24:20]

Done.

+      }
+    },
+
+    /**
+     * @param {Element} target The target we need to test.
+     * @return {bool} Whether or not the target is in this FocusRow.
+     */
+    contains: function(target) {
+      return this.rowElement_.contains(target);
     },
 
     /**
-     * Focuses the item at |index|.
-     * @param {number} index An index to focus. Must be between 0 and
-     *     this.items.length - 1.
+     * Called to set focus to a given row item based on the element.
+     * @param {Element} element The element that focus should be based on.

[Evan Stade, 2015/01/22 16:55:11]

nit: "An element from a row of the same type which previously held focus."

[hcarmona, 2015/01/24 01:24:19]

Done.

      */
-    focusIndex: function(index) {
-      this.items[index].focus();
+    focusEquivalentElement: function(element) {

[Evan Stade, 2015/01/22 16:55:11]

this whole function seems a bit extraneous. The callsite could just be
this.getEquivalentElement(element).focus();

[hcarmona, 2015/01/24 01:24:19]

Done.

+      var bestMatch = element;
+
+      if (!this.contains(bestMatch))
+        bestMatch = this.getEquivalentElement(element);
+
+      bestMatch.focus();
     },
 
     /** Call this to clean up event handling before dereferencing. */
@@ -153,37 +191,38 @@ cr.define('cr.ui', function() {
      */
     onFocusin_: function(e) {
       if (this.boundary_.contains(assertInstanceof(e.target, Node)))
-        this.activeIndex = this.items.indexOf(e.target);
+        this.onFocusChange_(e.target);
     },
 
     /**
-     * @param {Event} e A focus event.
+     * @param {Event} e The keydown event.
      * @private
      */
     onKeydown_: function(e) {
-      var item = this.items.indexOf(e.target);
-      if (item < 0)
+      if (!this.contains(e.target))
         return;
 
-      if (this.delegate_ && this.delegate_.onKeydown(this, e))
+      if (this.delegate && this.delegate.onKeydown(this, e))
         return;
 
+      var element = this.getEquivalentElement(e.target);

[Evan Stade, 2015/01/22 16:55:11]

I'm confused about what this is doing, probably because |element| is not a
descriptive variable name (and it's reused below).

I think this function needs better docs to indicate it only handles in-row focus
changes.

[hcarmona, 2015/01/24 01:24:19]

Removed element because it will always equal e.target so it doesn't make sense
to try and find a match.

Renamed where it was reused to elementToFocus to make the intention more clear.
Done.

+      var elementIndex = this.rowElements.indexOf(element);
       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;
-
-      if (!this.items[index])
-        return;
+        index = this.rowElements.length - 1;
 
-      this.focusIndex(index);
-      e.preventDefault();
+      element = this.rowElements[index];
+      if (element) {
+        this.focusEquivalentElement(element);
+        e.preventDefault();
+      }
     },
 
     /**
@@ -191,11 +230,14 @@ cr.define('cr.ui', function() {
      * @private
      */
     onMousedown_: function(e) {
-      if (this.delegate_ && this.delegate_.onMousedown(this, e))
+      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.onFocusChange_(e.target);
+      }
     },
   };