Maintain focused column 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 1824307cd0ab8ca69f046c4de687840668d4c2ec..cc76aa1cde2c3e547e14c4ae24c744c31bc59c9b 100644
--- a/chrome/browser/resources/options/inline_editable_list.js
+++ b/chrome/browser/resources/options/inline_editable_list.js
@@ -31,6 +31,12 @@ cr.define('options', function() {
     __proto__: DeletableItem.prototype,
 
     /**
+     * Index of currently focused column, or -1 for none.
+     * @type {number}
+     */
+    focusedColumnIndex: -1,

[bondd, 2014/10/29 21:14:21]

Is there a style recommendation for whether this should this go before or after
the private member variables?

[Dan Beam, 2014/10/29 22:39:07]

why is it not private?  because it's edited by the list?

[bondd, 2014/10/29 23:45:25]

Yes, it's edited by the list to tell the item which column was focused in the
previously focused item. Item can't figure it out by itself because it can't
(shouldn't) look into its siblings to see which column they had focused.

+
+    /**
      * Whether or not this item can be edited.
      * @type {boolean}
      * @private
@@ -74,7 +80,7 @@ cr.define('options', function() {
       this.editFields_ = [];
       this.addEventListener('mousedown', this.handleMouseDown_);
       this.addEventListener('keydown', this.handleKeyDown_);
-      this.addEventListener('leadChange', this.handleLeadChange_);
+      this.addEventListener('focusin', this.handleFocusIn_);
     },
 
     /** @override */
@@ -85,9 +91,8 @@ cr.define('options', function() {
     /**
      * Called when this element gains or loses 'lead' status. Updates editing
      * mode accordingly.
-     * @private
      */
-    handleLeadChange_: function() {
+    updateLeadState: function() {
       // Add focusability before call to updateEditState.
       if (this.lead) {
         this.setEditableValuesFocusable(true);
@@ -133,8 +138,7 @@ cr.define('options', function() {
 
         cr.dispatchSimpleEvent(this, 'edit', true);
 
-        var focusElement = this.editClickTarget_ || this.initialFocusElement;
-        this.editClickTarget_ = null;
+        var focusElement = this.getEditFocusElement_();
         if (focusElement)
           this.focusAndMaybeSelect_(focusElement);
       } else {
@@ -153,6 +157,42 @@ cr.define('options', function() {
     },
 
     /**
+     * Return editable element that should be focused, or null for none.
+     * @private
+     */
+    getEditFocusElement_: function() {
+      // If an edit field was clicked on then use the clicked element.
+      if (this.editClickTarget_) {
+        var result = this.editClickTarget_;
+        this.editClickTarget_ = null;
+        return result;
+      }
+
+      // If focusedColumnIndex is valid then use the element in that column.
+      if (this.focusedColumnIndex != -1) {
+        var nearestColumn =
+            this.getNearestColumnByIndex_(this.focusedColumnIndex);
+        if (nearestColumn)
+          return nearestColumn;
+      }
+
+      // It's possible that focusedColumnIndex hasn't been updated yet.
+      // Check getFocusedColumnIndex_ directly.
+      // This can't completely replace the above focusedColumnIndex check
+      // because InlineEditableItemList may have set focusedColumnIndex to a
+      // different value.
+      var columnIndex = this.getFocusedColumnIndex_();
+      if (columnIndex != -1) {
+        var nearestColumn = this.getNearestColumnByIndex_(columnIndex);
+        if (nearestColumn)
+          return nearestColumn;
+      }
+
+      // Everything else failed so return the default.
+      return this.initialFocusElement;
+    },
+
+    /**
      * Focus on the specified element, and select the editable text in it
      * if possible.
      * @param {!Element} control An element to be focused.
@@ -343,6 +383,25 @@ cr.define('options', function() {
     },
 
     /**
+     * Set the column index for a child element of this InlineEditableItem.
+     * Only elements with a column index will be keyboard focusable, e.g. by
+     * pressing the tab key.
+     * @param {Element} element Element whose column index to set. Method does
+     *     nothing if element is null.
+     * @param {number} columnIndex The new column index to set on the element.
+     *     -1 removes the column index.
+     */
+    setFocusableColumnIndex: function(element, columnIndex) {
+      if (!element)
+        return;
+
+      if (columnIndex >= 0)
+        element.setAttribute('inlineeditable-column', columnIndex);
+      else
+        element.removeAttribute('inlineeditable-column');
+    },
+
+    /**
      * Resets the editable version of any controls created by createEditable*
      * to match the static text.
      * @private
@@ -384,6 +443,40 @@ cr.define('options', function() {
     },
 
     /**
+     * Returns the index of the column that currently has focus, or -1 if no
+     * column has focus.
+     * @return {number}
+     * @private
+     */
+    getFocusedColumnIndex_: function() {
+      var element = document.activeElement;
+      if (element.editableVersion)
+        element = element.editableVersion;
+
+      if (element.hasAttribute('inlineeditable-column'))
+        return element.getAttribute('inlineeditable-column');

[Dan Beam, 2014/10/29 22:39:07]

this returns a string, not a number

[bondd, 2014/10/29 23:45:25]

Done. Is there a better way to do this, maybe by storing the attribute as a
number somehow? This seems to be the recommended way to do things, but maybe I
am missing something.

[Dan Beam, 2014/10/30 19:34:45]

no, setAttribute() always stringifies.  you could save it in JS only (e.g. just
element.editColumn_) but you'd have to change the way that you querySelector()
for [inlineeditable-column] (which would probably be fine).

[bondd, 2014/10/30 20:34:06]

So is it fine the way it is now in patch set #2 (with the parseInt call), or
would you like me to change this somehow?

+      return -1;
+    },
+
+    /**
+     * Returns the element from the column that has the largest index where:
+     * where:
+     *   + index <= startIndex, and
+     *   + the element exists, and
+     *   + the element is not disabled
+     * @return {Element}
+     * @private
+     */
+    getNearestColumnByIndex_: function(startIndex) {
+      for (var i = startIndex; i >= 0; --i) {
+        var el = this.querySelector('[inlineeditable-column="' + i + '"]');
+        if (el && !el.disabled)
+          return el;
+      }
+      return null;
+    },
+
+    /**
      * Called when a key is pressed. Handles committing and canceling edits.
      * @param {Event} e The key down event.
      * @private
@@ -451,6 +544,24 @@ cr.define('options', function() {
       if (editClickField && !editClickField.disabled)
         this.editClickTarget_ = editClickField;
     },
+
+    /**
+     * Called when this InlineEditableItem or any of its children are given
+     * focus. Updates focusedColumnIndex with the index of the newly focused
+     * column, or -1 if the focused element does not have a column index.
+     * @param {Event} e The focusin event.
+     * @private
+     */
+    handleFocusIn_: function(e) {
+      var target = e.target;
+      if (target.editableVersion)
+        target = target.editableVersion;
+
+      if (target.hasAttribute('inlineeditable-column'))
+        this.focusedColumnIndex = target.getAttribute('inlineeditable-column');

[Dan Beam, 2014/10/29 22:39:07]

this is not a number, it's a string

[bondd, 2014/10/29 23:45:25]

Done.

+      else
+        this.focusedColumnIndex = -1;
+    },
   };
 
   /**
@@ -507,6 +618,7 @@ cr.define('options', function() {
           // Add focusability before making other changes.
           leadItem.setEditableValuesFocusable(true);
           leadItem.setCloseButtonFocusable(true);
+          leadItem.focusedColumnIndex = -1;
           leadItem.updateEditState();
           // Remove focusability after making other changes.
           leadItem.setStaticValuesFocusable(false);
@@ -523,6 +635,32 @@ cr.define('options', function() {
     },
 
     /**
+     * Handles a change of the lead item from the selection model.
+     * @param {Event} e The property change event.

[Dan Beam, 2014/10/29 22:39:07]

don't duplicate doc if this is an @override

[bondd, 2014/10/29 23:45:25]

Done.

+     * @override
+     */
+    handleLeadChange: function(e) {
+      DeletableItemList.prototype.handleLeadChange.call(this, e);
+
+      var focusedColumnIndex = -1;
+      if (e.oldValue != -1) {
+        var element = this.getListItemByIndex(e.oldValue);
+        if (element) {
+          focusedColumnIndex = element.focusedColumnIndex;
+          element.updateLeadState();
+        }
+      }
+
+      if (e.newValue != -1) {
+        var element = this.getListItemByIndex(e.newValue);
+        if (element) {
+          element.focusedColumnIndex = focusedColumnIndex;
+          element.updateLeadState();
+        }
+      }
+    },
+
+    /**
      * Called after the DataModel for the list has been set.
      * @override
      */