Fewer focusable items in chrome://settings/searchEngines

chrome/browser/resources/options/inline_editable_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', function() {
   /** @const */ var DeletableItem = options.DeletableItem;
   /** @const */ var DeletableItemList = options.DeletableItemList;
 
   /**
    * Creates a new list item with support for inline editing.
@@ skipping 70 matching lines @@
     selectionChanged: function() {
       this.updateEditState();
     },
 
     /**
      * Called when this element gains or loses 'lead' status. Updates editing
      * mode accordingly.
      * @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);
+      }
     },
 
     /**
      * Updates the edit state based on the current selected and lead states.
      */
     updateEditState: function() {
       if (this.editable)
         this.editing = this.selected && this.lead;
     },
 
@@ skipping 13 matching lines @@
       else
         this.removeAttribute('editing');
 
       if (editing) {
         this.editCancelled_ = false;
 
         cr.dispatchSimpleEvent(this, 'edit', true);
 
         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) {
           if (this.isPlaceholder)
             this.parentNode.focusPlaceholder = true;
 
           this.updateStaticValues_();
           cr.dispatchSimpleEvent(this, 'commitedit', true);
         } else {
           this.resetEditableValues_();
@@ skipping 66 matching lines @@
      * Returns true if the item has been changed by an edit.
      * Can be overridden by subclasses to return false when nothing has changed
      * to avoid unnecessary commits.
      * @type {boolean}
      */
     get hasBeenEdited() {
       return true;
     },
 
     /**
+     * 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.
      * @return {HTMLElement} The HTML element for the cell.
      * @private
      */
     createEditableTextCell: function(text) {
       var container = /** @type {HTMLElement} */(
           this.ownerDocument.createElement('div'));
       var textEl = null;
@@ skipping 19 matching lines @@
           if (list && list.focusPlaceholder) {
             list.focusPlaceholder = false;
             if (list.shouldFocusPlaceholder())
               inputEl.focus();
           }
         }, 50);
       }
 
       // 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)
             handler();
         }, 0);
       });
       container.appendChild(inputEl);
       this.addEditField(inputEl, textEl);
 
       return container;
     },
 
     /**
      * Register an edit field.
      * @param {!Element} control An editable element. It's a form control
      *     element typically.
      * @param {Element} staticElement An element representing non-editable
      *     state.
      */
     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);
     },
 
     /**
      * Resets the editable version of any controls created by createEditable*
      * to match the static text.
      * @private
      */
     resetEditableValues_: function() {
       var editFields = this.editFields_;
@@ skipping 65 matching lines @@
       }
     },
 
     /**
      * Called when the list item is clicked. If the click target corresponds to
      * an editable item, stores that item to focus when edit mode is started.
      * @param {Event} e The mouse down event.
      * @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;
     },
   };
 
   /**
    * Takes care of committing changes to inline editable list items when the
    * window loses focus.
    */
   function handleWindowBlurs() {
     window.addEventListener('blur', function(e) {
       var itemAncestor = findAncestor(document.activeElement, function(node) {
@@ skipping 33 matching lines @@
 
     /**
      * Called when the list hierarchy as a whole loses or gains focus; starts
      * or ends editing for the lead item if necessary.
      * @param {Event} e The change event.
      * @private
      */
     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);
+      }
+    },
+
     /**
      * May be overridden by subclasses to disable focusing the placeholder.
      * @return {boolean} True if the placeholder element should be focused on
      *     edit commit.
      */
     shouldFocusPlaceholder: function() {
       return true;
     },
   };
 
   // Export
   return {
     InlineEditableItem: InlineEditableItem,
     InlineEditableItemList: InlineEditableItemList,
   };
 });