Improve keyboard navigation on 'Manage search engines' options page

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 89d10e7309592acb2871034315782d7fcaef7e6f..2ef2b7878c20de74929912d6946043a72cec311e 100644
--- a/chrome/browser/resources/options/inline_editable_list.js
+++ b/chrome/browser/resources/options/inline_editable_list.js
@@ -67,14 +67,29 @@ cr.define('options', function() {
      */
     editClickTarget_: null,
 
+    /**
+     * Index of currently focused column, or -1 for none.
+     * @type {number}
+     * @private
+     */
+    focusedColumnIndex_: -1,
+
+    /**
+     * Whether or not to ignore the next mouseup event.
+     * @type {boolean}
+     * @private
+     */
+    ignoreMouseUp_: false,

[Dan Beam, 2014/10/18 01:35:44]

from this class' point of view, isn't this actually more preventNextMouseUp_?

I'd expect the usage of something called |ignoreMouseUp_| to be:

  handleMouseUp_: function(e) {
    if (this.ignoreMouseUp_) {
      this.ignoreMouseUp_ = false;
      return;
    }
    ...
  },

(i.e. no preventDefault() call)

+
     /** @override */
     decorate: function() {
       DeletableItem.prototype.decorate.call(this);
 
       this.editFields_ = [];
       this.addEventListener('mousedown', this.handleMouseDown_);
+      this.addEventListener('mouseup', this.handleMouseUp_);

[Dan Beam, 2014/10/18 01:35:43]

why is this necessary?  what are we preventing the default for?

       this.addEventListener('keydown', this.handleKeyDown_);
-      this.addEventListener('leadChange', this.handleLeadChange_);
+      this.addEventListener('focusin', this.handleFocusIn_);
     },
 
     /** @override */
@@ -83,12 +98,24 @@ cr.define('options', function() {
     },
 
     /**
-     * Called when this element gains or loses 'lead' status. Updates editing
-     * mode accordingly.
-     * @private
+     * Whether the fields of this InlineEditableItem can be focused using the
+     * keyboard, e.g. by pressing the tab key.
+     * @type {boolean}
      */
-    handleLeadChange_: function() {
-      this.updateEditState();
+    set focusable(focusable) {

[bondd, 2014/10/17 01:23:15]

Add a getter for completeness, even though we never use it? Or omit getter so
we're not adding dead code?

[Dan Beam, 2014/10/18 01:35:43]

remove until needed

+      this.setEditableValuesFocusable_(focusable);
+      this.closeButtonFocusable = focusable; 
+    },
+
+    /**
+     * Index of currently focused column, or -1 for none.
+     * @type {number}
+     */
+    get focusedColumnIndex() {
+       return this.focusedColumnIndex_;

[Dan Beam, 2014/10/18 01:35:44]

wrong indent

+    },
+    set focusedColumnIndex(focusedColumnIndex) {
+      this.focusedColumnIndex_ = focusedColumnIndex;

[Dan Beam, 2014/10/18 01:35:44]

how is this useful over just a normal property?  e.g.

  /**
   * Doc comment.
   * @type {number}
   */
  focusColumnIndex: -1,

     },
 
     /**
@@ -120,8 +147,20 @@ cr.define('options', function() {
 
         cr.dispatchSimpleEvent(this, 'edit', true);
 

[Dan Beam, 2014/10/18 01:35:44]

can you add a comment to what all of this is doing?  maybe inside of each
conditional branch?

[bondd, 2014/10/29 21:12:39]

Done.

-        var focusElement = this.editClickTarget_ || this.initialFocusElement;
-        this.editClickTarget_ = null;
+        var focusElement = this.initialFocusElement;
+        if (this.editClickTarget_) {
+          focusElement = this.editClickTarget_;
+          this.editClickTarget_ = null;

[Dan Beam, 2014/10/18 01:35:44]

so why is this code better?  is it subtly different from:

  var focusElement = this.editClickTarget_ || this.initialFocusElement;
  this.editClickTarget_ = null;

?  it seems to be functionally equivalent to the previous code but harder to
understand...

+        }

[Dan Beam, 2014/10/18 01:35:44]

} else if (...) {

(else on same line)

+        else if (this.focusedColumnIndex_ != -1) {
+          focusElement =
+              this.getNearestColumnByIndex_(this.focusedColumnIndex_);
+        }
+        else {
+          var focusedColumn = this.getFocusedColumn_();
+          if (focusedColumn)
+            focusElement = focusedColumn;
+        }
 
         if (focusElement) {
           var self = this;
@@ -239,17 +278,11 @@ cr.define('options', function() {
       var container = /** @type {HTMLElement} */(
           this.ownerDocument.createElement('div'));
       var textEl = null;
-      if (!this.isPlaceholder) {
-        textEl = this.ownerDocument.createElement('div');
-        textEl.className = 'static-text';
-        textEl.textContent = text;
-        textEl.setAttribute('displaymode', 'static');
-        container.appendChild(textEl);
-      }

[Dan Beam, 2014/10/18 01:35:44]

did you mean to remove this?  because you left |textEl| lying around if so...

 
       var inputEl = this.ownerDocument.createElement('input');
       inputEl.type = 'text';
       inputEl.value = text;
+      inputEl.tabIndex = -1;
       if (!this.isPlaceholder) {
         inputEl.setAttribute('displaymode', 'edit');
       } else {
@@ -294,6 +327,24 @@ 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) {

[Dan Beam, 2014/10/18 01:35:44]

if (!element)
  return;

...

[bondd, 2014/10/29 21:12:39]

Done.

+        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
@@ -335,6 +386,60 @@ cr.define('options', function() {
     },
 
     /**
+     * Called when lead status is gained/lost.
+     * @private
+     */
+    updateLeadState_: function() {
+      this.focusable = this.lead;
+      this.updateEditState();
+    },
+
+    /**
+     * Sets whether the editable values can be given focus using the keyboard,
+     * e.g. by pressing the tab key.
+     * @param {boolean}
+     * @private
+     */
+    setEditableValuesFocusable_: function(focusable) {
+      var newTabIndex = focusable ? 0 : -1;
+      var editFields = this.editFields_;
+      for (var i = 0; i < editFields.length; i++)
+        editFields[i].tabIndex = newTabIndex;

[Dan Beam, 2014/10/18 01:35:44]

nit: curlies around 1-line for loops (no curlies around 1-line if/else)

+    },
+
+    /**
+     * Returns the element in the column that currently has focus, or null
+     * if no column has focus.
+     * @return {Element}
+     * @private
+     */
+    getFocusedColumn_: function() {
+      if (document.activeElement && 

[Dan Beam, 2014/10/18 01:35:43]

there will always be an activeElement, despite whatever Mozilla Developer
Network page you've seen saying this could be null.

+          document.activeElement.hasAttribute('inlineeditable-column')) {
+        return document.activeElement;
+      }
+      return null;      
+    },
+    
+    /**
+     * 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)

[Dan Beam, 2014/10/18 01:35:44]

nit: maybe add !el.hidden here unless you know that can never happen?

[bondd, 2014/10/29 21:12:39]

Under the new split CL it's okay for the element to be hidden. When the static
element is shown and the editable element is hidden we still want to get the
columnIndex from the editable element here.

+          return el;
+      }
+      return null;
+    },
+
+    /**
      * Called when a key is pressed. Handles committing and canceling edits.
      * @param {Event} e The key down event.
      * @private
@@ -375,7 +480,7 @@ cr.define('options', function() {
      * @private
      */
     handleMouseDown_: function(e) {
-      if (!this.editable || this.editing)
+      if (!this.editable || this.editing || !e.target)

[Dan Beam, 2014/10/18 01:35:44]

when can !e.target ever happen?  is there some test that's calling this in a
funky way?

[bondd, 2014/10/29 21:12:39]

Done.

         return;
 
       var clickTarget = e.target;
@@ -386,10 +491,40 @@ cr.define('options', function() {
         if (editFields[i] == clickTarget ||
             editFields[i].staticVersion == clickTarget) {
           this.editClickTarget_ = editFields[i];
+          // Prevent mouse up event from deselecting text.
+          this.ignoreMouseUp_ = true;
           return;
         }
       }
     },
+
+    /**
+     * Called when mouse up happens on the list item or any of its children.
+     * @param {Event} e The mouse up event.
+     * @private
+     */
+    handleMouseUp_: function(e) {
+      if (this.ignoreMouseUp_) {

[Dan Beam, 2014/10/18 01:35:44]

so what happens if you mouse down inside the item and mouse up outside of the
window?  i'll bet this code doesn't work :(

+        this.ignoreMouseUp_ = false;
+        e.preventDefault();
+      }
+    },
+
+    /**
+     * 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) {
+      if(e.target.hasAttribute('inlineeditable-column')) {

[Dan Beam, 2014/10/18 01:35:44]

if( => if (

[bondd, 2014/10/29 21:12:39]

Done.

+        this.focusedColumnIndex_ = 
+            e.target.getAttribute('inlineeditable-column');
+      }
+      else

[Dan Beam, 2014/10/18 01:35:43]

} else {
  // even though this part is a 1-liner, has curlies because top does
}

+        this.focusColumnIndex_ = -1;

[bondd, 2014/10/28 22:31:27]

focusedColumnIndex_

[bondd, 2014/10/29 21:12:39]

Done.

+    },
   };
 
   /**
@@ -428,6 +563,7 @@ cr.define('options', function() {
       this.setAttribute('inlineeditable', '');
       this.addEventListener('hasElementFocusChange',
                             this.handleListFocusChange_);
+      this.removeAttribute('tabIndex'); 

[Dan Beam, 2014/10/18 01:35:43]

tabindex

     },
 
     /**
@@ -436,17 +572,55 @@ cr.define('options', function() {
      * @param {Event} e The change event.
      * @private
      */
-    handleListFocusChange_: function(e) {
+    handleListFocusChange_: function(e) { 
       var leadItem = this.getListItemByIndex(this.selectionModel.leadIndex);
       if (leadItem) {
-        if (e.newValue)
+        if (e.newValue) {
+          leadItem.focusedColumnIndex = -1;
           leadItem.updateEditState();
+        }
         else
           leadItem.editing = false;
       }
     },
 
     /**
+     * Handles a change of the lead item from the selection model.
+     * @param {Event} pe The property change event.

[Dan Beam, 2014/10/18 01:35:44]

pe => e, event, pce (in order of preference)

[bondd, 2014/10/29 21:12:39]

Done.

+     * @override
+     */
+    handleLeadChange: function(pe) {
+      DeletableItemList.prototype.handleLeadChange.call(this, pe);
+
+      var focusedColumnIndex = -1;
+      var element;
+      if (pe.oldValue != -1) {
+        if ((element = this.getListItemByIndex(pe.oldValue))) {

[Dan Beam, 2014/10/18 01:35:44]

don't do assignment in conditionals

[bondd, 2014/10/29 21:12:39]

Done.

+          focusedColumnIndex = element.focusedColumnIndex;
+          element.updateLeadState_();

[bondd, 2014/10/28 22:31:27]

updateLeadState_ is private to a different class

+        }
+      }
+      
+      if (pe.newValue != -1) {
+        if ((element = this.getListItemByIndex(pe.newValue))) {
+          element.focusedColumnIndex = focusedColumnIndex;
+          element.updateLeadState_();

[bondd, 2014/10/28 22:31:27]

updateLeadState_ is private to a different class

+        }
+      }
+    },
+
+    /**
+     * Called after the DataModel for the list has been set.

[Dan Beam, 2014/10/18 01:35:43]

* @override

[bondd, 2014/10/28 22:31:27]

Done.

+     */
+    onSetDataModelComplete: function() {
+      DeletableItemList.prototype.onSetDataModelComplete.call(this);
+          
+      var firstItem = this.getListItemByIndex(0);
+      if (firstItem)
+        firstItem.focusable = true;
+    },
+
+    /**
      * May be overridden by subclasses to disable focusing the placeholder.
      * @return {boolean} True if the placeholder element should be focused on
      *     edit commit.