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 {
-            this.focusAndMaybeSelect_(focusElement);
-          }
-        }
+        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.
+     * @param {boolean} focusable The desired focusable state.
+     */
+    setEditableValuesFocusable: function(focusable) {
+      focusable = focusable && this.editable;
+      var editFields = this.editFields_;
+      for (var i = 0; i < editFields.length; i++) {
+        editFields[i].tabIndex = focusable ? 0 : -1;
+      }
+    },
+
+    /**
+     * Sets whether the static values can be given focus using the keyboard.
+     * @param {boolean} focusable The desired focusable state.
+     */
+    setStaticValuesFocusable: function(focusable) {
+      var editFields = this.editFields_;
+      for (var i = 0; i < editFields.length; i++) {
+        var staticVersion = editFields[i].staticVersion;
+        if (!staticVersion)
+          return;

[Dan Beam, 2014/10/30 22:20:43]

shouldn't this be continue...?  do you have any tests for this code?

[bondd, 2014/10/30 22:45:29]

Done.
Gah, my bad. Typo. There are currently no dialogs that behave differently with a
return vs. continue here, so none of my manual tests failed. All list items with
staticVersion == null have staticVersion == null for all edit fields on that
line. There would need to be an edit field with staticVersion == null before one
with staticVersion != null for return vs. continue to make a difference here.
That case may exist in the future but right now it doesn't.

[Dan Beam, 2014/10/31 01:13:31]

we should probably still have some tests for all this

+        if (this.editable) {
+          staticVersion.tabIndex = focusable ? 0 : -1;
+        } else {
+          // staticVersion remains visible when !this.editable. Remove
+          // tabindex so that it will not become focused by clicking on it and
+          // have selection box drawn around it.
+          staticVersion.removeAttribute('tabindex');
+        }
+      }
+    },
+
+    /**
+     * Sets whether the close button can be focused using the keyboard.
+     * @param {boolean} focusable The desired focusable state.
+     */
+    setCloseButtonFocusable: function(focusable) {
+      this.closeButtonElement.tabIndex =
+          focusable && this.closeButtonFocusAllowed ? 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;
+        control.staticVersion.addEventListener('focus',
+                                               this.handleFocus.bind(this));
+      }
       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 editClickTarget;
       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;
+          editClickTarget = editFields[i];
+          break;
         }
       }
+
+      if (this.editing) {
+        if (!editClickTarget) {
+          // Clicked on the list item outside of an edit field. Don't lose focus
+          // from currently selected edit field.
+          e.stopPropagation();
+          e.preventDefault();
+        }
+        return;
+      }
+
+      if (editClickTarget && !editClickTarget.disabled)
+        this.editClickTarget_ = editClickTarget;
     },
   };
 
   /**
    * 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);
+        } 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,
   };
 });