Make downloads list keyboard shortcuts more consistent.

chrome/browser/resources/history/history.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 <include src="../uber/uber_utils.js">
 <include src="history_focus_manager.js">
 
 ///////////////////////////////////////////////////////////////////////////////
 // Globals:
 /** @const */ var RESULTS_PER_PAGE = 150;
@@ skipping 232 matching lines @@
       chrome.send('removeBookmark', [self.url_]);
 
       this.model_.getView().onBeforeUnstarred(this);
       bookmarkSection.classList.remove('starred');
       this.model_.getView().onAfterUnstarred(this);
 
       bookmarkSection.removeEventListener('click', f);
       e.preventDefault();
     }.bind(this));
   }
+
+  if (focusless)
+    bookmarkSection.tabIndex = -1;
+
   entryBox.appendChild(bookmarkSection);
 
   var visitEntryWrapper = /** @type {HTMLElement} */(
       entryBox.appendChild(document.createElement('div')));
   if (addTitleFavicon || this.blockedVisit)
     visitEntryWrapper.classList.add('visit-entry');
   if (this.blockedVisit) {
     visitEntryWrapper.classList.add('blocked-indicator');
     visitEntryWrapper.appendChild(this.getVisitAttemptDOM_());
   } else {
@@ skipping 614 matching lines @@
 
 /**
  * Gets whether we are grouped by domain.
  * @return {boolean} Whether the results are grouped by domain.
  */
 HistoryModel.prototype.getGroupByDomain = function() {
   return this.groupByDomain_;
 };
 
 ///////////////////////////////////////////////////////////////////////////////
-// HistoryFocusObserver:
+// HistoryFocusRow:
 
 /**
+ * Provides an implementation for a single column grid.
  * @constructor
- * @implements {cr.ui.FocusRow.Observer}
+ * @extends {cr.ui.FocusRow}
  */
-function HistoryFocusObserver() {}
+function HistoryFocusRow() {}
 
-HistoryFocusObserver.prototype = {
+/**
+ * Decorates |rowElement| so that it can be treated as a HistoryFocusRow item.
+ * @param {Element} rowElement The element representing this row.
+ * @param {Node} boundary Focus events are ignored outside of this node.
+ */
+HistoryFocusRow.decorate = function(rowElement, boundary) {
+  rowElement.__proto__ = HistoryFocusRow.prototype;
+  rowElement.decorate(boundary);
+
+  /**
+   * 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.

[Dan Beam, 2015/01/29 18:46:46]

use

  // comments

instead of

  /** comment */

(the latter means jsdoc)

[Dan Beam, 2015/01/29 18:46:46]

i don't think this comment is all that helpful.  i think a reader will
understand they're both the same type by both being 'checkbox'.

[hcarmona, 2015/01/31 02:45:49]

Done.

[hcarmona, 2015/01/31 02:45:49]

Done.

+   */
+  rowElement.addFocusRow_('.entry-box input', 'checkbox');
+  rowElement.addFocusRow_('.domain-checkbox', 'checkbox');
+
+  // Add the remaining elements.

[Dan Beam, 2015/01/29 18:46:46]

not useful

[hcarmona, 2015/01/31 02:45:49]

Done.

+  rowElement.addFocusRow_('.bookmark-section.starred', 'star');
+  rowElement.addFocusRow_('[is="action-link"]', 'domain');
+  rowElement.addFocusRow_('.title a', 'title');
+  rowElement.addFocusRow_('.drop-down', 'menu');
+};
+
+HistoryFocusRow.prototype = {
+  __proto__: cr.ui.FocusRow.prototype,
+
   /** @override */
-  onActivate: function(row) {
-    this.getActiveRowElement_(row).classList.add('active');
+  onActiveStateChanged: function(state) {
+    this.classList.toggle('active', state);
   },
 
   /** @override */
-  onDeactivate: function(row) {
-    this.getActiveRowElement_(row).classList.remove('active');
+  getEquivalentElement: function(element) {
+    if (this.contains(element))
+      return element;
+
+    // All elements default to another element with the same type.
+    var ret = this.getColumn_(element.getAttribute('column-type'));

[Dan Beam, 2015/01/29 18:46:46]

s/ret/equivalent or el

[hcarmona, 2015/01/31 02:45:49]

Done.

+
+    if (!ret) {
+      switch (element.getAttribute('column-type')) {
+        case 'star':
+          ret = this.getColumn_('title') || this.getColumn_('domain');
+          break;
+        case 'domain':
+          ret = this.getColumn_('title');
+          break;
+        case 'title':
+          ret = this.getColumn_('domain');
+          break;
+        case 'menu':
+          ret = this.focusableElements[this.focusableElements.length - 1];
+      }
+    }
+
+    return ret || this.focusableElements[0];
   },
 
   /**
-   * @param {cr.ui.FocusRow} row The row to find an element for.
-   * @return {Element} |row|'s "active" element.
+   * @param {string} type The type of column to return.
+   * @return {?Element} The column matching the type.
    * @private
    */
-  getActiveRowElement_: function(row) {
-    return findAncestorByClass(row.items[0], 'entry') ||
-           findAncestorByClass(row.items[0], 'site-domain-wrapper');
+  getColumn_: function(type) {
+    return this.querySelector('[column-type=' + type + ']');
+  },
+
+  /**
+   * 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
+   */
+  addFocusRow_: function(query, type) {

[Dan Beam, 2015/01/29 18:46:46]

shouldn't this be addFocusItem_ or something?  this isn't adding a focus *row*,
is it?

[hcarmona, 2015/01/31 02:45:49]

Done.

+    var element = this.querySelector(query);
+    if (element) {
+      this.addFocusableElement(element);
+      element.setAttribute('column-type', type);
+    }
   },
 };
 
-///////////////////////////////////////////////////////////////////////////////
-// 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);
-    }
-    return !!menuButton;
-  },
-};
-
 ///////////////////////////////////////////////////////////////////////////////
 // HistoryView:
 
 /**
  * Functions and state for populating the page with HTML. This should one-day
  * contain the view and use event handlers, rather than pushing HTML out and
  * getting called externally.
  * @param {HistoryModel} model The model backing this view.
  * @constructor
  */
 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;
   this.lastDisplayed_ = [];
 
   this.model_.setView(this);
 
   this.currentVisits_ = [];
 
   // If there is no search button, use the search button label as placeholder
@@ skipping 211 matching lines @@
 
   this.positionNotificationBar();
 };
 
 /**
  * @param {Visit} visit The visit about to be removed from this view.
  */
 HistoryView.prototype.onBeforeRemove = function(visit) {
   assert(this.currentVisits_.indexOf(visit) >= 0);
 
-  var pos = this.focusGrid_.getPositionForTarget(document.activeElement);
-  if (!pos)
+  var rowIndex = this.focusGrid_.getRowIndexForTarget(document.activeElement);
+  if (rowIndex == -1)
     return;
 
-  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));
+  var rowToFocus = this.focusGrid_.rows[rowIndex + 1] ||
+                   this.focusGrid_.rows[rowIndex - 1];
+  if (rowToFocus)
+    rowToFocus.getEquivalentElement(document.activeElement).focus();
 };
 
 /** @param {Visit} visit The visit about to be unstarred. */
 HistoryView.prototype.onBeforeUnstarred = function(visit) {
   assert(this.currentVisits_.indexOf(visit) >= 0);
   assert(visit.bookmarkStar == document.activeElement);
 
-  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));
+  var rowIndex = this.focusGrid_.getRowIndexForTarget(document.activeElement);
+  var row = this.focusGrid_.rows[rowIndex];
+
+  // Focus the title or domain when the bookmarked star is removed because the
+  // star will no longer be focusable.
+  row.querySelector('[column-type=title], [column-type=domain]').focus();
 };
 
 /** @param {Visit} visit The visit that was just unstarred. */
 HistoryView.prototype.onAfterUnstarred = function(visit) {
   this.updateFocusGrid_();
 };
 
 /**
  * Removes a single entry from the view. Also removes gaps before and after
  * entry if necessary.
@@ skipping 77 matching lines @@
   } else {
     bar.classList.remove('alone');
   }
 };
 
 /**
  * @param {Element} el An element to look for.
  * @return {boolean} Whether |el| is in |this.focusGrid_|.
  */
 HistoryView.prototype.isInFocusGrid = function(el) {
-  return !!this.focusGrid_.getPositionForTarget(el);
+  return this.focusGrid_.getRowIndexForTarget(el) != -1;
 };
 
 // HistoryView, private: ------------------------------------------------------
 
 /**
  * Clear the results in the view.  Since we add results piecemeal, we need
  * to clear them out when we switch to a new page or reload.
  * @private
  */
 HistoryView.prototype.clear_ = function() {
@@ skipping 341 matching lines @@
   // time column.
   this.setTimeColumnWidth_();
 };
 
 var focusGridRowSelector = [
   ':-webkit-any(.day-results, .search-results) > .entry:not(.fade-out)',
   '.expand .grouped .entry:not(.fade-out)',
   '.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));
+    HistoryFocusRow.decorate(rows[i], this.resultDiv_);
+    this.focusGrid_.addRow(rows[i]);
   }
-
-  this.focusGrid_.setGrid(grid);
 };
 
 /**
  * Update the visibility of the page navigation buttons.
  * @private
  */
 HistoryView.prototype.updateNavBar_ = function() {
   this.updateRangeButtons_();
 
   // If grouping by domain is enabled, there's a control bar on top, don't show
@@ skipping 674 matching lines @@
     historyView.reload();
 }
 
 // Add handlers to HTML elements.
 document.addEventListener('DOMContentLoaded', load);
 
 // This event lets us enable and disable menu items before the menu is shown.
 document.addEventListener('canExecute', function(e) {
   e.canExecute = true;
 });