Makes the network dropdown keyboard accessible (focus friendly).

Makes the network dropdown keyboard accessible (focus friendly).

Also:
 * fixes drop-down buttons layout.
 * overlay for catching outside the menu clicks is implemented (mimics standard select control behavior).

BUG=chromium-os:18826
TEST=manual

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

[altimofeev, 2011-08-08 17:33:03]

PTAL

[xiyuan, 2011-08-08 20:07:43]

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:28:
this.parentNode.childNodes[1].focus();
Let's add two getters for title buttone and dropdown.

get titleButton() {
  return this.childNodes[1];  // or this.firstElementChild.nextElementSibling
},
get dropDownContainer() {
  return this.lastElementChild;
},

Then we could write something like this:
this.parentNode.titleButton.focus();

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:36: var container
= this.ownerDocument.createElement('div');
recommend to wrap the container into a class.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:53:
this.addEventListener('keydown', function (e) {
Could we put the logic into its own function since it's not trivial?

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:58:
switch(e.keyCode) {
do we need to handle "enter" key as well?

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:104: }
nit: no need for enclosing {} for one line branch.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:127:
container.firstItem = null;
clear container.selectedItem as well?

[altimofeev, 2011-08-09 10:32:30]

Thank you for the comments. PTAL.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:28:
this.parentNode.childNodes[1].focus();
On 2011/08/08 20:07:43, xiyuan wrote:
> Let's add two getters for title buttone and dropdown.
> 
> get titleButton() {
>   return this.childNodes[1];  // or this.firstElementChild.nextElementSibling
> },
> get dropDownContainer() {
>   return this.lastElementChild;
> },
> 
> Then we could write something like this:
> this.parentNode.titleButton.focus();

Done.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:36: var container
= this.ownerDocument.createElement('div');
On 2011/08/08 20:07:43, xiyuan wrote:
> recommend to wrap the container into a class.

Done.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:53:
this.addEventListener('keydown', function (e) {
On 2011/08/08 20:07:43, xiyuan wrote:
> Could we put the logic into its own function since it's not trivial?

Done.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:58:
switch(e.keyCode) {
On 2011/08/08 20:07:43, xiyuan wrote:
> do we need to handle "enter" key as well?

tl;dr. Done.

Enter is handled by JS directly, since the button has the focus. But processing
Enter here makes possible to distinguish Enter and click actions on the focused
button, which in turn allows to avoid 'mousedown' event usage. So no hack with
timer is needed to set focus.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:104: }
On 2011/08/08 20:07:43, xiyuan wrote:
> nit: no need for enclosing {} for one line branch.

Done.

http://codereview.chromium.org/7550070/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:127:
container.firstItem = null;
On 2011/08/08 20:07:43, xiyuan wrote:
> clear container.selectedItem as well?

Good point, this is useful for the case, when the menu is updated while being
shown.

[xiyuan, 2011-08-09 16:43:39]

Thank you for making all the changes.

One last thing I would like to ask is to try out a "blur" handler on the
titleButton element. We probably could get rid of the overlay element and tab
keydown handling by using it.

Other than this, LGTM.

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:60:
DropDownContainer.decorate(container);
nit: think you could combine line 59-60 like this:
var container = new DropDownContainer();

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:219:
this.focus();
Think a focusable element should automatically get focus when you click on it.
Do we need this focus call?

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:222: if
(this.enterPressed) {
Does this mean user needs to hit "enter" on the dropdown container and then
click on the title button? It's a bit confusing to user if that's the case.
Think we should auto close the container when enter is pressed.

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:237: * @param
{!Object} e Keydown event.
nit: {!Event}

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:266: case 9: { 
// Tab.
We should do this in "blur" handler instead of capturing tab. And if we use a
"blur" handler, we probably could get rid of the overlay as well.

[altimofeev, 2011-08-09 17:17:30]

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:60:
DropDownContainer.decorate(container);
On 2011/08/09 16:43:39, xiyuan wrote:
> nit: think you could combine line 59-60 like this:
> var container = new DropDownContainer();

Done.

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:219:
this.focus();
On 2011/08/09 16:43:39, xiyuan wrote:
> Think a focusable element should automatically get focus when you click on it.
> Do we need this focus call?

It doesn't get the focus, when menu is being opened. Actually, I don't know
why...

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:222: if
(this.enterPressed) {
On 2011/08/09 16:43:39, xiyuan wrote:
> Does this mean user needs to hit "enter" on the dropdown container and then
> click on the title button? It's a bit confusing to user if that's the case.
> Think we should auto close the container when enter is pressed.

Not exactly. There are 3 cases:
1. User clicks on the button - menu is opened or closed (no item action).
|this.enterPressed| == false.
2. User hits Enter, menu is shown - action for the selected item is fired. Menu
is being closed. |this.enterPressed| == true. (keyboard event is handled before
'onclick').
3. User hits Enter, menu is closed - menu is opened. |this.enterPressed| ==
false (see keydown handler).

General idea is that 
  hitting Enter on focused button == mouse click from events point of view

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:237: * @param
{!Object} e Keydown event.
On 2011/08/09 16:43:39, xiyuan wrote:
> nit: {!Event}

Done.

http://codereview.chromium.org/7550070/diff/1004/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:266: case 9: { 
// Tab.
On 2011/08/09 16:43:39, xiyuan wrote:
> We should do this in "blur" handler instead of capturing tab. And if we use a
> "blur" handler, we probably could get rid of the overlay as well.

1. As I have stated, button loses focus when menu is being shown. So processing
blur is not so simple.
2. Standard select do not lose the focus, when user clicks outside of the list.
List is closed, but focus stays with the select element. Also, no other action
is being acted (you can't open another select or push the button while one
select is opened).
3. Adding blur handler will involve more code, so current implementation looks
better for me.

[xiyuan, 2011-08-09 17:28:18]

LGTM

Okay. Let's get this in first.