Improve keyboard navigation on 'Manage search engines' options page

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 49 matching lines @@
     editCancelled_: true,
 
     /**
      * The editable item corresponding to the last click, if any. Used to decide
      * initial focus when entering edit mode.
      * @type {HTMLElement}
      * @private
      */
     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 */
     selectionChanged: function() {
       this.updateEditState();
     },
 
     /**
-     * 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,

+    },
+
+    /**
      * Updates the edit state based on the current selected and lead states.
      */
     updateEditState: function() {
       if (this.editable)
         this.editing = this.selected && this.lead;
     },
 
     /**
      * Whether the user is currently editing the list item.
      * @type {boolean}
      */
     get editing() {
       return this.hasAttribute('editing');
     },
     set editing(editing) {
       if (this.editing == editing)
         return;
 
       if (editing)
         this.setAttribute('editing', '');
       else
         this.removeAttribute('editing');
 
       if (editing) {
         this.editCancelled_ = false;
 
         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;
           // 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() {
@@ skipping 97 matching lines @@
      * 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;
-      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 {
         // At this point |this| is not attached to the parent list yet, so give
         // a short timeout in order for the attachment to occur.
         var self = this;
         window.setTimeout(function() {
           var list = self.parentNode;
           if (list && list.focusPlaceholder) {
             list.focusPlaceholder = false;
@@ skipping 19 matching lines @@
     },
 
     /**
      * 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;

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

staticVersion is only set by content_settings_exception_area.js. If that list
works without staticVersion we could get rid of staticVersion and all code
related to it. Is that something we want to do, or is it better to keep
staticVersion support around in case it’s useful in the future?

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

if there's only 1 user of an API with no immediate plans otherwise (I haven't
heard any), yes we should remove it.  I would recommend doing it separately,
though.

       this.editFields_.push(control);
     },
 
     /**
+     * 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
      */
     resetEditableValues_: function() {
       var editFields = this.editFields_;
       for (var i = 0; i < editFields.length; i++) {
         var staticLabel = editFields[i].staticVersion;
         if (!staticLabel && !this.isPlaceholder)
           continue;
@@ skipping 21 matching lines @@
         if (!staticLabel)
           continue;
 
         if (editFields[i].tagName == 'INPUT')
           staticLabel.textContent = editFields[i].value;
         // Add more tag types here as new createEditable* methods are added.
       }
     },
 
     /**
+     * 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
      */
     handleKeyDown_: function(e) {
       if (!this.editing)
         return;
 
       var endEdit = false;
       var handledKey = true;
@@ skipping 20 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 || 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;
       var editFields = this.editFields_;
       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];
+          // 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.

+    },
   };
 
   /**
    * 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) {
         return node instanceof InlineEditableItem;
@@ skipping 18 matching lines @@
      * @type {boolean}
      */
     focusPlaceholder: false,
 
     /** @override */
     decorate: function() {
       DeletableItemList.prototype.decorate.call(this);
       this.setAttribute('inlineeditable', '');
       this.addEventListener('hasElementFocusChange',
                             this.handleListFocusChange_);
+      this.removeAttribute('tabIndex'); 

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

tabindex

     },
 
     /**
      * 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) {
+    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.
      */
     shouldFocusPlaceholder: function() {
       return true;
     },
   };
 
   // Export
   return {
     InlineEditableItem: InlineEditableItem,
     InlineEditableItemList: InlineEditableItemList,
   };
 });