Fix tabindex in chrome://settings/passwords

chrome/browser/resources/options/password_manager_list.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.
 
 cr.define('options.passwordManager', function() {
   /** @const */ var ArrayDataModel = cr.ui.ArrayDataModel;
   /** @const */ var DeletableItemList = options.DeletableItemList;
   /** @const */ var DeletableItem = options.DeletableItem;
   /** @const */ var List = cr.ui.List;
 
@@ skipping 51 matching lines @@
       this.contentElement.appendChild(usernameLabel);
 
       // The stored password.
       var passwordInputDiv = this.ownerDocument.createElement('div');
       passwordInputDiv.className = 'password';
 
       // The password input field.
       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_) {
         var button = this.ownerDocument.createElement('button');
         button.hidden = true;
         button.className = 'list-inline-button custom-appearance';
         button.textContent = loadTimeData.getString('passwordShowButton');
         button.addEventListener('click', this.onClick_.bind(this), true);
         button.addEventListener('mousedown', function(event) {
           // Don't focus on this button by mousedown.
           event.preventDefault();
           // Don't handle list item selection. It causes focus change.
           event.stopPropagation();
         }, false);
         passwordInputDiv.appendChild(button);
         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);
     },
 
     /** @override */
     selectionChanged: function() {
       var input = this.passwordField;
       var button = this.passwordShowButton;
       // The button doesn't exist when passwords can't be shown.
       if (!button)
         return;
 
       if (this.selected) {
+        input.classList.remove('inactive-password');
+        this.setFocusable(true);
+        button.hidden = false;
         input.focus();
-        input.classList.remove('inactive-password');
-        input.tabIndex = 0;
-        this.closeButtonElement.tabIndex = 0;
-        button.hidden = false;
       } 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) {
       this.passwordField.value = password;
       this.passwordField.type = 'text';
 
       var button = this.passwordShowButton;
       if (button)
         button.textContent = loadTimeData.getString('passwordHideButton');
     },
@@ skipping 136 matching lines @@
      * @private
      */
     showPasswords_: true,
 
     /** @override */
     decorate: function() {
       DeletableItemList.prototype.decorate.call(this);
       Preferences.getInstance().addEventListener(
           'profile.password_manager_allow_show_passwords',
           this.onPreferenceChanged_.bind(this));
+      this.addEventListener('focus', this.onFocusIn_);
     },
 
     /**
      * Listener for changes on the preference.
      * @param {Event} event The preference update event.
      * @private
      */
     onPreferenceChanged_: function(event) {
       this.showPasswords_ = event.value.value;
       this.redraw();
@@ skipping 21 matching lines @@
       }
       PasswordManager.removeSavedPassword(index);
     },
 
     /**
      * The length of the list.
      */
     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);
+    },
   };
 
   /**
    * Create a new passwords list.
    * @constructor
    * @extends {options.DeletableItemList}
    */
   var PasswordExceptionsList = cr.ui.define('list');
 
   PasswordExceptionsList.prototype = {
@@ skipping 20 matching lines @@
     },
   };
 
   return {
     PasswordListItem: PasswordListItem,
     PasswordExceptionsListItem: PasswordExceptionsListItem,
     PasswordsList: PasswordsList,
     PasswordExceptionsList: PasswordExceptionsList,
   };
 });