Make the OOBE network menu accessible by improving keyboard handling

Make the OOBE network menu accessible by improving keyboard handling
and adding ARIA attributes.

BUG=96906
TEST=manual testing

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

[zel, 2011-09-16 21:47:32]

LGTM

[altimofeev, 2011-09-19 08:13:14]

Network drop-down is also used in 'screen_offline_message.html'. Please
's/div/label/' there as well.

http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/network_dropdown.js (left):

http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/network_dropdown.js:257: if
(!this.isShown)
Why do you need handle keys if menu isn't shown?

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

http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/network_dropdown.js:41:
'aria-activedescendant', this.id + selectedItem.id);
Shouldn't it be just 'selectedItem.id'?

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

http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe_screen_network.html:17: <label
for="networks-list-dropdown" i18n-content="selectNetwork"
Please check that this change doesn't break the menu layout.

[dmazzoni, 2011-09-19 17:17:58]

On Mon, Sep 19, 2011 at 1:13 AM, <altimofeev@chromium.org> wrote:

> http://codereview.chromium.**org/7917015/diff/1/chrome/**
>
browser/resources/chromeos/**login/network_dropdown.js#**oldcode257<http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chromeos/login/network_dropdown.js#oldcode257>
> chrome/browser/resources/**chromeos/login/network_**dropdown.js:257: if
> (!this.isShown)
> Why do you need handle keys if menu isn't shown?
>

On a native <select> control, you can change the value without opening it -
just press Tab to focus it and then press the up or down arrows to change
the value. With the changes I made, this works; you can change the value
with the keyboard whether it's open or not.


> http://codereview.chromium.**org/7917015/diff/1/chrome/**
>
browser/resources/chromeos/**login/network_dropdown.js<http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chromeos/login/network_dropdown.js>
> File chrome/browser/resources/**chromeos/login/network_**dropdown.js
> (right):
>
> http://codereview.chromium.**org/7917015/diff/1/chrome/**
>
browser/resources/chromeos/**login/network_dropdown.js#**newcode41<http://codereview.chromium.org/7917015/diff/1/chrome/browser/resources/chromeos/login/network_dropdown.js#newcode41>
> chrome/browser/resources/**chromeos/login/network_**dropdown.js:41:
> 'aria-activedescendant', this.id + selectedItem.id);
> Shouldn't it be just 'selectedItem.id'?
>

The wrapper around the item is the thing that gets selected; selectedItem is
the inner element. I set the wrapper's id to the dropdown menu's id plus the
selected item's id and then set the activedescendant of the menu button to
the wrapper id.

Another concern is that the inner element ids did not seem to be
page-unique, so I didn't want to use those.

I'll change the other file and double-check that the layout hasn't changed
before submitting.

In the future, please try to use <label> where possible; every control
should have a label (or placeholder text, also okay). I'll help with adding
ARIA to any custom controls as needed. Thanks.

- Dominic

[altimofeev, 2011-09-19 20:51:30]

Thank you for the explanations! But I have some more questions.

On 2011/09/19 17:17:58, Dominic Mazzoni wrote:
> The wrapper around the item is the thing that gets selected; selectedItem is
> the inner element. I set the wrapper's id to the dropdown menu's id plus the
> selected item's id and then set the activedescendant of the menu button to
> the wrapper id.

I did't get your point. Wrapper is being selected, so 'selectedItem' := wrapper
div. It means, that 'this.id + selectedItem.id' is dropdown's menu's id plus the
wrapper div id.
Also, notice that we don't set id for the inner item - (innerItem.iid :=
item.id, but innerItem.id isn't set)

[dmazzoni, 2011-09-19 20:57:42]

On Mon, Sep 19, 2011 at 1:51 PM, <altimofeev@chromium.org> wrote:

> I did't get your point. Wrapper is being selected, so 'selectedItem' :=
> wrapper
> div. It means, that 'this.id + selectedItem.id' is dropdown's menu's id
> plus the
> wrapper div id.
>

Well, see on line 177 where I set the id of the wrapper div to that same id.


> Also, notice that we don't set id for the inner item - (innerItem.iid :=
> item.id, but innerItem.id isn't set)
>

Exactly. I could set the id on the inner item if you prefer.

However, I'm concerned about setting it to just iid, because that wouldn't
be page-unique if someone used two of these controls on the same page. So I
think that generating an id based on the button name and the iid is a good
idea.

Make sense? So let me know if you would prefer if I set the id on the inner
item, I think that will work.

- Dominic

[altimofeev, 2011-09-20 07:57:41]

First, let me explain my notations:
_DropDownContainer_ contains _Wrappers_.
_Wrappers_ for items contains image and _InnerItem_.
In context of line 41, selectedItem is _Wrapper_
In context of line 177, wrapperDiv is _Wrapper_

Please find other comments inline:

On 2011/09/19 20:57:42, Dominic Mazzoni wrote:
> On Mon, Sep 19, 2011 at 1:51 PM, <mailto:altimofeev@chromium.org> wrote:
> 
> > I did't get your point. Wrapper is being selected, so 'selectedItem' :=
> > wrapper
> > div. It means, that 'this.id + selectedItem.id' is dropdown's menu's id
> > plus the
> > wrapper div id.
> >
> 
> Well, see on line 177 where I set the id of the wrapper div to that same id.

I think no. You set different ids in lines 41 and 177. I mean:
line 41: DropDownContainer.id + Wrapper.id = 2 * DropDownContainer.id +
InnerItem.idd
line 177: DropdownContainer.id + InnerItem.idd

They are the same iff DropDownContainer.id == '', which is not true.
 
> > Also, notice that we don't set id for the inner item - (innerItem.iid :=
> > item.id, but innerItem.id isn't set)
> >
> 
> Exactly. I could set the id on the inner item if you prefer.
> 
> However, I'm concerned about setting it to just iid, because that wouldn't
> be page-unique if someone used two of these controls on the same page. So I
> think that generating an id based on the button name and the iid is a good
> idea.
> 
> Make sense? So let me know if you would prefer if I set the id on the inner
> item, I think that will work.

Probably, you have confused InnerItem and Wrapper. While my note was about
InnerItem, you have answered about Wrapper. I think that setting id of the
Wrapper the way you did it - looks good. The only thing I want to emphasize was
that InnerItem has NOT id, but idd (line 167).

[dmazzoni, 2011-09-20 23:17:45]

On 2011/09/20 07:57:41, altimofeev wrote:
> I think no. You set different ids in lines 41 and 177. I mean:
> line 41: DropDownContainer.id + Wrapper.id = 2 * DropDownContainer.id +
> InnerItem.idd
> line 177: DropdownContainer.id + InnerItem.idd
> 
> They are the same iff DropDownContainer.id == '', which is not true.

Ahhh, you're totally right. Actually, DropDownContainer.id is '', at least in my
build, which is why it worked for me. :) But yes, it would have broken if
DropDownContainer.id was equal to anything and it should be just
selectedItem.id. Fixed.

I updated screen_offline_message.html as well and verified that this did not
affect the layout, and uploaded a new change. Please take another look, thanks!

- Dominic

[dmazzoni, 2011-09-21 04:29:27]

Realized one more thing - when you select a network it sometimes pops up a
dialog asking for a password or more info. I haven't properly handled that
case with my keyboard handling - if you change the selection without opening
it, it won't pop up the dialog. I'll fix that tomorrow.

- Dominic

On Tue, Sep 20, 2011 at 4:17 PM, <dmazzoni@chromium.org> wrote:

> On 2011/09/20 07:57:41, altimofeev wrote:
>
>> I think no. You set different ids in lines 41 and 177. I mean:
>> line 41: DropDownContainer.id + Wrapper.id = 2 * DropDownContainer.id +
>> InnerItem.idd
>> line 177: DropdownContainer.id + InnerItem.idd
>>
>
>  They are the same iff DropDownContainer.id == '', which is not true.
>>
>
> Ahhh, you're totally right. Actually, DropDownContainer.id is '', at least
> in my
> build, which is why it worked for me. :) But yes, it would have broken if
> DropDownContainer.id was equal to anything and it should be just
> selectedItem.id. Fixed.
>
> I updated screen_offline_message.html as well and verified that this did
> not
> affect the layout, and uploaded a new change. Please take another look,
> thanks!
>
> - Dominic
>
>
>
>
http://codereview.chromium.**org/7917015/<http://codereview.chromium.org/7917...
>

[altimofeev, 2011-09-21 09:09:12]

On 2011/09/21 04:29:27, Dominic Mazzoni wrote:
> Realized one more thing - when you select a network it sometimes pops up a
> dialog asking for a password or more info. I haven't properly handled that
> case with my keyboard handling - if you change the selection without opening
> it, it won't pop up the dialog. I'll fix that tomorrow.

You are right. Actually, it is not clear for me whether this behavior is what
the user expects. I mean, drop-down button (title) and drop-down menu are two
different things. Drop-down button shows current network status. It shouldn't
show 'Disable wi-fi' or anything like this. It doesn't even show wifi, if
Ethernet is connected.

Sorry, that this took a few iterations for me to understand, but probably we do
not want to change the title, when the user navigates through the menu. It
implies, that menu navigation without opening the drop-down menu is impossible.

My proposal is to leave only speaking related part of the fix.

[dmazzoni, 2011-09-21 21:15:33]

OK, I deleted everything except the ARIA changes, and I changed the role from a
'listbox' to a 'button with a pop-up' to make it more clear that you have to
open it up to select something.

The only non-speaking change now is that both 'space' and 'enter' will pop it
open.

[altimofeev, 2011-09-22 05:58:30]

LGTM, thanks!

[commit-bot: I haz the power, 2011-09-22 07:42:47]

Can't apply patch for file
chrome/browser/resources/chromeos/login/network_dropdown.js.
While running patch -p0 --forward --force;
patching file chrome/browser/resources/chromeos/login/network_dropdown.js
Hunk #1 FAILED at 24.
Hunk #2 FAILED at 35.
Hunk #3 succeeded at 80 (offset 27 lines).
Hunk #4 FAILED at 79.
Hunk #5 succeeded at 181 (offset 27 lines).
Hunk #6 succeeded at 197 (offset 27 lines).
Hunk #7 succeeded at 249 (offset 27 lines).
Hunk #8 succeeded at 276 (offset 27 lines).
3 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/resources/chromeos/login/network_dropdown.js.rej

[commit-bot: I haz the power, 2011-09-22 09:59:02]

Change committed as 102258