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..98201aea44f189a036c4a982a3adfe26e8d214e3 100644
--- a/chrome/browser/resources/downloads/downloads.js
+++ b/chrome/browser/resources/downloads/downloads.js
@@ -91,6 +91,8 @@ function Downloads() {
   this.node_ = $('downloads-display');
   this.summary_ = $('downloads-summary-text');
   this.searchText_ = '';
+  this.focusGrid_ = new cr.ui.FocusGrid(this.node_,
+                                        new DownloadFocusObserver());
 
   // Keep track of the dates of the newest and oldest downloads so that we
   // know where to insert them.
@@ -176,6 +178,21 @@ Downloads.prototype.updateResults = function() {
 
   if (loadTimeData.getBoolean('allow_deleting_history'))
     $('clear-all').hidden = !hasDownloads || this.searchText_.length > 0;
+
+  var grid = [];
+  for (var node = this.node_.firstChild; node; node = node.nextSibling) {
+    // Add all clickable elements as a row into the grid.
+    grid.push(node.getElementsByClassName('row-item'));

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

use querySelectorAll('.row-item') unless you want all of these to change when
row-item classes are added/re oved from the DOM

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

Elements are now added explicitly, so there's no need to query for them.

+  }
+  this.focusGrid_.setGrid(grid);
+};
+
+/**
+ * Decorate elem so that it can be added to the focusGrid_.
+ * @param {!Element} elem The element that should be decorated.
+ */
+Downloads.makeFocusable = function(elem) {

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

Maybe makePartOfFocusGrid or addFocusGridItemClass?

makeFocusable makes me think it's going to set the tabIndex.

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

what happens if this class is added after the focusgrid has been created?  how
does it get updated?  can we add an assert or a rebuildFocusGrid_ or something
here to make sure?

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

Created the rebuildFocusGrid_ method to make it more explicit when the grid is
rebuilt. It doesn't make sense to call it inside this method because this is
called multiple times while building a single download. rebuildFocusGrid_ will
be called in updateResults.

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

Done.

+  elem.classList.add('row-item');
 };
 
 /**
@@ -304,6 +321,36 @@ Downloads.prototype.onCommand_ = function(e) {
 };
 
 ///////////////////////////////////////////////////////////////////////////////
+// DownloadFocusObserver
+
+/**
+ * @constructor
+ * @implements {cr.ui.FocusRow.Observer}
+ */
+function DownloadFocusObserver() {}

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

you could probably make the HistoryFocusObserver into a more generic
ActiveFocusObserver or something like that (that just takes an ancestor class
and active class to apply)

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

Observer will now handle the case when a column misses. This will allow any page
to handle a miss any way they want to.

+
+DownloadFocusObserver.prototype = {
+  /** @override */
+  onActivate: function(row) {
+    this.getActiveRowElement_(row).classList.add('active');
+  },
+
+  /** @override */
+  onDeactivate: function(row) {
+    this.getActiveRowElement_(row).classList.remove('active');
+  },
+
+  /**
+   * @param {cr.ui.FocusRow} row The row to find an element for.
+   * @return {Element} |row|'s "active" element.
+   * @private
+   */
+  getActiveRowElement_: function(row) {
+    return findAncestorByClass(row.items[0], 'download');
+  },
+};
+
+///////////////////////////////////////////////////////////////////////////////
 // Download
 /**
  * A download and the DOM representation for that download.
@@ -354,6 +401,7 @@ function Download(download) {
   this.nodeFileLink_ = createActionLink(this.openFile_.bind(this));
   this.nodeFileLink_.className = 'name';
   this.nodeFileLink_.style.display = 'none';
+  Downloads.makeFocusable(this.nodeFileLink_);
   this.nodeTitleArea_.appendChild(this.nodeFileLink_);
 
   this.nodeFileName_ = createElementWithClassName('span', 'name');
@@ -368,6 +416,7 @@ function Download(download) {
 
   this.nodeURL_ = createElementWithClassName('a', 'src-url');
   this.nodeURL_.target = '_blank';
+  Downloads.makeFocusable(this.nodeURL_);
   nodeURLDiv.appendChild(this.nodeURL_);
 
   // Controls.
@@ -379,6 +428,7 @@ function Download(download) {
   if (loadTimeData.valueExists('control_showinfolder')) {
     this.controlShow_ = createActionLink(this.show_.bind(this),
         loadTimeData.getString('control_showinfolder'));
+    Downloads.makeFocusable(this.controlShow_);
     this.nodeControls_.appendChild(this.controlShow_);
   } else {
     this.controlShow_ = null;
@@ -387,26 +437,31 @@ function Download(download) {
   this.controlRetry_ = document.createElement('a');
   this.controlRetry_.download = '';
   this.controlRetry_.textContent = loadTimeData.getString('control_retry');
+  Downloads.makeFocusable(this.controlRetry_);
   this.nodeControls_.appendChild(this.controlRetry_);
 
   // Pause/Resume are a toggle.
   this.controlPause_ = createActionLink(this.pause_.bind(this),
       loadTimeData.getString('control_pause'));
+  Downloads.makeFocusable(this.controlPause_);
   this.nodeControls_.appendChild(this.controlPause_);
 
   this.controlResume_ = createActionLink(this.resume_.bind(this),
       loadTimeData.getString('control_resume'));
+  Downloads.makeFocusable(this.controlResume_);
   this.nodeControls_.appendChild(this.controlResume_);
 
   if (loadTimeData.getBoolean('allow_deleting_history')) {
     this.controlRemove_ = createActionLink(this.remove_.bind(this),
         loadTimeData.getString('control_removefromlist'));
     this.controlRemove_.classList.add('control-remove-link');
+    Downloads.makeFocusable(this.controlRemove_);
     this.nodeControls_.appendChild(this.controlRemove_);
   }
 
   this.controlCancel_ = createActionLink(this.cancel_.bind(this),
       loadTimeData.getString('control_cancel'));
+  Downloads.makeFocusable(this.controlCancel_);
   this.nodeControls_.appendChild(this.controlCancel_);
 
   this.controlByExtension_ = document.createElement('span');
@@ -428,10 +483,12 @@ function Download(download) {
   this.malwareSave_ = createActionLink(
       this.saveDangerous_.bind(this),
       loadTimeData.getString('danger_restore'));
+  Downloads.makeFocusable(this.malwareSave_);
   this.malwareNodeControls_.appendChild(this.malwareSave_);
   this.malwareDiscard_ = createActionLink(
       this.discardDangerous_.bind(this),
       loadTimeData.getString('control_removefromlist'));
+  Downloads.makeFocusable(this.malwareDiscard_);
   this.malwareNodeControls_.appendChild(this.malwareDiscard_);
   this.danger_.appendChild(this.malwareNodeControls_);
 
@@ -646,6 +703,7 @@ Download.prototype.update = function(download) {
       this.controlByExtensionLink_.href =
           'chrome://extensions#' + this.byExtensionId_;
       this.controlByExtensionLink_.textContent = this.byExtensionName_;
+      Downloads.makeFocusable(this.controlByExtensionLink_);
       this.controlByExtension_.appendChild(this.controlByExtensionLink_);
       if (slugIndex < (formatted.length - slug.length))
         this.controlByExtension_.appendChild(document.createTextNode(