Modified tab change to work with status tray change

Modified tab change to work with status tray change

The status tray change made it so that a view must be accessiblity focusable to be read. Tabs are now accessibility focusable so now they will be read with the status tray change.

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

[evy, 2014-07-25 01:50:20]

A quick fix to a tab problem I was having today.

[sky, 2014-07-25 03:19:30]

https://codereview.chromium.org/414203002/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/414203002/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab.cc:466: SetAccessibilityFocusable(true);
Tabs aren't set up to be focusable. I'm not opposed to doing this, but it isn't
as simple as just flipping this bit.

[dmazzoni, 2014-07-25 05:54:24]

On Thu, Jul 24, 2014 at 8:19 PM, <sky@chromium.org> wrote:

> Tabs aren't set up to be focusable. I'm not opposed to doing this, but
> it isn't as simple as just flipping this bit.
>

Eventually I would like to make tabs keyboard-focusable like they are in
Firefox - see http://crbug.com/100412 - but that's another discussion.

I propose a slightly different fix:
AccessibilityEventRouterViews::FindFirstAccessibleAncestor returns the
highest ancestor if nothing is accessibility focusable. I propose that if a
view isn't accessibility focusable and none of its ancestors are either,
you just return the original view.

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

[sky, 2014-07-25 16:05:34]

On Thu, Jul 24, 2014 at 10:54 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:
> On Thu, Jul 24, 2014 at 8:19 PM, <sky@chromium.org> wrote:
>>
>> Tabs aren't set up to be focusable. I'm not opposed to doing this, but
>> it isn't as simple as just flipping this bit.
>
>
> Eventually I would like to make tabs keyboard-focusable like they are in
> Firefox - see http://crbug.com/100412 - but that's another discussion.

This is why I said it's more than just flipping the accessibility
enabled bit. I would love to see keyboard navigation for the tabstrip
too.

> I propose a slightly different fix:
> AccessibilityEventRouterViews::FindFirstAccessibleAncestor returns the
> highest ancestor if nothing is accessibility focusable. I propose that if a
> view isn't accessibility focusable and none of its ancestors are either, you
> just return the original view.

evy and myself spoke and I think she was inclined to do this too.

  -Scott

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

[evy, 2014-07-25 16:18:08]

The fully accessible tab tray does sound awesome, but this fix is probably best
for now.

[sky, 2014-07-25 20:14:27]

How about test coverage?

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:597: }
nit: no {} here.

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:600:
else
nit: no else here, see style guide.

[evy, 2014-07-25 20:55:31]

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:597: }
On 2014/07/25 20:14:26, sky wrote:
> nit: no {} here.

Done.

https://codereview.chromium.org/414203002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:600:
else
On 2014/07/25 20:14:26, sky wrote:
> nit: no else here, see style guide.

Done.

[sky, 2014-07-25 21:12:31]

LGTM

https://codereview.chromium.org/414203002/diff/40001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/accessibility/accessibility_event_router_views_unittest.cc
(right):

https://codereview.chromium.org/414203002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views_unittest.cc:442:
accessible_view =
nit: inline assignment so I don't have to worry about whether accessible_view is
set before you use it.

[evy, 2014-07-26 00:11:19]

The CQ bit was checked by evy@chromium.org

[commit-bot: I haz the power, 2014-07-26 00:12:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/evy@chromium.org/414203002/60001

[commit-bot: I haz the power, 2014-07-26 19:53:45]

Change committed as 285795