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..317076dbebaf18afc60a016a67b1f02226e94871 100644
--- a/ui/webui/resources/js/cr/ui/focus_row.js
+++ b/ui/webui/resources/js/cr/ui/focus_row.js
@@ -88,6 +88,20 @@ cr.define('cr.ui', function() {
     onMousedown: assertNotReached,
   };
 
+  /**
+   * Determines if element should be focusable.
+   * @param {!Element} element
+   */
+  FocusRow.isFocusable = function(element) {

[Dan Beam, 2014/12/22 23:19:59]

why is this public?  and static?

[hcarmona, 2015/01/13 00:04:40]

Made private and moved to the downloads page to be used in determining whether
to add an element to the FocusRow.

+    var style = window.getComputedStyle(element);

[Dan Beam, 2014/12/22 23:19:59]

getComputedStyle already takes the entire hierarchy into account -- no need to
traverse

[hcarmona, 2015/01/13 00:04:41]

This doesn't work for display because display:none is added to a parent element
but the child element has display:block set. This means that even though the
element has display:block, it's hidden because its parent has display:none and
it doesn't look like there's a concept of priority other than the most specific
rule wins.

+    if (style.visibility == 'hidden' || style.display == 'none')

[Dan Beam, 2014/12/22 23:19:59]

what about tabindex < 0?

[hcarmona, 2015/01/13 00:04:41]

This will be used to update the tabIndex. All elements should have a tabIndex of
0 when a row is focused and -1 when they're not focused. This allows TAB to be
used to traverse elements in the selected row and skip out of the list when
tabbing after the last element.

+      return false;
+
+    if (element.parentElement)

[Dan Beam, 2014/12/22 23:19:59]

this can also be null when the element's not in the DOM (not just at the top of
the tree)

[hcarmona, 2015/01/13 00:04:40]

Done.

+      return FocusRow.isFocusable(element.parentElement);
+    return true;
+  };
+
   /** @interface */
   FocusRow.Observer = function() {};
 
@@ -109,16 +123,24 @@ cr.define('cr.ui', function() {
     get activeIndex() {
       return this.activeIndex_;
     },
+
+    /**
+     * Set the active index and update focusability. An index out of range will
+     * make all items not focusable if the row was previously focused.
+     */
     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;
+      if (this.items[index]) {
+        this.items.forEach(function(item) {
+          if (FocusRow.isFocusable(item))
+            item.tabIndex = 0;
+        }.bind(this));

[Dan Beam, 2014/12/22 23:19:59]

pass in a thisArg instead of .bind()ing

[hcarmona, 2015/01/13 00:04:41]

bind is left over from previous code. Removed it.

+      }
+      else if (wasActive)

[dmazzoni, 2014/12/22 23:14:07]

Formatting nit: the "else" should be on the same line as the "{", and the braces
around the "else" block are required if you used them for the "if" block, e.g.:

if (foo) {
  bar;
} else {
  baz;
}

[Dan Beam, 2014/12/22 23:19:59]

} else if (...) {

[hcarmona, 2015/01/13 00:04:41]

Done.

[hcarmona, 2015/01/13 00:04:41]

Done.

+        this.items.forEach(function(item) { item.tabIndex = -1; });
 
-      if (this.items[index])
-        this.items[index].tabIndex = 0;
+      this.activeIndex_ = index;
 
       if (!this.observer_)
         return;
@@ -133,13 +155,52 @@ cr.define('cr.ui', function() {
         this.observer_.onDeactivate(this);
     },
 
+    /** @private */
+    previousIndex_: 0,

[Dan Beam, 2014/12/22 23:19:59]

not very explanatory here...

[hcarmona, 2015/01/13 00:04:40]

Code changed so this is no longer necessary.

+
     /**
      * Focuses the item at |index|.

[dmazzoni, 2014/12/22 23:14:07]

Update these docs?

[hcarmona, 2015/01/13 00:04:41]

Code changed so this is no longer necessary.

      * @param {number} index An index to focus. Must be between 0 and
      *     this.items.length - 1.
      */
     focusIndex: function(index) {

[Dan Beam, 2014/12/22 23:19:59]

change from index to ID like bondd@ did in settings (and allow specifying a
column ID for each type of field)

[hcarmona, 2015/01/13 00:04:41]

Done.

+      if (!FocusRow.isFocusable(this.items[index])) {
+        // Find first focusable element smaller than the invalid index.
+        var smallIndex = index;
+        for (smallIndex; smallIndex >= 0; --smallIndex) {
+          if (FocusRow.isFocusable(this.items[smallIndex]))
+            break;
+        }
+
+        // Find first focusable element larger than the invalid index.
+        var largeIndex = index;
+        for (largeIndex; largeIndex < this.items.length; ++largeIndex) {
+          if (FocusRow.isFocusable(this.items[largeIndex]))
+            break;
+        }
+
+        if (smallIndex > -1 && largeIndex < this.items.length) {
+          // Can focus in either direction. Give preference to the direction
+          // away from previousIndex_.
+          if (this.previousIndex_ > index)
+            index = smallIndex;
+          else
+            index = largeIndex;
+        } else if (smallIndex > -1) {
+          // Can only focus smaller.
+          index = smallIndex;
+        } else if (largeIndex < this.items.length) {
+          // Can only focus larger.
+          index = largeIndex;
+        } else {
+          // This code path shouldn't be executed because it means we have a row
+          // with nothing focusable.
+          assert(false);

[Dan Beam, 2014/12/22 23:19:59]

assertNotReached()

[hcarmona, 2015/01/13 00:04:41]

Done.

+        }
+      }
+
       this.items[index].focus();
+      this.previousIndex_ = index;

[Dan Beam, 2014/12/22 23:19:59]

previouslyFocusedIndex_?

[hcarmona, 2015/01/13 00:04:41]

Code has changed and this is different now.

     },
 
     /** Call this to clean up event handling before dereferencing. */