Improve Tab-key navigation for editing Autofill addresses and credit cards.

Improve Tab-key navigation for editing Autofill addresses and credit cards.

Also clean up some of the WebUI list code.

BUG=78773
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102055

[Ilya Sherman, 2011-09-15 04:10:39]

The diffs here are a little confusing, so I've annotated them with intents.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_options_list.js (left):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_options_list.js:311: },
This functionality is now in |handleListFocusChange_| (the slight delay is
already taken care of by list.js).

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/inline_editable_list.js (left):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:95: },
This was moved to deletable_item_list.js.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:128:
this.removeAttribute('editing');
This if/else got merged with the if/else below.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:145: if (self.editing
&& self.focusPlaceholder) {
List items don't have a 'focusPlaceholder' property.  I'm not sure what the
original intent of this code was...

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/js/cr/ui/list.js (left):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/js/cr/ui/list.js:866: // event to trigger
redrawing the list, we won't end up with empty space.
This comment is also included within the implementation of
getItemsInViewPort()... no need to have it twice.

[James Hawkins, 2011-09-15 19:28:02]

+arv since he wrote list.js

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_options_list.js:325: if (selectedItem)
Is there nothing in |e| that gives us better data for mouse vs keyboard?

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_options_list.js:330:
this.selectionModel.unselectAll();
Hmm. Why unselect?  Blur doesn't unselect on other lists.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/deletable_item_list.js:80: * Called when the
input element receives focus. Selects this item in the
s/input element/close button element/

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/inline_editable_list.js (left):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:128:
this.removeAttribute('editing');
On 2011/09/15 04:10:39, Ilya Sherman wrote:
> This if/else got merged with the if/else below.

This block was intentionally kept separate from below to make sure the
connection with set/remove editing attribute was not lost.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:145: if (self.editing
&& self.focusPlaceholder) {
On 2011/09/15 04:10:39, Ilya Sherman wrote:
> List items don't have a 'focusPlaceholder' property.  I'm not sure what the
> original intent of this code was...

But InlineEditableLists do...

[Ilya Sherman, 2011-09-15 20:39:01]

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/autofill_options_list.js (right):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_options_list.js:325: if (selectedItem)
On 2011/09/15 19:28:02, James Hawkins wrote:
> Is there nothing in |e| that gives us better data for mouse vs keyboard?

Not that I could find.  If you know of something, please let me know!

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/autofill_options_list.js:330:
this.selectionModel.unselectAll();
On 2011/09/15 19:28:02, James Hawkins wrote:
> Hmm. Why unselect?  Blur doesn't unselect on other lists.

I personally think blur should unselect *all* lists... but this is just
maintaining parity with what we had before (see the red section above).

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/deletable_item_list.js (right):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/deletable_item_list.js:80: * Called when the
input element receives focus. Selects this item in the
On 2011/09/15 19:28:02, James Hawkins wrote:
> s/input element/close button element/

Generalized slightly to "focusable child element", since this is also used by
the InlineEditableItem code.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/inline_editable_list.js (left):

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:128:
this.removeAttribute('editing');
On 2011/09/15 19:28:02, James Hawkins wrote:
> On 2011/09/15 04:10:39, Ilya Sherman wrote:
> > This if/else got merged with the if/else below.
> 
> This block was intentionally kept separate from below to make sure the
> connection with set/remove editing attribute was not lost.

Ok, restored, though I find the second block harder to read with this one
separated out from it.

http://codereview.chromium.org/7903001/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/inline_editable_list.js:145: if (self.editing
&& self.focusPlaceholder) {
On 2011/09/15 19:28:02, James Hawkins wrote:
> On 2011/09/15 04:10:39, Ilya Sherman wrote:
> > List items don't have a 'focusPlaceholder' property.  I'm not sure what the
> > original intent of this code was...
> 
> But InlineEditableLists do...

InlineEditableItemLists have this property.  InlineEditableItems do not.  |self|
is a list item, not a list.  Am I missing something there?

[Ilya Sherman, 2011-09-19 21:24:21]

Ping.

[James Hawkins, 2011-09-19 23:00:06]

LGTM

http://codereview.chromium.org/7903001/diff/5001/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (left):

http://codereview.chromium.org/7903001/diff/5001/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:145: if (self.editing
&& self.focusPlaceholder) {
You're right. I checked it out, and this check is wrong and unnecessary if it
were modified to be correct (self.parent.focusPlaceholder).

[Ilya Sherman, 2011-09-19 23:57:43]

arv, would you like me to wait for your review?  (Will commit tomorrow if I
don't hear from you.)

[commit-bot: I haz the power, 2011-09-20 23:49:25]

Change committed as 102055