Improve keyboard navigation on 'Manage search engines' options page

Improve keyboard navigation on 'Manage search engines' options page
1. Selected column is maintained when pressing up/down in list to
    change selected list item.
2. Shift+Tab now returns to to previously focused list item field
   (wherever Tab just came from) instead of always returning to the
   first field of the list item.
3. 'Default search strings' list now starts with first text field of
    first item focused.
4. 'Add new search engine' list item's focus order now behaves the same
    as other list items.
5. 'X' delete button is now part of the focus order.
BUG=276874

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

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:105: set
focusable(focusable) {
Add a getter for completeness, even though we never use it? Or omit getter so
we're not adding dead code?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:325:
control.staticVersion = staticElement;
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?

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:597: handleLeadChange: function(pe) {
I made this public because I need to hook it in InlineEditableItemList. It seems
private vs. public for handlers in this class is a bit random: for example
handleDragStart, handleKeyDown, handleMouseDown are public, while
handleDoubleClick_ and handlePointerDownUp_ are private. It seems to follow
convention that if I want to override handleLeadChange I should just make it
public here. Or am I missing something?

[bondd, 2014-10-17 01:24:00]

bondd@chromium.org changed reviewers:
+ dbeam@chromium.org

[bondd, 2014-10-17 01:24:00]

bondd@chromium.org changed required reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-10-18 01:35:42]

dbeam@chromium.org changed reviewers:
+ arv@chromium.org

[Dan Beam, 2014-10-18 01:35:45]

+arv@ for a second set of eyes on all this List magic

in general I think this CL should be split up further (you list 5 separate
things; with some creativity i'll bet they can be independently split so that
they can land separately and still enhance the experience).

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:67:
this.closeButtonFocusable = true;
what's the benefit to this very specific accessor over just using the current
|closeButtonElement| like this:

  this.closeButtonElement.tabIndex = 0;

or

  this.closeButtonElement.removeAttribute('tabindex');

?  tabIndex will always be public (unless we attempt to change this), so it
seems odd to try to hide it otherwise...

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/deletable_item_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:108: set
closeButtonFocusable(value) {
nit: value => focusable

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/deletable_item_list.js:108: set
closeButtonFocusable(value) {
nit:

  if (focusable != this.closeButtonFocusable_)
    return;

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (left):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:248: }
did you mean to remove this?  because you left |textEl| lying around if so...

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:82: ignoreMouseUp_:
false,
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)

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:90:
this.addEventListener('mouseup', this.handleMouseUp_);
why is this necessary?  what are we preventing the default for?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:105: set
focusable(focusable) {
On 2014/10/17 01:23:15, bondd wrote:
> Add a getter for completeness, even though we never use it? Or omit getter so
> we're not adding dead code?

remove until needed

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:115: return
this.focusedColumnIndex_;
wrong indent

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:118:
this.focusedColumnIndex_ = focusedColumnIndex;
how is this useful over just a normal property?  e.g.

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

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:149: 
can you add a comment to what all of this is doing?  maybe inside of each
conditional branch?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:153:
this.editClickTarget_ = null;
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...

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:154: }
} else if (...) {

(else on same line)

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:325:
control.staticVersion = staticElement;
On 2014/10/17 01:23:15, bondd wrote:
> 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?

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.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:339: if(element) {
if (!element)
  return;

...

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:407:
editFields[i].tabIndex = newTabIndex;
nit: curlies around 1-line for loops (no curlies around 1-line if/else)

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:417: if
(document.activeElement &&
there will always be an activeElement, despite whatever Mozilla Developer
Network page you've seen saying this could be null.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:436: if (el &&
!el.disabled)
nit: maybe add !el.hidden here unless you know that can never happen?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:483: if (!this.editable
|| this.editing || !e.target)
when can !e.target ever happen?  is there some test that's calling this in a
funky way?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:507: if
(this.ignoreMouseUp_) {
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 :(

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:521:
if(e.target.hasAttribute('inlineeditable-column')) {
if( => if (

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:525: else
} else {
  // even though this part is a 1-liner, has curlies because top does
}

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:566:
this.removeAttribute('tabIndex');
tabindex

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:589: * @param {Event}
pe The property change event.
pe => e, event, pce (in order of preference)

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:598: if ((element =
this.getListItemByIndex(pe.oldValue))) {
don't do assignment in conditionals

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:613: * Called after the
DataModel for the list has been set.
* @override

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.css (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.css:315: background-color:
rgb(255, 255, 255);
^ why is this necessary?  also, why not "white"?

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/search_engine_manager_engine_list.js:142: var
makeDefaultButtonEl = null;
probably don't need the "= null"

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:115: if (this.dataModel_ != dataModel) {
nit: change to:

  if (this.dataModel_ == dataModel)
    return;

(I assume your method *shouldn't* be called in this case)

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:149: }
indent off

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:158: * dataModel has been set.
nit: Override to be notified when |this.dataModel| is set.

or something more direct.  it's pretty clear that this is optionally overridden
as it's not abstract.

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:597: handleLeadChange: function(pe) {
On 2014/10/17 01:23:15, bondd wrote:
> I made this public because I need to hook it in InlineEditableItemList. It
seems
> private vs. public for handlers in this class is a bit random: for example
> handleDragStart, handleKeyDown, handleMouseDown are public, while
> handleDoubleClick_ and handlePointerDownUp_ are private. It seems to follow
> convention that if I want to override handleLeadChange I should just make it
> public here. Or am I missing something?

why can't the derived class just listen to 'leadChange' events?  

it seems like both construction and decoration ensure the base class subscribes
to events first, basically giving you the exact same code (functionally).

this is what the current code does (but both the derived and base classes have
methods named handleLeadChange_, which can lead to some quirky results).

[Dan Beam, 2014-10-25 00:26:51]

what's holding this CL up?

[bondd, 2014-10-27 13:42:04]

On 2014/10/25 00:26:51, Dan Beam wrote:
> what's holding this CL up?

I did some further testing while I was splitting the CL and found issues with
staticVersion in content_settings_exception_area.js (my change broke some
things). I changed my approach to accommodate the new issues. It works now, I'm
just cleaning it up and splitting it into two pieces. I hope to have it uploaded
today or tomorrow.

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

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:526:
this.focusColumnIndex_ = -1;
focusedColumnIndex_

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:600:
element.updateLeadState_();
updateLeadState_ is private to a different class

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:607:
element.updateLeadState_();
updateLeadState_ is private to a different class

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:613: * Called after the
DataModel for the list has been set.
On 2014/10/18 01:35:43, Dan Beam wrote:
> * @override

Done.

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:115: if (this.dataModel_ != dataModel) {
On 2014/10/18 01:35:44, Dan Beam wrote:
> nit: change to:
> 
>   if (this.dataModel_ == dataModel)
>     return;
> 
> (I assume your method *shouldn't* be called in this case)

Done.

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:149: }
On 2014/10/18 01:35:45, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:158: * dataModel has been set.
On 2014/10/18 01:35:44, Dan Beam wrote:
> nit: Override to be notified when |this.dataModel| is set.
> 
> or something more direct.  it's pretty clear that this is optionally
overridden
> as it's not abstract.

Done.

[bondd, 2014-10-28 22:32:52]

This CL is going to die and be replaced by several smaller ones.

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

Responded to reviewer comments from dbeam@.
This CL has been split into two smaller ones, but I responded to the reviewer
comments on this one too in case that's helpful.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:149: 
On 2014/10/18 01:35:44, Dan Beam wrote:
> can you add a comment to what all of this is doing?  maybe inside of each
> conditional branch?

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:339: if(element) {
On 2014/10/18 01:35:44, Dan Beam wrote:
> if (!element)
>   return;
> 
> ...

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:436: if (el &&
!el.disabled)
On 2014/10/18 01:35:44, Dan Beam wrote:
> nit: maybe add !el.hidden here unless you know that can never happen?

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.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:483: if (!this.editable
|| this.editing || !e.target)
On 2014/10/18 01:35:44, Dan Beam wrote:
> when can !e.target ever happen?  is there some test that's calling this in a
> funky way?

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:521:
if(e.target.hasAttribute('inlineeditable-column')) {
On 2014/10/18 01:35:44, Dan Beam wrote:
> if( => if (

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:526:
this.focusColumnIndex_ = -1;
On 2014/10/28 22:31:27, bondd wrote:
> focusedColumnIndex_

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:589: * @param {Event}
pe The property change event.
On 2014/10/18 01:35:44, Dan Beam wrote:
> pe => e, event, pce (in order of preference)

Done.

https://codereview.chromium.org/664583006/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:598: if ((element =
this.getListItemByIndex(pe.oldValue))) {
On 2014/10/18 01:35:44, Dan Beam wrote:
> don't do assignment in conditionals

Done.

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/664583006/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/list.js:597: handleLeadChange: function(pe) {
On 2014/10/18 01:35:45, Dan Beam wrote:
> On 2014/10/17 01:23:15, bondd wrote:
> > I made this public because I need to hook it in InlineEditableItemList. It
> seems
> > private vs. public for handlers in this class is a bit random: for example
> > handleDragStart, handleKeyDown, handleMouseDown are public, while
> > handleDoubleClick_ and handlePointerDownUp_ are private. It seems to follow
> > convention that if I want to override handleLeadChange I should just make it
> > public here. Or am I missing something?
> 
> why can't the derived class just listen to 'leadChange' events?  
> 
> it seems like both construction and decoration ensure the base class
subscribes
> to events first, basically giving you the exact same code (functionally).
> 
> this is what the current code does (but both the derived and base classes have
> methods named handleLeadChange_, which can lead to some quirky results).

Maybe you are confused about the inheritance here (or hey, maybe I am). Current
code listens to 'leadChange' in InlineEditableItem (which is not derived from
this List class). I need to hook handleLeadChange in InlineEditableItemList
(which is derived from this List class). Current code
InlineEditableItem.handleLeadChange_ doesn't overlap with this one, because
neither class inherits from the other.

Based on that info, does it still make sense to listen for 'leadChange' in
InlineEditableItemList vs. making handleLeadChange protected?

[Dan Beam, 2014-11-12 23:41:14]

can we close this issue now?

[bondd, 2014-11-12 23:59:55]

Closing this issue.
Replaced by
https://codereview.chromium.org/668603004/
https://codereview.chromium.org/666823003/
https://codereview.chromium.org/683813004/
https://codereview.chromium.org/688043003/