Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=fukino

Committed: https://crrev.com/4feee4215681b9a878a85e66c1019b09803a47c7
Cr-Commit-Position: refs/heads/master@{#420122}

[dmazzoni, 2016-09-20 00:11:43]

Description was changed from

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
==========

to

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-09-20 00:13:34]

dmazzoni@chromium.org changed reviewers:
+ dbeam@chromium.org

[dmazzoni, 2016-09-20 00:13:35]

Here's one possible fix.

An alternative fix would be to set an unique ID before we update
the aria-activedescendant attribute if the list item doesn't already
have a unique ID.

Any preference?

[Dan Beam, 2016-09-20 00:48:22]

lgtm

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/list_item.js (right):

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/list_item.js:16: * @type {number}
nit: @private {number}

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/list_item.js:45: this.id = 'listitem-' +
ListItem.nextUniqueIdSuffix_++;
did you check if anything is selecting by [id^="list"] or something like that? 
probably not, but it would break

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/list_item.js:45: this.id = 'listitem-' +
ListItem.nextUniqueIdSuffix_++;
you probbbbably want to set this only if it's not already set?

[nektarios, 2016-09-20 15:43:28]

+ this.id = 'listitem-' + ListItem.nextUniqueIdSuffix_++;

So if there are multiple lists on the page, this won't work?

[Dan Beam, 2016-09-20 18:04:32]

On 2016/09/20 15:43:28, nektarios wrote:
> + this.id = 'listitem-' + ListItem.nextUniqueIdSuffix_++;
> 
> So if there are multiple lists on the page, this won't work?

i'm pretty sure it's a globally unique ID as currently implemented

[dmazzoni, 2016-09-20 19:40:28]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-09-20 19:40:29]

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
File ui/webui/resources/js/cr/ui/list_item.js (right):

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/list_item.js:16: * @type {number}
On 2016/09/20 at 00:48:22, Dan Beam wrote:
> nit: @private {number}

Done

https://codereview.chromium.org/2346393004/diff/1/ui/webui/resources/js/cr/ui...
ui/webui/resources/js/cr/ui/list_item.js:45: this.id = 'listitem-' +
ListItem.nextUniqueIdSuffix_++;
On 2016/09/20 at 00:48:22, Dan Beam wrote:
> you probably want to set this only if it's not already set?

Done

> did you check if anything is selecting by [id^="list"] or something like that?
 probably not, but it would break

Good thought. I id a few greps and couldn't find anything that looks
for a list ID in any js or css code with /resources/. Hopefully we're
okay.

[dmazzoni, 2016-09-20 19:40:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2346393004/#ps20001
(title: "Address feedback")

[commit-bot: I haz the power, 2016-09-20 19:40:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 20:20:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 20:20:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-09-20 21:21:35]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-09-20 21:22:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 22:33:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 22:33:35]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2016-09-20 23:24:23]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 23:25:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 00:36:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 00:36:56]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-09-21 18:25:54]

dmazzoni@chromium.org changed reviewers:
+ fukino@chromium.org

[dmazzoni, 2016-09-21 18:25:54]

+fukino for small fix to a ui/file_manager test

Dan, I made one other change to have it remove aria-activedescendant
when the lead item is removed, this is to prevent it from triggering
accessibility audit failures at the end of tests when
aria-activedescendant points to an invalid id. Let me know if that
seems reasonable.

[Dan Beam, 2016-09-21 18:33:18]

On 2016/09/21 18:25:54, dmazzoni wrote:
> +fukino for small fix to a ui/file_manager test
> 
> Dan, I made one other change to have it remove aria-activedescendant
> when the lead item is removed, this is to prevent it from triggering
> accessibility audit failures at the end of tests when
> aria-activedescendant points to an invalid id. Let me know if that
> seems reasonable.

lgtm

[dmazzoni, 2016-09-21 18:44:50]

Description was changed from

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=fukino
==========

[dmazzoni, 2016-09-21 18:44:53]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:45:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 18:50:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-21 18:54:28]

Description was changed from

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=fukino
==========

to

==========
Give descendants of cr.ui.ListItem a unique ID to fix accessibility.

The unique ID is necessary so that aria-activedescendant can be
set on the parent list.

The unique ID was being set in List.createItem(), which was
being overriden by several subclasses, including BookmarkList.
Instead, set the unique ID in ListItem.decorate(), since
subclasses of ListItem seem to all call the inherited method.

With this change, there isn't an easy way to compute the ID
prefix once per list and share it between all of the list items.
It seems expensive to walk the parents for each new list item
created, so I opted to get rid of the custom ID prefix and
just make each one unique, since that's what really matters.

Manually tested with VoiceOver - arrowing through the bookmarks
gives proper spoken feedback.

BUG=634063
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=fukino

Committed: https://crrev.com/4feee4215681b9a878a85e66c1019b09803a47c7
Cr-Commit-Position: refs/heads/master@{#420122}
==========

[commit-bot: I haz the power, 2016-09-21 18:54:29]

Patchset 3 (id:??) landed as
https://crrev.com/4feee4215681b9a878a85e66c1019b09803a47c7
Cr-Commit-Position: refs/heads/master@{#420122}

[fukino, 2016-09-22 10:07:51]

lgtm. Thanks!