Improve accessibility of user image selection screen.

Improve accessibility of user image selection screen.

Gives each image a spoken description. Fixes the ARIA markup so it's
exposed as an interactive listbox when accessibility is turned on.
Also, remaps up/down to be the same as left/right when accessibility
is on, since the 2-dimensional structure is not exposed.

Here's exactly the feedback it gives now, when you tab to the list box
and one item is selected:

"Choose a picture to display for your account on the sign-in screen.
 Cupcake, List box 7 of 20".

BUG=126186, chromium-os:26999
TEST=manual testing with spoken feedback on

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150170

[dmazzoni, 2012-05-04 16:36:38]

+nkostylev

[Ivan Korotkov, 2012-05-04 17:02:27]

http://codereview.chromium.org/10376003/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/default_user_images.h (right):

http://codereview.chromium.org/10376003/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/default_user_images.h:37: const std::string
GetDefaultImageDescription(int index);
No need in const

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list.js:835: item.id =
this.uniqueIdPrefix_ + '-' + this.nextUniqueIdSuffix_;
Please make assigning unique IDs to each element optional, so that this doesn't
affect other lists.

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list_item.js:39:
this.setAttribute('role', 'listitem');
This can break multiple styles on different pages. Please change
UserImagesGridItem.decorate instead.
Also, you'll have to update chromeos/login/oobe.css and
options2/chromeos/image_picker.css because they rely on [role=listitem]

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/list_selection_controller.js
(right):

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list_selection_controller.js:226: //
Special case: if ChromeVox is running (which provides spoken
Can you instead change GridSelectionController's getIndexAbove/getIndexBelow?
Or, if this is user images grid-only change, derive it's own selection
controller from GridSelectionController in user_images_grid.js

[dmazzoni, 2012-05-04 17:48:00]

http://codereview.chromium.org/10376003/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/default_user_images.h (right):

http://codereview.chromium.org/10376003/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/default_user_images.h:37: const std::string
GetDefaultImageDescription(int index);
On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> No need in const

Done.

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list.js:835: item.id =
this.uniqueIdPrefix_ + '-' + this.nextUniqueIdSuffix_;
On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> Please make assigning unique IDs to each element optional, so that this
doesn't
> affect other lists.

I don't want to make it optional, then we'll have to explicitly enable it to
make any other list accessible. This change should improve accessibility for
other lists.

The unique id prefix is derived from the id of the nearest ancestor, so it's
extremely unlikely two lists will end up with the same id. If they did, it's
unlikely it'd break anything other than accessibility.

The only other option I see is to make nextUniqueIdSuffix a class variable (i.e.
a global) so that ids are never shared between list items.

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list_item.js:39:
this.setAttribute('role', 'listitem');
On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> This can break multiple styles on different pages. Please change
> UserImagesGridItem.decorate instead.

Good catch.

Actually I do want to fix this because 'option' is always correct, 'listitem'
was wrong. 'listitem' is the accessible equivalent of a <li> element, 'option'
is the equivalent of an option in a list box, which is what this is.

> Also, you'll have to update chromeos/login/oobe.css and
> options2/chromeos/image_picker.css because they rely on [role=listitem]

Done.

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/list_selection_controller.js
(right):

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list_selection_controller.js:226: //
Special case: if ChromeVox is running (which provides spoken
On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> Can you instead change GridSelectionController's getIndexAbove/getIndexBelow?
> Or, if this is user images grid-only change, derive it's own selection
> controller from GridSelectionController in user_images_grid.js

Sure, moved to grid.js. This should apply to any grid.

[Ivan Korotkov, 2012-05-04 22:03:33]

LGTM

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list.js:835: item.id =
this.uniqueIdPrefix_ + '-' + this.nextUniqueIdSuffix_;
On 2012/05/04 17:48:00, Dominic Mazzoni wrote:
> On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> > Please make assigning unique IDs to each element optional, so that this
> doesn't
> > affect other lists.
> 
> I don't want to make it optional, then we'll have to explicitly enable it to
> make any other list accessible. This change should improve accessibility for
> other lists.
> 
> The unique id prefix is derived from the id of the nearest ancestor, so it's
> extremely unlikely two lists will end up with the same id. If they did, it's
> unlikely it'd break anything other than accessibility.
> 
> The only other option I see is to make nextUniqueIdSuffix a class variable
(i.e.
> a global) so that ids are never shared between list items.

Ok, I guess these IDs won't hurt.

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

http://codereview.chromium.org/10376003/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/list_item.js:39:
this.setAttribute('role', 'listitem');
On 2012/05/04 17:48:00, Dominic Mazzoni wrote:
> On 2012/05/04 17:02:27, Ivan Korotkov wrote:
> > This can break multiple styles on different pages. Please change
> > UserImagesGridItem.decorate instead.
> 
> Good catch.
> 
> Actually I do want to fix this because 'option' is always correct, 'listitem'
> was wrong. 'listitem' is the accessible equivalent of a <li> element, 'option'
> is the equivalent of an option in a list box, which is what this is.
> 
> > Also, you'll have to update chromeos/login/oobe.css and
> > options2/chromeos/image_picker.css because they rely on [role=listitem]
> 
> Done.

Ok then. I hope you've caught all the listitem CSS references :)

[dmazzoni, 2012-05-04 22:57:24]

Thanks!

nkostylev: can you approve for the chromeos directories?
estade: can you approve for chrome/browser/resources?

[Nikita (slow), 2012-05-05 10:53:29]

lgtm

[Evan Stade, 2012-05-07 20:58:02]

lgtm with notes

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe.css:297: #user-image-grid
[role=option] {
use (single) quotes

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/image_picker.css (right):

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/image_picker.css:26: .image-picker
[role=option] {
add (single) quotes

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/grid.js:361: return (window.cvox &&
cvox.Api && cvox.Api &&
cvox.Api checked twice

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/grid.js:375: if (index == last) {
no curlies

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/grid.js:391: if (index == 0) {
no curlies

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:94: * The prefix to use when
giving each item an unique id.
"a unique"

http://codereview.chromium.org/10376003/diff/2002/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/list.js:836:
this.nextUniqueIdSuffix_++;
increment on same line as read?

[dmazzoni, 2012-05-07 21:00:51]

FYI, I did some more testing and found this makes other listboxes worse with
VoiceOver. I'll upload a fix and address these other comments shortly.

[dmazzoni, 2012-08-02 22:59:58]

Could you take a fresh look? The basic idea is the same, I just rebased and
lessened the scope of this change to only affect the user image screen.

The only substantial change to the general cr.ui.List/Grid code is for GridItem
to be an LI element rather than a SPAN, but I tested all of the uses of
cr.ui.Grid and this doesn't break anything.

Thanks.

[Ivan Korotkov, 2012-08-03 01:00:23]

Wow, I thought this one was abandoned long ago :)
LGTM with nits, please make sure you don't break any layout in OOBE/options.

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:3996: +      <message
name="IDS_LOGIN_DEFAULT_USER" desc="Description of the default user icon with a
picture of a beaker">
Could you add a suffix to these strings, something like
IDS_LOGIN_DEFAULT_USER_TITLE_3? I'm adding a bunch of attribution info for them
with names IDS_LOGIN_DEFAULT_USER_AUTHOR_3 and IDS_LOGIN_DEFAULT_USER_WEBSITE_3.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/default_user_images.cc (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/default_user_images.cc:107: // IDs of default user
image descriptions.
// String IDs...

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/default_user_images.cc:130: std::string
GetDefaultImageDescription(int index) {
Should be string16 and GetStringUTF16.
I actually suggest to remove the function and call GetStringUTF16 directly from
the caller site but that's up to you. If not, move kDefaultImageDescriptions to
a anonymous namespace.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:482: #user-image-grid li {
I'm worried about future changes and nested stuff. What about '#user-image-grid
> [role=option]'?

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:399:
console.error('\n\n' + document.body.innerHTML + '\n');
Cleanup?

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:542:
setUserImages_: function(imageInfos) {
There are two distinct implementations in this file, looks like you only changed
one of them.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:544: for (var
i = 0, imageInfo; imageInfo = imageInfos[i]; i++)
Indentation & add parens, please.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/change_picture_options.css
(left):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/change_picture_options.css:33:
#user-image-grid [role=listitem] {
Ditto

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/grid.js:361: return (window.cvox &&
cvox.Api &&
No need in braces.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/grid.js:362: cvox.Api.isChromeVoxActive
&& cvox.Api.isChromeVoxActive());
No need in parens.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/list.js:105: nextUniqueIdSuffix_: 0,
Don't put them into prototype: that makes them shared across all List objects.
Instead, you can assign them in constructor or on the first usage at all.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/list.js:610:
this.setAttribute('aria-activedescendant', listItem.id);
Indentation.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:85:
DictionaryValue* dict = new DictionaryValue();
Please use base::, scoped_ptr and release:
scoped_ptr<base::DictionaryValue> image_info(new base::DictionaryValue);
....
image_urls.Append(image_info.release());

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:87:
dict->Set("title", new StringValue(GetDefaultImageDescription(i)));
Use dict->SetString

[Nikita (slow), 2012-08-03 03:51:49]

lgtm

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:3996: +      <message
name="IDS_LOGIN_DEFAULT_USER" desc="Description of the default user icon with a
picture of a beaker">
In a context of adding new avatars these strings are not needed / should be
updated.

http://code.google.com/p/chromium/issues/detail?id=139401#c6

These avatars will be unavailable for selection.

[dmazzoni, 2012-08-03 22:59:01]

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:3996: +      <message
name="IDS_LOGIN_DEFAULT_USER" desc="Description of the default user icon with a
picture of a beaker">
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Could you add a suffix to these strings, something like
> IDS_LOGIN_DEFAULT_USER_TITLE_3? I'm adding a bunch of attribution info for
them
> with names IDS_LOGIN_DEFAULT_USER_AUTHOR_3 and
IDS_LOGIN_DEFAULT_USER_WEBSITE_3.

Done.

http://codereview.chromium.org/10376003/diff/25001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:3996: +      <message
name="IDS_LOGIN_DEFAULT_USER" desc="Description of the default user icon with a
picture of a beaker">
On 2012/08/03 03:51:49, Nikita Kostylev wrote:
> In a context of adding new avatars these strings are not needed / should be
> updated.

Got it, I'm working with Min Li now to use the updated strings.

Since they're not checked in yet, I can't check that the order is correct - but
I'll check in the names now so they can get translated, and we can update the
order of the strings after the images are added.

> http://code.google.com/p/chromium/issues/detail?id=139401#c6
> 
> These avatars will be unavailable for selection.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/default_user_images.cc (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/default_user_images.cc:107: // IDs of default user
image descriptions.
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> // String IDs...

Done.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/default_user_images.cc:130: std::string
GetDefaultImageDescription(int index) {
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Should be string16 and GetStringUTF16.
> I actually suggest to remove the function and call GetStringUTF16 directly
from
> the caller site but that's up to you. If not, move kDefaultImageDescriptions
to
> a anonymous namespace.

Hmmm, I think it's better to have the constants in this file so that somebody
modifying kDefaultImageResources would also modify 
kDefaultImageDescriptions. Moved to anonymous namespace.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:482: #user-image-grid li {
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> I'm worried about future changes and nested stuff. What about
'#user-image-grid
> > [role=option]'?

The purpose of "role" is for accessibility. Tying the CSS to the accessible role
makes it difficult to fix accessibility problems.

In particular, it turns out that role=listbox/role=option is good for simple
lists/grids, but role=list/role=listitem is better for lists of items that
contain form controls (like the file manager or search engine editor) - but they
all inherit from the same cr.ui.List/cr.ui.Grid.

So, changed to #user-image-grid > li as discussed.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:399:
console.error('\n\n' + document.body.innerHTML + '\n');
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Cleanup?

Done.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:542:
setUserImages_: function(imageInfos) {
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> There are two distinct implementations in this file, looks like you only
changed
> one of them.

Ah - I thought the top one was going to go away. Thanks.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:544: for (var
i = 0, imageInfo; imageInfo = imageInfos[i]; i++)
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Indentation & add parens, please.

Done.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/change_picture_options.css
(left):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/change_picture_options.css:33:
#user-image-grid [role=listitem] {
Changed to > li as discussed.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/grid.js:362: cvox.Api.isChromeVoxActive
&& cvox.Api.isChromeVoxActive());
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> No need in parens.

Removed parens. Braces are still needed, or am I misunderstanding?

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/list.js:105: nextUniqueIdSuffix_: 0,
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Don't put them into prototype: that makes them shared across all List objects.
> Instead, you can assign them in constructor or on the first usage at all.

Moved to decorate(), or did you have something else in mind?

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/list.js:610:
this.setAttribute('aria-activedescendant', listItem.id);
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Indentation.

Done.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:85:
DictionaryValue* dict = new DictionaryValue();
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Please use base::, scoped_ptr and release:
> scoped_ptr<base::DictionaryValue> image_info(new base::DictionaryValue);
> ....
> image_urls.Append(image_info.release());

Nice, thanks.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:87:
dict->Set("title", new StringValue(GetDefaultImageDescription(i)));
On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> Use dict->SetString

Done.

[Ivan Korotkov, 2012-08-03 23:19:37]

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:542:
setUserImages_: function(imageInfos) {
On 2012/08/03 22:59:01, Dominic Mazzoni wrote:
> On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> > There are two distinct implementations in this file, looks like you only
> changed
> > one of them.
> 
> Ah - I thought the top one was going to go away. Thanks.

It is, but we're keeping both of them operational for now.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/grid.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/grid.js:362: cvox.Api.isChromeVoxActive
&& cvox.Api.isChromeVoxActive());
On 2012/08/03 22:59:01, Dominic Mazzoni wrote:
> On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> > No need in parens.
> 
> Removed parens. Braces are still needed, or am I misunderstanding?

Uh, right, that was a stray comment.

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/cr/ui/list.js (right):

http://codereview.chromium.org/10376003/diff/25001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/cr/ui/list.js:105: nextUniqueIdSuffix_: 0,
On 2012/08/03 22:59:01, Dominic Mazzoni wrote:
> On 2012/08/03 01:00:23, Ivan Korotkov wrote:
> > Don't put them into prototype: that makes them shared across all List
objects.
> > Instead, you can assign them in constructor or on the first usage at all.
> 
> Moved to decorate(), or did you have something else in mind?

decorate is OK

http://codereview.chromium.org/10376003/diff/36001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10376003/diff/36001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:482: #user-image-grid li {
You forgot >

[dmazzoni, 2012-08-03 23:32:49]

http://codereview.chromium.org/10376003/diff/36001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10376003/diff/36001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:482: #user-image-grid li {
On 2012/08/03 23:19:37, Ivan Korotkov wrote:
> You forgot >

Oops, fixed. Thanks.

[Evan Stade, 2012-08-07 02:06:25]

is there anything in particular I should look at? I am not an author of any of
this code.

[dmazzoni, 2012-08-07 02:58:26]

Evan - sorry, I think you were an author of one of the files I touched in
an earlier revision. I think the changes are all in chromeos now. Thanks.

On Mon, Aug 6, 2012 at 7:06 PM, <estade@chromium.org> wrote:

> is there anything in particular I should look at? I am not an author of
> any of
> this code.
>
>
http://codereview.chromium.**org/10376003/<http://codereview.chromium.org/103...
>

[Evan Stade, 2012-08-07 03:01:36]

np. removing myself.