Fix tabindex in chrome://settings/passwords

chrome/browser/resources/options/password_manager_list.js

Index: chrome/browser/resources/options/password_manager_list.js
diff --git a/chrome/browser/resources/options/password_manager_list.js b/chrome/browser/resources/options/password_manager_list.js
index 9c40e98d79070d88b8568a3dbe10b0e16a558d2e..b6d4da559d232273d6152470c04a9bef1fcd9511 100644
--- a/chrome/browser/resources/options/password_manager_list.js
+++ b/chrome/browser/resources/options/password_manager_list.js
@@ -69,12 +69,11 @@ cr.define('options.passwordManager', function() {
       var passwordInput = this.ownerDocument.createElement('input');
       passwordInput.type = 'password';
       passwordInput.className = 'inactive-password';
-      this.closeButtonElement.tabIndex = -1;
-      passwordInput.tabIndex = -1;
       passwordInput.readOnly = true;
       passwordInput.value = this.showPasswords_ ? this.password : '********';
       passwordInputDiv.appendChild(passwordInput);
       this.passwordField = passwordInput;
+      this.setFocusable(false);
 
       // The show/hide button.
       if (this.showPasswords_) {
@@ -93,6 +92,12 @@ cr.define('options.passwordManager', function() {
         this.passwordShowButton = button;
       }
 
+      // |handleFocus| is defined in the base class and |closeButtonElement_|
+      // listens for the focus event. Listening for focusin on |this| will also
+      // include |closeButtonElement_|.
+      this.closeButtonElement_.removeEventListener('focus', this.handleFocus);

[Dan Beam, 2015/01/24 02:42:25]

shouldn't this be:

  this.closeButtonElement_.removeEventListener(
      'focus', this.closeButtonElement_.handleFocus);

i don't think it'll work otherwise.

is this really necessary?  this seems like the second time I'm not sure this is
even working, leading me to believe it isn't required.

ultimately it's probably better to make some accessor on close buttons called
.ignoreFocusEvents() or something that does this internally.

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

Acknowledged.

+      this.addEventListener('focusin', this.handleFocus);

[Dan Beam, 2015/01/24 02:42:25]

don't pass a 'focusin' event to a method that expects a 'focus' event, imo

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

Rather than adding a focusin event to the PasswordListItem, I've added listeners
for the focus event to both of the additional controls in the PasswordListItem.
This has the desired effect (highlight the row when an item is focused) without
having to remove the listener from the close button.

+
       this.contentElement.appendChild(passwordInputDiv);
     },
 
@@ -105,20 +110,27 @@ cr.define('options.passwordManager', function() {
         return;
 
       if (this.selected) {
-        input.focus();
         input.classList.remove('inactive-password');
-        input.tabIndex = 0;
-        this.closeButtonElement.tabIndex = 0;
+        this.setFocusable(true);
         button.hidden = false;
+        input.focus();
       } else {
         input.classList.add('inactive-password');
-        input.tabIndex = -1;
-        this.closeButtonElement.tabIndex = -1;
+        this.setFocusable(false);
         button.hidden = true;
       }
     },
 
     /**
+     * Set the focusability of this row.
+     * @param {boolean} focusable
+     */
+    setFocusable: function(focusable) {
+      var tabIndex = focusable ? 0 : -1;
+      this.passwordField.tabIndex = this.closeButtonElement.tabIndex = tabIndex;
+    },
+
+    /**
      * Reveals the plain text password of this entry.
      */
     showPassword: function(password) {
@@ -275,6 +287,7 @@ cr.define('options.passwordManager', function() {
       Preferences.getInstance().addEventListener(
           'profile.password_manager_allow_show_passwords',
           this.onPreferenceChanged_.bind(this));
+      this.addEventListener('focus', this.onFocusIn_);
     },
 
     /**
@@ -316,6 +329,17 @@ cr.define('options.passwordManager', function() {
     get length() {
       return this.dataModel.length;
     },
+
+    /**
+     * Will make to first row focusable if none are selected. This makes it
+     * possible to tab into the rows without pressing up/down first.
+     * @param {Event} e The focusin event.

[Dan Beam, 2015/01/24 02:42:25]

this is no longer a focusin event

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

Done.

+     * @private
+     */
+    onFocusIn_: function(e) {

[Dan Beam, 2015/01/24 02:42:24]

onFocus_

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

Done.

+      if (!this.selectedItem && this.items)
+        this.items[0].setFocusable(true);
+    },
   };
 
   /**