Make downloads list keyboard shortcuts more consistent.

chrome/browser/resources/downloads/downloads.js

Index: chrome/browser/resources/downloads/downloads.js
diff --git a/chrome/browser/resources/downloads/downloads.js b/chrome/browser/resources/downloads/downloads.js
index fd7ccc9e6dee71dd48d02317fc2fb38d95c60fa6..ab49cf27698ff1872562e0fc3dfe6f24b6efa968 100644
--- a/chrome/browser/resources/downloads/downloads.js
+++ b/chrome/browser/resources/downloads/downloads.js
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 // TODO(jhawkins): Use hidden instead of showInline* and display:none.
+// TODO(hcarmona): This file is big: it may be good to split it up.
 
 /**
  * The type of the download object. The definition is based on
@@ -77,6 +78,113 @@ function createButton(onclick, value) {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// DownloadFocusRow:
+
+/**
+ * Provides an implementation for a single column grid.
+ * @constructor
+ * @extends {cr.ui.FocusRow}
+ */
+DownloadFocusRow = function() {};

[Dan Beam, 2015/01/27 22:32:23]

function DownloadFocusRow() {}

is the more typical style

[hcarmona, 2015/01/29 00:00:37]

Done.

+
+/**
+ * Decorates |focusRow| so that it can be treated as a DownloadFocusRow.
+ * @param {Element} focusRow The element that has all the columns represented
+ *     by |download|.
+ * @param {Download} download The Download representing this row.
+ * @param {Node} boundary Focus events are ignored outside of this node.
+ */
+DownloadFocusRow.decorate = function(focusRow, download, boundary) {
+  focusRow.__proto__ = DownloadFocusRow.prototype;
+  focusRow.decorate(boundary);
+
+  // Add all clickable elements as a row into the grid.
+  focusRow.addFocusRow_(download.nodeFileLink_, 'name');
+  focusRow.addFocusRow_(download.nodeURL_, 'url');
+  focusRow.addFocusRow_(download.controlShow_, 'show');
+  focusRow.addFocusRow_(download.controlRetry_, 'retry');
+  focusRow.addFocusRow_(download.controlPause_, 'pause');
+  focusRow.addFocusRow_(download.controlResume_, 'resume');
+  focusRow.addFocusRow_(download.controlRemove_, 'remove');
+  focusRow.addFocusRow_(download.controlCancel_, 'cancel');
+
+  // Only one set of these two buttons will be added to a download row at a
+  // time. They have the same type so that focus is handled well in case
+  // there is a dangerous download right next to a malicious download.
+  focusRow.addFocusRow_(download.malwareSave_, 'save');
+  focusRow.addFocusRow_(download.malwareDiscard_, 'discard');
+  focusRow.addFocusRow_(download.dangerSave_, 'save');
+  focusRow.addFocusRow_(download.dangerDiscard_, 'discard');
+
+  focusRow.addFocusRow_(download.controlByExtensionLink_, 'extension');
+};
+
+DownloadFocusRow.prototype = {
+  __proto__: cr.ui.FocusRow.prototype,
+
+  /** @override */
+  getEquivalentElement: function(element) {
+    if (this.contains(element))
+      return element;
+
+    // All elements default to another element with the same type.

[Dan Beam, 2015/01/27 22:32:23]

nit: arguably

var columnType = element.getAttribute('column-type');
... re-use columnType ...

[hcarmona, 2015/01/29 00:00:37]

Done.

+    var query = '[column-type=' + element.getAttribute('column-type') + ']';
+    var ret = this.querySelector(query);

[Dan Beam, 2015/01/27 22:32:23]

nit: ret -> el, element, equiv, equivalent (all seem better)

[hcarmona, 2015/01/29 00:00:37]

Done.

+
+    if (!ret) {
+      // The following element types are at the same UI level.
+      var grayLinks = ['show', 'retry', 'pause', 'resume', 'remove', 'cancel'];

[Dan Beam, 2015/01/27 22:32:23]

nit: grayLinks -> equivalents (or rename so this isn't based on the
presentation)

[hcarmona, 2015/01/29 00:00:37]

Done.

+      if (grayLinks.indexOf(element.getAttribute('column-type')) != -1) {
+        ret = this.querySelector(
+            '[column-type=show], [column-type=retry], ' +
+            '[column-type=pause], [column-type=resume], ' +
+            '[column-type=remove], [column-type=cancel]');

[Dan Beam, 2015/01/27 22:32:23]

why are we duplicating this list of types?

[hcarmona, 2015/01/29 00:00:37]

Done. Not duplicated anymore.

+      }
+    }
+
+    // Any other miss should return the first focusable element.

[Dan Beam, 2015/01/27 22:32:23]

nit: // Return the first focusable element if no equivalent type is found.

[hcarmona, 2015/01/29 00:00:37]

Done.

+    return ret || this.focusableElements[0];
+  },
+
+  /**
+   * Add an element if it exists in this FocusRow and is focusable.
+   * @param {Element} element The element that should be added.
+   * @param {string} type The type to use for the element.

[Dan Beam, 2015/01/27 22:32:23]

can we make these doc strings more useful or remove?

[hcarmona, 2015/01/29 00:00:37]

Done.

+   * @private
+   */
+  addFocusRow_: function(element, type) {
+    if (this.shouldFocus_(element)) {
+      this.addFocusableElement(element);
+      element.setAttribute('column-type', type);
+    }
+  },
+
+  /**
+   * Determines if element should be focusable.
+   * @param {!Element} element
+   * @return {boolean}
+   * @private
+   */
+  shouldFocus_: function(element) {
+    if (!element)
+      return false;
+
+    // Hidden elements are not focusable.
+    var style = window.getComputedStyle(element);
+    if (style.visibility == 'hidden' || style.display == 'none')

[Dan Beam, 2015/01/27 22:32:23]

here's the full amount of checks that blink does, btw:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[hcarmona, 2015/01/29 00:00:37]

This check is sufficient for the downloads page. Do we need to expand it to all
possibilities? If so, can blink provide this information (to avoid duplicating
code)? Or is it better to rename this function to make it clear that
shouldFocus_ applies only to downloads?

+      return false;
+
+    // Verify that all ancestors are focusable. This is necessary because of the
+    // case where a node has display set to something other than 'none', but it
+    // has an ancestor with display == 'none'.
+    if (element.parentElement)
+      return this.shouldFocus_(element.parentElement);
+
+    return true;

[Dan Beam, 2015/01/27 22:32:23]

nit:

  // Verify all ancestors are focusable.
  return !element.parentNode || this.shouldFocus_(element.parentNode);

[hcarmona, 2015/01/29 00:00:37]

Done.

+  },
+};
+
+///////////////////////////////////////////////////////////////////////////////
 // Downloads
 /**
  * Class to hold all the information about the visible downloads.
@@ -91,6 +199,7 @@ function Downloads() {
   this.node_ = $('downloads-display');
   this.summary_ = $('downloads-summary-text');
   this.searchText_ = '';
+  this.focusGrid_ = new cr.ui.FocusGrid();
 
   // Keep track of the dates of the newest and oldest downloads so that we
   // know where to insert them.
@@ -176,6 +285,24 @@ Downloads.prototype.updateResults = function() {
 
   if (loadTimeData.getBoolean('allow_deleting_history'))
     $('clear-all').hidden = !hasDownloads || this.searchText_.length > 0;
+
+  this.rebuildFocusGrid_();
+};
+
+/**
+ * Rebuild the focusGrid_ using the elements that each download will have.
+ * @private
+ */
+Downloads.prototype.rebuildFocusGrid_ = function() {
+  this.focusGrid_.destroy();
+
+  // Keys for downloads are in the reverse order of what is displayed.
+  var keys = Object.keys(this.downloads_);

[Dan Beam, 2015/01/27 22:32:23]

I'm not sure that the order of keys returned from Object.keys() is defined, so
you may want to sort to find another way to get a strong guarantee if it
matters.

[hcarmona, 2015/01/29 00:00:37]

The order does matter. Updated code so that it is not dependent on the keys and
instead the grid is based on querying for the downloads from the document.

+  for (var i = keys.length - 1; i >= 0; --i) {
+    var download = this.downloads_[keys[i]];
+    DownloadFocusRow.decorate(download.node, download, this.node_);

[Dan Beam, 2015/01/27 22:32:23]

are all the rows always blasted away and rebuilt?  otherwise it seems like these
rows might be (harmfully or wastefully) re-decorated here.

[hcarmona, 2015/01/29 00:00:37]

Changed so that rows are only decorated once.

+    this.focusGrid_.addRow(download.node);
+  }
 };
 
 /**