Make downloads list keyboard shortcuts more consistent.

chrome/browser/resources/history/history.js

Index: chrome/browser/resources/history/history.js
diff --git a/chrome/browser/resources/history/history.js b/chrome/browser/resources/history/history.js
index a71592de4ee5d70587681b99c7b513c00db04f3a..95336126dd72b60ef5dd966225b0c5c9a4c34f88 100644
--- a/chrome/browser/resources/history/history.js
+++ b/chrome/browser/resources/history/history.js
@@ -250,6 +250,8 @@ Visit.prototype.getResultDOM = function(propertyBag) {
       e.preventDefault();
     }.bind(this));
   }
+  if (focusless)
+    bookmarkSection.tabIndex = -1;
   entryBox.appendChild(bookmarkSection);
 
   var visitEntryWrapper = /** @type {HTMLElement} */(
@@ -884,69 +886,88 @@ HistoryModel.prototype.getGroupByDomain = function() {
 };
 
 ///////////////////////////////////////////////////////////////////////////////
-// HistoryFocusObserver:
+// HistoryFocusRow:
 
 /**
- * @constructor
- * @implements {cr.ui.FocusRow.Observer}
+ * Provides an implementation for a single column grid.
+ * @param {Node} boundary Focus events are ignored outside of this node.
+ * @param {Element} rowElement The element representing this row.
+ * @extends {cr.ui.FocusRow}
  */
-function HistoryFocusObserver() {}
+HistoryFocusRow = function(boundary, rowElement) {
+  cr.ui.FocusRow.apply(this, arguments);
+
+  /**
+   * Both of these elements are checkboxes and a history focusRow will only
+   * have 1 of the two. By giving them both the same type we can ensure
+   * that a checkbox remains focused when focus changes.
+   */
+  this.addFocusRow_('.entry-box input', 'checkbox');
+  this.addFocusRow_('.domain-checkbox', 'checkbox');
+
+  // Add the remaining elements.
+  this.addFocusRow_('.bookmark-section.starred', 'star');
+  this.addFocusRow_('[is="action-link"]', 'domain');
+  this.addFocusRow_('.title a', 'title');
+  this.addFocusRow_('.drop-down', 'menu');
+};
+
+HistoryFocusRow.prototype = {
+  __proto__: cr.ui.FocusRow.prototype,
 
-HistoryFocusObserver.prototype = {
   /** @override */
-  onActivate: function(row) {
-    this.getActiveRowElement_(row).classList.add('active');
+  onActivate: function() {
+    this.rowElement_.classList.add('active');
   },
 
   /** @override */
   onDeactivate: function(row) {
-    this.getActiveRowElement_(row).classList.remove('active');
+    this.rowElement_.classList.remove('active');
+  },
+
+  /** @override */
+  getEquivalentElement: function(element) {
+    if (this.contains(element))
+      return element;
+
+    var ret = null;
+
+    switch (element.getAttribute('column-type')) {
+      case ('checkbox'):
+        ret = this.getMatch('[column-type="checkbox"]');

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

what is getMatch? seems a lot like querySelector

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

Done. Replaced getMatch with querySelector.

+        break;
+      case ('star'):
+        ret = this.getMatch('[column-type="star"]') ||
+              this.getMatch('[column-type="title"]') ||
+              this.getMatch('[column-type="domain"]');
+        break;
+      case ('domain'):
+        ret = this.getMatch('[column-type="domain"]') ||
+              this.getMatch('[column-type="title"]');
+        break;
+      case ('title'):
+        ret = this.getMatch('[column-type="title"]') ||
+              this.getMatch('[column-type="domain"]');
+        break;
+      case ('menu'):
+        ret = this.rowElements[this.rowElements.length - 1];
+    }
+
+    return ret || this.rowElements[0];
   },
 
   /**
-   * @param {cr.ui.FocusRow} row The row to find an element for.
-   * @return {Element} |row|'s "active" element.
+   * Add a focusable element if it exists in this FocusRow.
+   * @param {string} query A query to select the appropriate element.
+   * @param {string} type The type to use for the element.
    * @private
    */
-  getActiveRowElement_: function(row) {
-    return findAncestorByClass(row.items[0], 'entry') ||
-           findAncestorByClass(row.items[0], 'site-domain-wrapper');
-  },
-};
-
-///////////////////////////////////////////////////////////////////////////////
-// HistoryFocusGrid:
-
-/**
- * @param {Node=} opt_boundary
- * @param {cr.ui.FocusRow.Observer=} opt_observer
- * @constructor
- * @extends {cr.ui.FocusGrid}
- */
-function HistoryFocusGrid(opt_boundary, opt_observer) {
-  cr.ui.FocusGrid.apply(this, arguments);
-}
-
-HistoryFocusGrid.prototype = {
-  __proto__: cr.ui.FocusGrid.prototype,
-
-  /** @override */
-  onMousedown: function(row, e) {
-    // TODO(dbeam): Can cr.ui.FocusGrid know about cr.ui.MenuButton? If so, bake
-    // this logic into the base class directly.
-    var menuButton = findAncestorByClass(e.target, 'menu-button');
-    if (menuButton) {
-      // Deactivate any other active row.
-      this.rows.some(function(r) {
-        if (r.activeIndex >= 0 && r != row) {
-          r.activeIndex = -1;
-          return true;
-        }
-      });
-      // Activate only the row with a pressed menu button.
-      row.activeIndex = row.items.indexOf(menuButton);
+  addFocusRow_: function(query, type) {
+    var element = this.getMatch(query);
+    if (element) {
+      this.setFocusableElement(element);
+      element.setAttribute('column-type', type);
     }
-    return !!menuButton;
   },
 };
 
@@ -964,8 +985,7 @@ function HistoryView(model) {
   this.editButtonTd_ = $('edit-button');
   this.editingControlsDiv_ = $('editing-controls');
   this.resultDiv_ = $('results-display');
-  this.focusGrid_ = new HistoryFocusGrid(this.resultDiv_,
-                                         new HistoryFocusObserver);
+  this.focusGrid_ = new cr.ui.FocusGrid();
   this.pageDiv_ = $('results-pagination');
   this.model_ = model;
   this.pageIndex_ = 0;
@@ -1204,7 +1224,7 @@ HistoryView.prototype.onBeforeRemove = function(visit) {
   var row = this.focusGrid_.rows[pos.row + 1] ||
             this.focusGrid_.rows[pos.row - 1];
   if (row)
-    row.focusIndex(Math.min(pos.col, row.items.length - 1));
+    row.focusEquivalentElement(pos.element);
 };
 
 /** @param {Visit} visit The visit about to be unstarred. */
@@ -1214,7 +1234,7 @@ HistoryView.prototype.onBeforeUnstarred = function(visit) {
 
   var pos = this.focusGrid_.getPositionForTarget(document.activeElement);
   var row = this.focusGrid_.rows[pos.row];
-  row.focusIndex(Math.min(pos.col + 1, row.items.length - 1));
+  row.focusEquivalentElement(pos.element);
 };
 
 /** @param {Visit} visit The visit that was just unstarred. */
@@ -1674,26 +1694,15 @@ var focusGridRowSelector = [
   '.site-domain-wrapper'
 ].join(', ');
 
-var focusGridColumnSelector = [
-  '.entry-box input',
-  '.bookmark-section.starred',
-  '.title a',
-  '.drop-down',
-  '.domain-checkbox',
-  '[is="action-link"]',
-].join(', ');
-
 /** @private */
 HistoryView.prototype.updateFocusGrid_ = function() {
   var rows = this.resultDiv_.querySelectorAll(focusGridRowSelector);
-  var grid = [];
+  this.focusGrid_.destroy();
 
   for (var i = 0; i < rows.length; ++i) {
     assert(rows[i].parentNode);
-    grid.push(rows[i].querySelectorAll(focusGridColumnSelector));
+    this.focusGrid_.addRow(new HistoryFocusRow(this.resultDiv_, rows[i]));
   }
-
-  this.focusGrid_.setGrid(grid);
 };
 
 /**