Make user remove button focusable, and set up the correct tab order.

Make user remove button focusable, and set up the correct tab order.


BUG=chromium-os:20044
TEST=Add two or more users.  Check that you can tab to all users and remove buttons, and that you can remove users.


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

[dmazzoni, 2011-10-26 16:14:49]

http://codereview.chromium.org/8396032/diff/2001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

http://codereview.chromium.org/8396032/diff/2001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/user_pod_row.js:477:
this.pods[i].tabIndex = i + 1;
I'm pretty sure everything in the page with the same tabIndex will be focused in
DOM order. I think it might be more robust to just have some fixed tab indexes
for each *group* of related controls - that way it doesn't matter how many of
each there are and there's less to keep track of.

How about defining some constants at the top, with a comment explaining this,
e.g.:

var TOP_CONTROLS_TAB_INDEX = 1;
var PASSWORD_FIELDS_TAB_INDEX = 2;
var REMOVE_BUTTONS_TAB_INDEX = 3;

Optional (not part of this CL): how hard would it be to add a unit test that
ensures that nobody adds a control to this page in the future without setting
the tab index? That'd be a fantastic way to help prevent future regressions.

[Zachary Kuznia, 2011-10-27 08:19:39]

Done.  Nikita, do you know how I could set up a unit test like that?

On 2011/10/26 16:14:49, Dominic Mazzoni wrote:
>
http://codereview.chromium.org/8396032/diff/2001/chrome/browser/resources/chr...
> File chrome/browser/resources/chromeos/login/user_pod_row.js (right):
> 
>
http://codereview.chromium.org/8396032/diff/2001/chrome/browser/resources/chr...
> chrome/browser/resources/chromeos/login/user_pod_row.js:477:
> this.pods[i].tabIndex = i + 1;
> I'm pretty sure everything in the page with the same tabIndex will be focused
in
> DOM order. I think it might be more robust to just have some fixed tab indexes
> for each *group* of related controls - that way it doesn't matter how many of
> each there are and there's less to keep track of.
> 
> How about defining some constants at the top, with a comment explaining this,
> e.g.:
> 
> var TOP_CONTROLS_TAB_INDEX = 1;
> var PASSWORD_FIELDS_TAB_INDEX = 2;
> var REMOVE_BUTTONS_TAB_INDEX = 3;
> 
> Optional (not part of this CL): how hard would it be to add a unit test that
> ensures that nobody adds a control to this page in the future without setting
> the tab index? That'd be a fantastic way to help prevent future regressions.

[Nikita (slow), 2011-10-27 10:04:21]

On 2011/10/27 08:19:39, Zachary Kuznia wrote:
> Done.  Nikita, do you know how I could set up a unit test like that?

I think it should be a WebUI browser_test. Unfortunately we don't have such
tests to cover login yet. We're planning to add those, probably even in Q4.

[Nikita (slow), 2011-10-27 10:04:35]

On 2011/10/27 10:04:21, Nikita Kostylev wrote:
> On 2011/10/27 08:19:39, Zachary Kuznia wrote:
> > Done.  Nikita, do you know how I could set up a unit test like that?
> 
> I think it should be a WebUI browser_test. Unfortunately we don't have such
> tests to cover login yet. We're planning to add those, probably even in Q4.

Forgot to include URL
http://dev.chromium.org/Home/domui-testing/webui-browser_tests

[dmazzoni, 2011-10-27 13:13:41]

lgtm

[commit-bot: I haz the power, 2011-10-28 03:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/8396032/5001

[commit-bot: I haz the power, 2011-10-28 04:54:26]

Change committed as 107683