Fewer focusable items in chrome://settings/searchEngines

chrome/browser/resources/options/inline_editable_list.js

Index: chrome/browser/resources/options/inline_editable_list.js
diff --git a/chrome/browser/resources/options/inline_editable_list.js b/chrome/browser/resources/options/inline_editable_list.js
index 1461fca292fefed6a6ae4b729d459018957307b9..f9d6d436c9da3399030bcc348adcd2e439aecc4d 100644
--- a/chrome/browser/resources/options/inline_editable_list.js
+++ b/chrome/browser/resources/options/inline_editable_list.js
@@ -88,7 +88,20 @@ cr.define('options', function() {
      * @private
      */
     handleLeadChange_: function() {
+      // Add focusability before call to updateEditState.
+      if (this.lead) {
+        this.setEditableValuesFocusable(true);
+        this.setCloseButtonFocusable(true);
+      }
+
       this.updateEditState();
+
+      // Remove focusability after call to updateEditState.
+      this.setStaticValuesFocusable(false);
+      if (!this.lead) {
+        this.setEditableValuesFocusable(false);
+        this.setCloseButtonFocusable(false);
+      }
     },
 
     /**
@@ -122,27 +135,8 @@ cr.define('options', function() {
 
         var focusElement = this.editClickTarget_ || this.initialFocusElement;
         this.editClickTarget_ = null;
-
-        if (focusElement) {
-          var self = this;
-          // We should delay to give focus on |focusElement| if this is called
-          // in mousedown event handler. If we did give focus immediately, Blink
-          // would try to focus on an ancestor of the mousedown target element,
-          // and remove focus from |focusElement|.
-          if (focusElement.staticVersion &&
-              focusElement.staticVersion.hasAttribute('tabindex')) {
-            setTimeout(function() {
-              if (self.editing) {
-                if (focusElement.disabled)
-                  self.parentNode.focus();
-                self.focusAndMaybeSelect_(focusElement);
-              }
-              focusElement.staticVersion.removeAttribute('tabindex');
-            }, 0);
-          } else {

[bondd, 2014/10/28 22:29:57]

No longer required for new way of focusing elements.

+        if (focusElement)
             this.focusAndMaybeSelect_(focusElement);
-          }
-        }
       } else {
         if (!this.editCancelled_ && this.hasBeenEdited &&
             this.currentInputIsValid) {
@@ -229,6 +223,76 @@ cr.define('options', function() {
     },
 
     /**
+     * Sets whether the editable values can be given focus using the
+     * keyboard+mouse, mouse only, or neither.

[Dan Beam, 2014/10/29 01:37:13]

using the keyboard.

[bondd, 2014/10/29 17:19:08]

Done.

+     *
+     * Result:
+     * If this item is not editable: focusable by neither
+     * else if the field is disabled: focusable by mouse only
+     * else:
+     *   if wantFocusable == true: focusable by mouse+keyboard
+     *   else: focusable by mouse only

[Dan Beam, 2014/10/29 01:37:13]

^ I don't think this is required, unfocusable means you can still see them (and
hence, click) ;)

[bondd, 2014/10/29 17:19:07]

I removed the entire "Result" comment block. Is that what you intended?

+     *
+     * @param {boolean} wantFocusable The desired focusable state.
+     */
+    setEditableValuesFocusable: function(wantFocusable) {

[Dan Beam, 2014/10/29 01:37:13]

wantFocusable => focusable

[Dan Beam, 2014/10/29 01:37:13]

this.editable is loop invariant, I believe, so it'd probably be the easiest to
just:

focusable = focusable && this.editable;

[bondd, 2014/10/29 17:19:08]

Done.

+      var editFields = this.editFields_;
+      for (var i = 0; i < editFields.length; i++) {
+        if (this.editable) {
+          if (wantFocusable && !editFields[i].disabled)

[Dan Beam, 2014/10/29 01:37:13]

it does seem like [disabled][tabindex=0] is actually focusable
http://jsfiddle.net/na5dehp4/show/

[Dan Beam, 2014/10/29 01:37:14]

can't this whole loop body be

editFields[i].tabIndex = focusable && !editFields[i].disabled ? 0 : -1;

[bondd, 2014/10/29 17:19:08]

Done. I had guessed that condensing logic like this would be frowned upon, but I
finally went and read the whole style guide and found out that I guessed wrong.
:)

[bondd, 2014/10/29 17:19:08]

Did you mean "it doesn't seem"? That button you linked is not focusable for me.

[Dan Beam, 2014/10/29 22:28:11]

doesn't**

+              editFields[i].tabIndex = 0;

[Dan Beam, 2014/10/29 01:37:13]

indent off

+          else
+            editFields[i].tabIndex = -1;
+        }

[Dan Beam, 2014/10/29 01:37:13]

} else {

+        else {
+          editFields[i].removeAttribute('tabindex');

[bondd, 2014/10/29 17:19:08]

I got rid of this per your above comment re: condensing the loop body. Tested it
with this line removed and everything still works.

+        }
+      }
+    },
+
+    /**
+     * Sets whether the static values can be given focus using the
+     * keyboard+mouse, mouse only, or neither.
+     *
+     * Result:
+     * If this item is not editable: focusable by neither
+     * else if the field is disabled: focusable by mouse only
+     * else:
+     *   if wantFocusable == true: focusable by mouse+keyboard
+     *   else: focusable by mouse only
+     *
+     * @param {boolean} wantFocusable The desired focusable state.
+     */
+    setStaticValuesFocusable: function(wantFocusable) {
+      var editFields = this.editFields_;
+      for (var i = 0; i < editFields.length; i++) {
+        var staticVersion = editFields[i].staticVersion;
+        if (!staticVersion)
+          continue;
+
+        if (this.editable) {

[Dan Beam, 2014/10/29 01:37:13]

same general nits here as in setEditableValuesFocusable()

[bondd, 2014/10/29 17:19:08]

Done.

+          if (wantFocusable && !staticVersion.disabled)
+              staticVersion.tabIndex = 0;
+          else
+            staticVersion.tabIndex = -1;
+        }
+        else {
+          staticVersion.removeAttribute('tabindex');
+          }

[Dan Beam, 2014/10/29 01:37:13]

indent off (-2 \s)

+      }
+    },
+
+    /**
+     * Sets whether the close button can be focused using the keyboard.
+     *
+     * @param {boolean} wantFocusable The desired focusable state.
+     */
+    setCloseButtonFocusable: function(wantFocusable) {

[Dan Beam, 2014/10/29 01:37:13]

why can't we just use an ES5 getter/setter for this?

[bondd, 2014/10/29 17:19:08]

To keep it consistent with setStaticValuesFocusable and
setEditableValuesFocusable, since all 3 are usually used together.

Google JavaScript Style Guide says "EcmaScript 5 getters and setters for
properties are discouraged". Maybe that just means don't wrap variables, but
putting functionality inside them is okay (?). What is your interpretation of
that sentence from the style guide? Either way it seems we use ES5 getters and
setters all over the place. Let me know if you want me to change this into an
ES5 setter.

[Dan Beam, 2014/10/29 22:28:11]

that doesn't apply to Chrome code, as you've determined -- we use them
everywhere.

i think the internal methods can stay as set*() but it'd be weird if a caller or
downstream class said:

  list.closeButtonFocusAllowed = false;

and it didn't have any immediate effect.

+      if (this.closeButtonFocusAllowed || !wantFocusable)
+        this.closeButtonElement.tabIndex = wantFocusable ? 0 : -1;
+    },
+
+    /**
      * Returns a div containing an <input>, as well as static text if
      * isPlaceholder is not true.
      * @param {string} text The text of the cell.
@@ -268,7 +332,7 @@ cr.define('options', function() {
 
       // In some cases 'focus' event may arrive before 'input'.
       // To make sure revalidation is triggered we postpone 'focus' handling.
-      var handler = this.handleFocus_.bind(this);
+      var handler = this.handleFocus.bind(this);
       inputEl.addEventListener('focus', function() {
         window.setTimeout(function() {
           if (inputEl.ownerDocument.activeElement == inputEl)
@@ -290,6 +354,19 @@ cr.define('options', function() {
      */
     addEditField: function(control, staticElement) {
       control.staticVersion = staticElement;
+      if (this.editable)
+        control.tabIndex = -1;
+
+      if (control.staticVersion) {
+        if (this.editable)
+          control.staticVersion.tabIndex = -1;
+        control.staticVersion.editableVersion = control;
+
+        var handler = this.handleFocus.bind(this);
+        control.staticVersion.addEventListener('focus', function() {

[Dan Beam, 2014/10/29 01:37:13]

control.staticVersion.addEventListener('focus', this.handleFocus.bind(this));

[bondd, 2014/10/29 17:19:08]

Done.

+          handler();
+        });
+      }
       this.editFields_.push(control);
     },
 
@@ -375,20 +452,32 @@ cr.define('options', function() {
      * @private
      */
     handleMouseDown_: function(e) {
-      if (!this.editable || this.editing)
+      if (!this.editable)
         return;
 
       var clickTarget = e.target;
       var editFields = this.editFields_;
+      var clickField;

[Dan Beam, 2014/10/29 01:37:14]

clickField => editClickTarget

[bondd, 2014/10/29 17:19:08]

Done.

[Dan Beam, 2014/10/29 22:28:11]

editClickTarget (note the "Target")

[bondd, 2014/10/29 23:08:54]

Done.

       for (var i = 0; i < editFields.length; i++) {
-        if (editFields[i].staticVersion == clickTarget)
-          clickTarget.tabIndex = 0;
-        if (editFields[i] == clickTarget ||
-            editFields[i].staticVersion == clickTarget) {
-          this.editClickTarget_ = editFields[i];
-          return;
+        if ((editFields[i] == clickTarget ||
+            editFields[i].staticVersion == clickTarget)) {

[bondd, 2014/10/28 22:29:57]

Oops, double parens (( ))

[bondd, 2014/10/29 17:19:08]

Done.

+          clickField = editFields[i];
+          break;
         }
       }
+
+      if (this.editing) {
+        if (!clickField) {
+          // Clicked on the list item outside of an edit field. Don't lose
+          // focus from currently selected edit field.

[Dan Beam, 2014/10/29 01:37:13]

wrap at 80 cols

[bondd, 2014/10/29 17:19:08]

Done.

+          e.stopPropagation();
+          e.preventDefault();
+        }
+        return;
+      }
+
+      if (clickField && !clickField.disabled)
+        this.editClickTarget_ = clickField;
     },
   };
 
@@ -442,10 +531,39 @@ cr.define('options', function() {
     handleListFocusChange_: function(e) {
       var leadItem = this.getListItemByIndex(this.selectionModel.leadIndex);
       if (leadItem) {
-        if (e.newValue)
+        if (e.newValue) {
+          // Add focusability before making other changes.
+          leadItem.setEditableValuesFocusable(true);
+          leadItem.setCloseButtonFocusable(true);
           leadItem.updateEditState();
-        else
+          // Remove focusability after making other changes.
+          leadItem.setStaticValuesFocusable(false);
+        }

[Dan Beam, 2014/10/29 01:37:13]

} else {

[bondd, 2014/10/29 17:19:08]

Done.

+        else {
+          // Add focusability before making other changes.
+          leadItem.setStaticValuesFocusable(true);
+          leadItem.setCloseButtonFocusable(true);
           leadItem.editing = false;
+          // Remove focusability after making other changes.
+          if (!leadItem.isPlaceholder)
+            leadItem.setEditableValuesFocusable(false);
+        }
+      }
+    },
+
+    /**
+     * Called after the DataModel for the list has been set.
+     * @override
+     */
+    onSetDataModelComplete: function() {
+      DeletableItemList.prototype.onSetDataModelComplete.call(this);
+
+      var firstItem = this.getListItemByIndex(0);
+      if (firstItem) {
+        firstItem.setStaticValuesFocusable(true);
+        firstItem.setCloseButtonFocusable(true);
+        if (firstItem.isPlaceholder)
+          firstItem.setEditableValuesFocusable(true);
       }
     },