Added basic accessibility to TreeView.

Added basic accessibility to TreeView.

BUG=322729
TEST=manual with ChromeVox

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

[dmazzoni, 2013-11-22 23:45:29]

lgtm

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/accessibil...
File chrome/browser/accessibility/accessibility_events.h (right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_events.h:369: // Count of items at
the current depth.
Should this be "Count of children..."?

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:312:
int dept = -1;
dept -> depth

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.h
(right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.h:103:
void SendTreeNotification(
static

https://codereview.chromium.org/79543010/diff/80001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/79543010/diff/80001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view.cc:236:
NotifyAccessibilityEvent(ui::AccessibilityTypes::EVENT_FOCUS, true);
I'm tempted to use EVENT_SELECTION_CHANGED here (seems more accurate) but go
ahead and plumb it through to a focus event inside the event router. OK to leave
alone if that doesn't work or complicates things a lot for some reason.

[zel, 2013-11-23 00:17:16]

+sky for ui/views/* OWNER review

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/accessibil...
File chrome/browser/accessibility/accessibility_events.h (right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/accessibil...
chrome/browser/accessibility/accessibility_events.h:369: // Count of items at
the current depth.
On 2013/11/22 23:45:30, Dominic Mazzoni wrote:
> Should this be "Count of children..."?

Done.

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:312:
int dept = -1;
On 2013/11/22 23:45:30, Dominic Mazzoni wrote:
> dept -> depth

Done.

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.h
(right):

https://codereview.chromium.org/79543010/diff/80001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.h:103:
void SendTreeNotification(
On 2013/11/22 23:45:30, Dominic Mazzoni wrote:
> static

Done.

https://codereview.chromium.org/79543010/diff/80001/ui/views/controls/tree/tr...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/79543010/diff/80001/ui/views/controls/tree/tr...
ui/views/controls/tree/tree_view.cc:236:
NotifyAccessibilityEvent(ui::AccessibilityTypes::EVENT_FOCUS, true);
On 2013/11/22 23:45:30, Dominic Mazzoni wrote:
> I'm tempted to use EVENT_SELECTION_CHANGED here (seems more accurate) but go
> ahead and plumb it through to a focus event inside the event router. OK to
leave
> alone if that doesn't work or complicates things a lot for some reason.

I will add TODO here to address this later.

I picked EVENT_FOCUS since that how it seemed other controls, like menu, were
handling item selection.

[David Tseng, 2013-11-23 02:10:18]

Looks like you still need to update
chrome/common/extensions/api/experimental_accessibility.json

with the Tree/TreeitemDetails json structures.

[zel, 2013-11-23 02:18:52]

On 2013/11/23 02:10:18, David Tseng wrote:
> Looks like you still need to update
> chrome/common/extensions/api/experimental_accessibility.json
> 
> with the Tree/TreeitemDetails json structures.

added

[David Tseng, 2013-11-23 02:45:14]

LGTM

https://codereview.chromium.org/79543010/diff/200001/chrome/browser/accessibi...
File chrome/browser/accessibility/accessibility_events.cc (right):

https://codereview.chromium.org/79543010/diff/200001/chrome/browser/accessibi...
chrome/browser/accessibility/accessibility_events.cc:269: const std::string&
menu_name)
nit: tree_name

[sky, 2013-11-24 18:58:27]

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:238:
NotifyAccessibilityEvent(ui::AccessibilityTypes::EVENT_FOCUS, true);
Can you move this to the end of the function. I would rather ScrollRectToVisible
first.

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:407: if (!selected_node_ ||
!selected_node_->model_node())
The second null check isn't necessary.

[sky, 2013-11-24 18:58:35]

LGTM with these changes

[zel, 2013-12-02 05:36:09]

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
File ui/views/controls/tree/tree_view.cc (right):

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:238:
NotifyAccessibilityEvent(ui::AccessibilityTypes::EVENT_FOCUS, true);
On 2013/11/24 18:58:28, sky wrote:
> Can you move this to the end of the function. I would rather
ScrollRectToVisible
> first.

Done.

https://codereview.chromium.org/79543010/diff/200001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:407: if (!selected_node_ ||
!selected_node_->model_node())
On 2013/11/24 18:58:28, sky wrote:
> The second null check isn't necessary.

Done.

[commit-bot: I haz the power, 2013-12-02 05:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/79543010/270001

[commit-bot: I haz the power, 2013-12-02 07:38:24]

Change committed as 238047