Implement accessible states and notifications for the tab strip.

Implement accessible states and notifications for the tab strip.

This change just exposes information about what tabs are selected
and fires proper notifications when tab selection changes. This will
allow screen readers to announce when switching tabs even if the
web content doesn't have focus. It's also a good first step towards
making the tab strip fully accessible too (e.g. making it possible to
perform operations on multiple tabs with just the keyboard).

As part of this change I renamed the "selection changed" event to
"text selection changed" so that there's no confusion between the
text-selection-related events, and tab/list selection events.

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

[Ben Goodger (Google), 2014-07-07 19:46:30]

lgtm

[dtseng, 2014-07-08 17:29:08]

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:793: 
Are these sorted?

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
File ui/accessibility/ax_enums.idl (left):

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:34: 
See comment above:
  // For new entries to the following three enums, also add to                  
  // chrome/common/extensions/apis/automation.idl.

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:36: 
Seems like we're getting pretty loose with these? Can we consolidate?

[dmazzoni, 2014-07-08 20:45:47]

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:793: 
On 2014/07/08 17:29:08, dtseng wrote:
> Are these sorted?

They're vectors of ints. I think they're in order in which they were added to
the selection, so we can't assume anything about the order and a set difference
is the safest way to compute this.

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
File ui/accessibility/ax_enums.idl (left):

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:34: 
On 2014/07/08 17:29:08, dtseng wrote:
> See comment above:
>   // For new entries to the following three enums, also add to                
 
>   // chrome/common/extensions/apis/automation.idl.                            
 

Thanks for the reminder. Done.

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:36: 
On 2014/07/08 17:29:08, dtseng wrote:
> Seems like we're getting pretty loose with these? Can we consolidate?

None of the ones I'm changing here are redundant, they're all different events
on Windows.

We can consolidate one or two of these if we rename things in blink too. I filed
this bug: http://crbug.com/392270 - I've been waiting for Blink to get merged
into the Chrome codebase to make this an easier refactoring.

Would you like me to add comments for these?

[David Tseng, 2014-07-08 20:57:43]

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:793: 
On 2014/07/08 20:45:47, dmazzoni wrote:
> On 2014/07/08 17:29:08, dtseng wrote:
> > Are these sorted?
> 
> They're vectors of ints. I think they're in order in which they were added to
> the selection, so we can't assume anything about the order and a set
difference
> is the safest way to compute this.

Right. StlSetDifference's docs say:
// Returns a new ResultType containing the difference of two sorted containers.

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
File ui/accessibility/ax_enums.idl (left):

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:36: 
On 2014/07/08 20:45:47, dmazzoni wrote:
> On 2014/07/08 17:29:08, dtseng wrote:
> > Seems like we're getting pretty loose with these? Can we consolidate?
> 
> None of the ones I'm changing here are redundant, they're all different events
> on Windows.
> 
> We can consolidate one or two of these if we rename things in blink too. I
filed
> this bug: http://crbug.com/392270 - I've been waiting for Blink to get merged
> into the Chrome codebase to make this an easier refactoring.
> 
> Would you like me to add comments for these?

Yes; maybe even separate Windows-specific ones to their own section.

[David Tseng, 2014-07-08 21:01:27]

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
File ui/accessibility/ax_enums.idl (left):

https://codereview.chromium.org/349033010/diff/20001/ui/accessibility/ax_enum...
ui/accessibility/ax_enums.idl:36: 
On 2014/07/08 20:57:43, David Tseng wrote:
> On 2014/07/08 20:45:47, dmazzoni wrote:
> > On 2014/07/08 17:29:08, dtseng wrote:
> > > Seems like we're getting pretty loose with these? Can we consolidate?
> > 
> > None of the ones I'm changing here are redundant, they're all different
events
> > on Windows.
> > 
> > We can consolidate one or two of these if we rename things in blink too. I
> filed
> > this bug: http://crbug.com/392270 - I've been waiting for Blink to get
merged
> > into the Chrome codebase to make this an easier refactoring.
> > 
> > Would you like me to add comments for these?
> 
> Yes; maybe even separate Windows-specific ones to their own section.

Another idea: maybe even prefix the Windows-only ones with WIN or something?
Then, we could omit the comment and disambiguate between something like
selection_text_changed and text_selection_changed.

[dmazzoni, 2014-07-09 17:10:36]

I filed a couple of new bugs, deleted some events, and commented the rest. Have
a look.

[dmazzoni, 2014-07-09 17:19:43]

On Tue, Jul 8, 2014 at 2:01 PM, <dtseng@chromium.org> wrote:

> Another idea: maybe even prefix the Windows-only ones with WIN or
> something? Then, we could omit the comment and disambiguate between
> something like selection_text_changed and text_selection_changed.
>

We can consider this, but I don't think it would necessarily help in the
short term. I think once we get rid of the events that can be implicit -
meaning that Blink just updates the AXNode, and the platform-specific code
fires the right events to indicate what platform-specific states changed -
we'll be left with very few events that only apply on Windows currently,
and I think most of them are ones that may be useful in Chrome OS. So let's
consolidate and clean up first and then re-evaluate.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-07-09 18:07:35]

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/349033010/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:793: 
On 2014/07/08 20:57:43, David Tseng wrote:
> On 2014/07/08 20:45:47, dmazzoni wrote:
> > On 2014/07/08 17:29:08, dtseng wrote:
> > > Are these sorted?
> > 
> > They're vectors of ints. I think they're in order in which they were added
to
> > the selection, so we can't assume anything about the order and a set
> difference
> > is the safest way to compute this.
> 
> Right. StlSetDifference's docs say:
> // Returns a new ResultType containing the difference of two sorted
containers.
> 

FYI.  Took a look and |selected_indices| is sorted
  // Returns the selected indices. The selection is always ordered in acending  
...

[dmazzoni, 2014-07-09 18:13:01]

On Wed, Jul 9, 2014 at 11:07 AM, <dtseng@chromium.org> wrote:

> FYI.  Took a look and |selected_indices| is sorted
>   // Returns the selected indices. The selection is always ordered in
> acending
>

You're right, I saw that too and forgot to respond. Let me add a couple of
comments to clarify that.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-09 18:27:09]

Added some comments to tab_strip.cc, PTAL

On Wed, Jul 9, 2014 at 11:12 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> On Wed, Jul 9, 2014 at 11:07 AM, <dtseng@chromium.org> wrote:
>
>> FYI.  Took a look and |selected_indices| is sorted
>>   // Returns the selected indices. The selection is always ordered in
>> acending
>>
>
> You're right, I saw that too and forgot to respond. Let me add a couple of
> comments to clarify that.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-07-09 18:41:44]

https://codereview.chromium.org/349033010/diff/100001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/349033010/diff/100001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:31: checked_state_changed,     // Implicit
Can we group with major headings like...
// Web.
foo,
bar,

// Implicit.
baz
...

// ...
...

[David Tseng, 2014-07-09 18:41:55]

lgtm

[dmazzoni, 2014-07-09 19:14:48]

https://codereview.chromium.org/349033010/diff/100001/ui/accessibility/ax_enu...
File ui/accessibility/ax_enums.idl (right):

https://codereview.chromium.org/349033010/diff/100001/ui/accessibility/ax_enu...
ui/accessibility/ax_enums.idl:31: checked_state_changed,     // Implicit
On 2014/07/09 18:41:44, David Tseng wrote:
> Can we group with major headings like...

Would you prefer that over alphabetical? I'm worried that not sorting them could
make it harder to find the right notification at a glance.

No strong preference.

[chromium-reviews, 2014-07-09 20:19:50]

On Wed, Jul 9, 2014 at 12:14 PM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.org/349033010/diff/100001/ui/
> accessibility/ax_enums.idl
> File ui/accessibility/ax_enums.idl (right):
>
> https://codereview.chromium.org/349033010/diff/100001/ui/
> accessibility/ax_enums.idl#newcode31
> ui/accessibility/ax_enums.idl:31: checked_state_changed,     // Implicit
> On 2014/07/09 18:41:44, David Tseng wrote:
>
>> Can we group with major headings like...
>>
>
> Would you prefer that over alphabetical? I'm worried that not sorting
> them could make it harder to find the right notification at a glance.
>
> No strong preference.
>
> With just a few of these seems fine as is. Thanks.

https://codereview.chromium.org/349033010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-10 17:35:05]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:36:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/349033010/120001

[commit-bot: I haz the power, 2014-07-10 19:29:01]

Change committed as 282396

[David Tseng, 2014-07-11 19:28:09]

FYI; this causes a DCHECK in accessibility_event_router_views.cc:226
because the tabs role isn't handled.


On Tue, Jul 8, 2014 at 10:29 AM, <dtseng@google.com> wrote:

>
> https://codereview.chromium.org/349033010/diff/20001/
> chrome/browser/ui/views/tabs/tab_strip.cc
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
> https://codereview.chromium.org/349033010/diff/20001/
> chrome/browser/ui/views/tabs/tab_strip.cc#newcode793
> chrome/browser/ui/views/tabs/tab_strip.cc:793:
> Are these sorted?
>
> https://codereview.chromium.org/349033010/diff/20001/ui/
> accessibility/ax_enums.idl
> File ui/accessibility/ax_enums.idl (left):
>
> https://codereview.chromium.org/349033010/diff/20001/ui/
> accessibility/ax_enums.idl#oldcode34
> ui/accessibility/ax_enums.idl:34:
> See comment above:
>   // For new entries to the following three enums, also add to
>
>   // chrome/common/extensions/apis/automation.idl.
>
> https://codereview.chromium.org/349033010/diff/20001/ui/
> accessibility/ax_enums.idl#oldcode36
> ui/accessibility/ax_enums.idl:36:
> Seems like we're getting pretty loose with these? Can we consolidate?
>
> https://codereview.chromium.org/349033010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-07-11 19:45:17]

Thanks, I'll upload a fix for that.


On Fri, Jul 11, 2014 at 12:28 PM, David Tseng <dtseng@chromium.org> wrote:

> FYI; this causes a DCHECK in accessibility_event_router_views.cc:226
> because the tabs role isn't handled.
>
>
> On Tue, Jul 8, 2014 at 10:29 AM, <dtseng@google.com> wrote:
>
>>
>> https://codereview.chromium.org/349033010/diff/20001/
>> chrome/browser/ui/views/tabs/tab_strip.cc
>> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>>
>> https://codereview.chromium.org/349033010/diff/20001/
>> chrome/browser/ui/views/tabs/tab_strip.cc#newcode793
>> chrome/browser/ui/views/tabs/tab_strip.cc:793:
>> Are these sorted?
>>
>> https://codereview.chromium.org/349033010/diff/20001/ui/
>> accessibility/ax_enums.idl
>> File ui/accessibility/ax_enums.idl (left):
>>
>> https://codereview.chromium.org/349033010/diff/20001/ui/
>> accessibility/ax_enums.idl#oldcode34
>> ui/accessibility/ax_enums.idl:34:
>> See comment above:
>>   // For new entries to the following three enums, also add to
>>
>>   // chrome/common/extensions/apis/automation.idl.
>>
>> https://codereview.chromium.org/349033010/diff/20001/ui/
>> accessibility/ax_enums.idl#oldcode36
>> ui/accessibility/ax_enums.idl:36:
>> Seems like we're getting pretty loose with these? Can we consolidate?
>>
>> https://codereview.chromium.org/349033010/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.