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..a925ae3ec2b9a96c3b757e5cb1426fec1eec0ae2 100644
--- a/chrome/browser/resources/downloads/downloads.js
+++ b/chrome/browser/resources/downloads/downloads.js
@@ -77,6 +77,122 @@ function createButton(onclick, value) {
 }
 
 ///////////////////////////////////////////////////////////////////////////////
+// DownloadFocusRow:
+
+/**
+ * Provides an implementation for a single column grid.
+ * @constructor
+ * @param {Download} download The Download representing this row.
+ * @param {Node} boundary Focus events are ignored outside of this node.
+ * @extends {cr.ui.FocusRow}
+ */
+DownloadFocusRow = function(download, boundary) {
+  var self = download.node;
+  self.__proto__ = DownloadFocusRow.prototype;
+  self.download_ = download;
+  self.decorate(boundary);
+  return self;

[Dan Beam, 2015/01/24 02:37:07]

what's the point to this pattern of returning something instead of the default
|this| that a constructor function typically returns automatically?

[hcarmona, 2015/01/27 18:30:39]

I had seen this in the code when a constructor would create an element to
represent the object. Although none of those were decorating an element that was
passed in. I changed this to use a static decorate method so that it's more
clear that the element is being decorated rather than a new object being
created.

+};
+
+DownloadFocusRow.prototype = {
+  __proto__: cr.ui.FocusRow.prototype,
+
+  /** @override */
+  decorate: function(boundary, opt_delegate) {
+    cr.ui.FocusRow.prototype.decorate.apply(this, arguments);
+
+    // Add all clickable elements as a row into the grid.
+    this.addFocusRow_(this.download_.nodeFileLink_, 'name');

[Dan Beam, 2015/01/24 02:37:08]

i understand @private is file-level in closure, but this file is way bigger than
it should be (arguably it should be split a few ways).

can you add a TODO here to split these classes into more files and use accessors
for these private members (e.g. nodeURL_) instead?

[hcarmona, 2015/01/27 18:30:39]

Done.

+    this.addFocusRow_(this.download_.nodeURL_, 'url');
+    this.addFocusRow_(this.download_.controlShow_, 'show');
+    this.addFocusRow_(this.download_.controlRetry_, 'retry');
+    this.addFocusRow_(this.download_.controlPause_, 'pause');
+    this.addFocusRow_(this.download_.controlResume_, 'resume');
+    this.addFocusRow_(this.download_.controlRemove_, 'remove');
+    this.addFocusRow_(this.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.
+    this.addFocusRow_(this.download_.malwareSave_, 'save');
+    this.addFocusRow_(this.download_.malwareDiscard_, 'discard');
+    this.addFocusRow_(this.download_.dangerSave_, 'save');
+    this.addFocusRow_(this.download_.dangerDiscard_, 'discard');
+
+    this.addFocusRow_(this.download_.controlByExtensionLink_, 'extension');
+  },
+
+  /** @override */
+  getEquivalentElement: function(element) {
+    if (this.contains(element))
+      return element;
+
+    // All elements default to another element with the same type.
+    var query = '[column-type="' + element.getAttribute('column-type') + '"]';
+    var ret = this.querySelector(query);
+
+    // The following element types are at the same UI level.
+    if (!ret) {
+      switch (element.getAttribute('column-type')) {

[Dan Beam, 2015/01/24 02:37:08]

what is this entire switch trying to accomplish?

[hcarmona, 2015/01/27 18:30:39]

Got rid of switch to make it more clear what's happening.

There are a few UI elements that are at the same level visually, so the first
available one should retain focus.

+        case ('show'):

[Dan Beam, 2015/01/24 02:37:07]

no parens

  case 'show':

[hcarmona, 2015/01/27 18:30:39]

Done.

+        case ('retry'):
+        case ('pause'):
+        case ('resume'):
+        case ('remove'):
+        case ('cancel'):
+          ret = this.querySelector('[column-type="show"]') ||
+                this.querySelector('[column-type="retry"]') ||
+                this.querySelector('[column-type="pause"]') ||
+                this.querySelector('[column-type="resume"]') ||
+                this.querySelector('[column-type="remove"]') ||
+                this.querySelector('[column-type="cancel"]');

[Dan Beam, 2015/01/24 02:37:07]

querySelector('[column-type=show], [column-type=retry]...')

[hcarmona, 2015/01/27 18:30:39]

Done.

+          break;
+      }
+    }
+
+    // Any other miss should return the first focusable element.
+    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.
+   * @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')
+      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;
+  },
+};
+
+///////////////////////////////////////////////////////////////////////////////
 // Downloads
 /**
  * Class to hold all the information about the visible downloads.
@@ -91,6 +207,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 +293,23 @@ 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_);
+  for (var i = keys.length - 1; i >= 0; --i) {
+    this.focusGrid_.addRow(new DownloadFocusRow(this.downloads_[keys[i]],
+                                                this.node_));
+  }
 };
 
 /**