Enable keyboard shortcuts for chrome://extensions

chrome/browser/resources/extensions/extension_error.js

Index: chrome/browser/resources/extensions/extension_error.js
diff --git a/chrome/browser/resources/extensions/extension_error.js b/chrome/browser/resources/extensions/extension_error.js
index 2452b12f8aaa2cade0966ddc34310b4bad681033..0218804133db857d8f30026ee270ffef00795e9c 100644
--- a/chrome/browser/resources/extensions/extension_error.js
+++ b/chrome/browser/resources/extensions/extension_error.js
@@ -28,24 +28,35 @@ cr.define('extensions', function() {
   /**
    * Creates a new ExtensionError HTMLElement; this is used to show a
    * notification to the user when an error is caused by an extension.
-   * @param {Object} error The error the element should represent.
+   * @param {RuntimeError} error The error the element should represent.
+   * @param {Element} boundary The boundary for the focus grid.
    * @constructor
-   * @extends {HTMLDivElement}
+   * @extends {cr.ui.FocusRow}
    */
-  function ExtensionError(error) {
+  function ExtensionError(error, boundary) {
     var div = cloneTemplate('extension-error-metadata');
     div.__proto__ = ExtensionError.prototype;
-    div.decorate(error);
+    div.decorate(error, boundary);
     return div;
   }
 
   ExtensionError.prototype = {
-    __proto__: HTMLDivElement.prototype,
+    __proto__: cr.ui.FocusRow.prototype,
+
+    /** @override */
+    getEquivalentElement: function(element) {
+      return assertInstanceof(
+          this.querySelector('.extension-error-view-details'), Element);
+    },
 
     /**
-     * @param {RuntimeError} error
+     * @param {RuntimeError} error The error the element should represent
+     * @param {Element} boundary The boundary for the FocusGrid.
+     * @override
      */
-    decorate: function(error) {
+    decorate: function(error, boundary) {
+      cr.ui.FocusRow.prototype.decorate.call(this, boundary);
+
       // Add an additional class for the severity level.
       if (error.level == 0)
         this.classList.add('extension-error-severity-info');
@@ -56,6 +67,7 @@ cr.define('extensions', function() {
 
       var iconNode = document.createElement('img');
       iconNode.className = 'extension-error-icon';
+      iconNode.alt = '';

[not at google - send to devlin, 2015/02/21 00:47:01]

+rdevlin.cronin

It looks like this alt should describe the severity level, unless there's some
other way the severity level is described to screen readers. You can leave it as
a todo for now though, pending what Devlin wants to do about it.

[Dan Beam, 2015/02/21 00:49:58]

doesn't the message indicate severity in some way or is this solely
presentational at this point?

[not at google - send to devlin, 2015/02/21 00:54:41]

It's meant to emulate devtools. If I were to do:

console.error('forgot the milk')
  [x] forgot the milk
console.log('forgot the milk')
      forgot the milk

those seem like different things that are worth communicating.

[Dan Beam, 2015/02/21 00:55:53]

so that's a "yes" :)

[not at google - send to devlin, 2015/02/21 00:57:05]

Confused. But, I think my comment has merit.

[Devlin, 2015/02/23 16:36:28]

Yeah, for perfect a11y this should describe the severity level ('log',
'warn[ing]', 'error').  The source of the image is set in css based on the class
(see lines 61-66), but I don't _think_ there's a way to do that for alt-text, so
it might be good to set here (I'm fine with it as a TODO, since it isn't a
regression to have it absent).

[hcarmona, 2015/02/23 22:25:08]

Do we have 18n strings for these warnings? If so, I can add them. If not, I'll
create an issue to add them.

       this.insertBefore(iconNode, this.firstChild);
 
       var messageSpan = this.querySelector('.extension-error-message');
@@ -79,6 +91,8 @@ cr.define('extensions', function() {
           extensions.ExtensionErrorOverlay.getInstance().setErrorAndShowOverlay(
               error, extensionUrl);
         });
+
+        this.addFocusableElement(viewDetailsLink);
       }
     },
   };
@@ -109,19 +123,70 @@ cr.define('extensions', function() {
     __proto__: HTMLDivElement.prototype,
 
     decorate: function() {
-      this.contents_ = this.querySelector('.extension-error-list-contents');
+      this.focusGrid_ = new cr.ui.FocusGrid();
+      this.gridBoundary_ = this.querySelector('.extension-error-list-contents');
+      this.gridBoundary_.addEventListener('focus', this.onFocus_.bind(this));
+      this.gridBoundary_.addEventListener('focusin',
+                                          this.onFocusin_.bind(this));
       this.errors_.forEach(function(error) {
         if (idIsValid(error.extensionId)) {

[not at google - send to devlin, 2015/02/21 00:47:01]

+rdevlin.cronin

How can this idIsValid check fail, these errors should be populated from our
trusted C++ code?

[Devlin, 2015/02/23 16:36:28]

IIRC, this is just us being paranoid.  Yes, these should be populated via
trusted code, but there's a chance the renderer is compromised, and these errors
can (indirectly) cause access to the filesystem.  That said, I'm pretty sure we
have checks on the C++ side, too.  Up to kalman/dbeam if they want to keep the
extra check here.

[not at google - send to devlin, 2015/02/23 16:53:26]

That's true. In this case I'd argue that it'd just hide a bug on the browser
side if anything; we should be validating things like extension IDs. Hopefully
that isn't a source of a WebUI vulnerability (e.g. triggering a bug, versus
"just" triggering a privilege escalation). In any case totally orthogonal, sorry
bout that :-)

[hcarmona, 2015/02/23 22:25:08]

What should we do about the check? Keep it or get rid of it?

-          this.contents_.appendChild(document.createElement('li')).appendChild(
-              new ExtensionError(error));
+          var focusRow = new ExtensionError(error, this.gridBoundary_);
+          this.gridBoundary_.appendChild(
+              document.createElement('li')).appendChild(focusRow);
+          this.focusGrid_.addRow(focusRow);
         }
       }, this);
 
-      var numShowing = this.contents_.children.length;
+      var numShowing = this.focusGrid_.rows.length;
       if (numShowing > ExtensionErrorList.MAX_ERRORS_TO_SHOW_)
         this.initShowMoreLink_();
     },
 
+    /** @return {?Element} The "Show more" element or null if it's hidden. */
+    getToggleElement: function() {

[not at google - send to devlin, 2015/02/21 00:47:01]

The comment is more useful than the name of the function; should it be
getShowMoreElement? Is it a toggle because it says "Show less" sometimes, in
which case the comment needs to express that as well?

[hcarmona, 2015/02/23 22:25:08]

Updated to comment to reflect that the "Show more" and "Show less" button are
the same element.

+      return this.querySelector(
+          '.extension-error-list-show-more [is="action-link"]:not([hidden])');
+    },
+
+    /** @return {!Element} The element containing the list of errors. */
+    getListElement: function() {

[not at google - send to devlin, 2015/02/21 00:47:01]

ListElement makes me think like HTMLUListElement or whatever. Maybe
getErrorListElement?

[hcarmona, 2015/02/23 22:25:08]

Done.

+      return this.gridBoundary_;
+    },
+
+    /**
+     * The gird should not be focusable once it or an element inside it is

[not at google - send to devlin, 2015/02/21 00:47:01]

grid

[Dan Beam, 2015/02/21 00:49:58]

#noregerts

[hcarmona, 2015/02/23 22:25:08]

Done.

+     * focused. This is necessary to allow tabbing out of the grid in reverse.
+     * @private
+     */
+    onFocusin_: function() {
+      this.gridBoundary_.tabIndex = -1;
+    },
+
+    /**
+     * Focus the first focusable row when tabbing into the grid for the
+     * first time.
+     * @private
+     */
+    onFocus_: function() {
+      var activeRow = this.gridBoundary_.querySelector('.focus-row-active');
+      var toggleButton = this.getToggleElement();
+
+      if (!toggleButton || toggleButton.isShowingAll) {
+        activeRow = activeRow || this.focusGrid_.rows[0];
+        activeRow.getEquivalentElement(null).focus();
+      } else {
+        var rows = this.focusGrid_.rows;
+
+        var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_;
+        var focusedIndex = rows.indexOf(activeRow);
+
+        if (focusedIndex < firstVisible)
+          activeRow = rows[firstVisible];
+
+        activeRow.getEquivalentElement(null).focus();
+      }
+    },
+
     /**
      * Initialize the "Show More" link for the error list. If there are more
      * than |MAX_ERRORS_TO_SHOW_| errors in the list.
@@ -144,6 +209,9 @@ cr.define('extensions', function() {
       });
 
       link.addEventListener('click', function(e) {
+        // Needs to be enabled in case the focused row is now hidden.
+        this.gridBoundary_.tabIndex = 0;
+
         link.isShowingAll = !link.isShowingAll;
 
         var message = link.isShowingAll ? 'extensionErrorsShowFewer' :