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;
       }
 
+      // A row should be focused when any element in it is focused, not just
+      // the close button.
+      this.closeButtonElement_.removeEventListener(this.handleFocus, 'focus');

[Dan Beam, 2015/01/23 00:00:37]

i'm confused.

a) the order is 'eventName', eventHandlerFunction[1]

where is the corresponding this.closeButtonElement_.addEventListener call or
this.closeButtonElement_ even defined?

is this supposed to say

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

?

[1]
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener

[hcarmona, 2015/01/23 21:27:35]

Done.

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

[Dan Beam, 2015/01/23 00:00:37]

why do we need to use focusin here?  because it bubbles?

[hcarmona, 2015/01/23 21:27:34]

Yes, we need to focus event to bubble up from the elements in |this|.

+
       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;
+        // Move the focus away from anything else that may have been focused.

[Dan Beam, 2015/01/23 00:00:37]

this comment is confusing.  needed?

[hcarmona, 2015/01/23 21:27:35]

Done.

         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 {bool} focusable

[Dan Beam, 2015/01/23 00:00:37]

boolean

[hcarmona, 2015/01/23 21:27:34]

Done.

+     */
+    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('focusin', 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;
     },
+
+    /**
+     * Called when focus enters this list. Will make the first row focusable if
+     * nothing is selected.
+     * Without this: the list will be focused and no row will be focused until
+     * up/down is pressed making it appear that the row items are not focusable.

[Dan Beam, 2015/01/23 00:00:37]

write this sentence using the word "focus" less

[hcarmona, 2015/01/23 21:27:35]

Reworded to make more clear.

+     * @param {Event} e The focusin event.
+     * @private
+     */
+    onFocusIn_: function(e) {
+      if (e.target == this && !this.selectedItem && this.items)

[Dan Beam, 2015/01/23 00:00:37]

if we're only looking for the case where e.target == this, why not just use
focus instead?

[hcarmona, 2015/01/23 21:27:34]

Done.

+        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,
   };
 });