Fixed accessibility issues in bookmark bubble dialog.

Fixed several accessibility issues in bookmark bubble dialog:

1. BookmarkBubbleView: added BookmarkBubbleView::GetAccessibleState() so we can say what this UI is all about
2. BookmarkBubbleView: added spoken feedback to [Name] text box
3. BookmarkBubbleView: changed spoken feedback for for [Folder] to "Bookmark folder"
4. fixed AccessibilityEventRouterViews::GetViewContext() to prevent drilling down into child elements of a dialog if the dialog defines AccessibleViewState::name already.
5. BookmarkEditorView: added BookmarkBubbleView::GetAccessibleState() so we can say what this UI is all about
6. BookmarkEditorView: changed spoken feedback for for [Name] and [URL] text edit fields


BUG=177321, 127042
TEST=manual with spoken feedback turned on

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

[David Tseng, 2013-11-20 22:13:27]

https://codereview.chromium.org/79273002/diff/1/chrome/app/bookmarks_strings....
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/79273002/diff/1/chrome/app/bookmarks_strings....
chrome/app/bookmarks_strings.grdp:186: <message
name="IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK_ACCESSIBILITY" desc="Title of the bubble
when re-clicking on a bookmark, read by text-to-speach accessability feature">
s/accessability/accessibility (and you probably could just call it spoken
feedback).

https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/acces...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:398:
// A control in alert, toolbar or dialog should use the its accessible name
s/the its/its.

Perhaps rephrase
    // A control in alert, toolbar or dialog should use its accessible name

as

    // A control inside of alert, toolbar or dialog should use that container's
accessible name

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

https://codereview.chromium.org/79273002/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:406:
state.role == ui::AccessibilityTypes::ROLE_TOOLBAR) &&
Did you test this with infobars? It looks like the below block makes a best
guess at the context based on the first static text child view for alerts and
dialogs.

[zel, 2013-11-21 00:19:22]

PTAL

I've added items 5-7 with the latest update.

https://codereview.chromium.org/79273002/diff/1/chrome/app/bookmarks_strings....
File chrome/app/bookmarks_strings.grdp (right):

https://codereview.chromium.org/79273002/diff/1/chrome/app/bookmarks_strings....
chrome/app/bookmarks_strings.grdp:186: <message
name="IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK_ACCESSIBILITY" desc="Title of the bubble
when re-clicking on a bookmark, read by text-to-speach accessability feature">
On 2013/11/20 22:13:27, David Tseng wrote:
> s/accessability/accessibility (and you probably could just call it spoken
> feedback).

Done.

https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/acces...
File chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
(right):

https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:398:
// A control in alert, toolbar or dialog should use the its accessible name
On 2013/11/20 22:13:27, David Tseng wrote:
> s/the its/its.
> 
> Perhaps rephrase
>     // A control in alert, toolbar or dialog should use its accessible name
> 
> as
> 
>     // A control inside of alert, toolbar or dialog should use that
container's
> accessible name

Done.

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

https://codereview.chromium.org/79273002/diff/30001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:406:
state.role == ui::AccessibilityTypes::ROLE_TOOLBAR) &&
On 2013/11/20 22:13:27, David Tseng wrote:
> Did you test this with infobars? It looks like the below block makes a best
> guess at the context based on the first static text child view for alerts and
> dialogs.

Well, this statement ensures that if a parent of such UI elements explicitly
defines the context, that's the one we should use instead of drilling through
its children nodes for the first static text. From what I can see in the code,
not many top level view containers (i.e. dialog, alerts) actually override
GetAccessibleState().

[David Tseng, 2013-11-21 01:33:02]

On Wed, Nov 20, 2013 at 4:19 PM, <zelidrag@chromium.org> wrote:

> PTAL
>
> I've added items 5-7 with the latest update.
>
> I'd actually rather we split native tree view  s into a separate cl.

Treeitem info's  need:
- child index
- child count
- depth
- expanded or collapsed
... so that a client can formulate an utterance like:
<down arrow>
Vacation bookmarks folder treeitem expanded level 3, 1 of 10.
<down arrow>
Summer 2011 bookmarks folder treeitem expanded level 4, 2 of 2.
<down arrow>
photos treeitem 1 of 1
<left arrow>
Summer 2011 bookmarks folder treeitem collapsed level 4, 2 of 2.

which is another reason why this API will be redone once we have a cache of
the views tree in the new AXTree/automation API.

>
> https://codereview.chromium.org/79273002/diff/1/chrome/
> app/bookmarks_strings.grdp
> File chrome/app/bookmarks_strings.grdp (right):
>
> https://codereview.chromium.org/79273002/diff/1/chrome/
> app/bookmarks_strings.grdp#newcode186
> chrome/app/bookmarks_strings.grdp:186: <message
> name="IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK_ACCESSIBILITY" desc="Title of
> the bubble when re-clicking on a bookmark, read by text-to-speach
> accessability feature">
> On 2013/11/20 22:13:27, David Tseng wrote:
>
>> s/accessability/accessibility (and you probably could just call it
>>
> spoken
>
>> feedback).
>>
>
> Done.
>
>
> https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/
> accessibility/accessibility_event_router_views.cc
> File
> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
> (right):
>
> https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/
> accessibility/accessibility_event_router_views.cc#newcode398
> chrome/browser/ui/views/accessibility/accessibility_
> event_router_views.cc:398:
> // A control in alert, toolbar or dialog should use the its accessible
> name
> On 2013/11/20 22:13:27, David Tseng wrote:
>
>> s/the its/its.
>>
>
>  Perhaps rephrase
>>      // A control in alert, toolbar or dialog should use its accessible
>>
> name
>
>  as
>>
>
>       // A control inside of alert, toolbar or dialog should use that
>>
> container's
>
>> accessible name
>>
>
> Done.
>
>
> https://codereview.chromium.org/79273002/diff/30001/
> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
> File
> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
> (right):
>
> https://codereview.chromium.org/79273002/diff/30001/
> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc#
> newcode406
> chrome/browser/ui/views/accessibility/accessibility_
> event_router_views.cc:406:
> state.role == ui::AccessibilityTypes::ROLE_TOOLBAR) &&
> On 2013/11/20 22:13:27, David Tseng wrote:
>
>> Did you test this with infobars? It looks like the below block makes a
>>
> best
>
>> guess at the context based on the first static text child view for
>>
> alerts and
>
>> dialogs.
>>
>
> Well, this statement ensures that if a parent of such UI elements
> explicitly defines the context, that's the one we should use instead of
> drilling through its children nodes for the first static text. From what
> I can see in the code, not many top level view containers (i.e. dialog,
> alerts) actually override GetAccessibleState().
>
>  <https://codereview.chromium.org/79273002/>

Yes understood. I just wanted to verify that we still speak the alert text
as context followed by the actual buttons (since we would prefer it to or
in addition to the name of the alert, if any).


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

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

[zelidrag1, 2013-11-21 06:45:10]

On Wed, Nov 20, 2013 at 5:32 PM, David Tseng <dtseng@chromium.org> wrote:

>
>
>
> On Wed, Nov 20, 2013 at 4:19 PM, <zelidrag@chromium.org> wrote:
>
>> PTAL
>>
>> I've added items 5-7 with the latest update.
>>
>> I'd actually rather we split native tree view  s into a separate cl.
>
> Treeitem info's  need:
> - child index
> - child count
> - depth
> - expanded or collapsed
> ... so that a client can formulate an utterance like:
> <down arrow>
> Vacation bookmarks folder treeitem expanded level 3, 1 of 10.
> <down arrow>
> Summer 2011 bookmarks folder treeitem expanded level 4, 2 of 2.
> <down arrow>
> photos treeitem 1 of 1
> <left arrow>
> Summer 2011 bookmarks folder treeitem collapsed level 4, 2 of 2.
>
> which is another reason why this API will be redone once we have a cache
> of the views tree in the new AXTree/automation API.
>

I am ok with splitting CL, but let's chat about the spoken context details
tomorrow. It's should be easy too add extra stuff there but Peter was
concerned that if we say too much it would be too confusing for regular
user.



>> https://codereview.chromium.org/79273002/diff/1/chrome/
>> app/bookmarks_strings.grdp
>> File chrome/app/bookmarks_strings.grdp (right):
>>
>> https://codereview.chromium.org/79273002/diff/1/chrome/
>> app/bookmarks_strings.grdp#newcode186
>> chrome/app/bookmarks_strings.grdp:186: <message
>> name="IDS_BOOKMARK_BUBBLE_PAGE_BOOKMARK_ACCESSIBILITY" desc="Title of
>> the bubble when re-clicking on a bookmark, read by text-to-speach
>> accessability feature">
>> On 2013/11/20 22:13:27, David Tseng wrote:
>>
>>> s/accessability/accessibility (and you probably could just call it
>>>
>> spoken
>>
>>> feedback).
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/
>> accessibility/accessibility_event_router_views.cc
>> File
>> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
>> (right):
>>
>> https://codereview.chromium.org/79273002/diff/1/chrome/browser/ui/views/
>> accessibility/accessibility_event_router_views.cc#newcode398
>> chrome/browser/ui/views/accessibility/accessibility_
>> event_router_views.cc:398:
>> // A control in alert, toolbar or dialog should use the its accessible
>> name
>> On 2013/11/20 22:13:27, David Tseng wrote:
>>
>>> s/the its/its.
>>>
>>
>>  Perhaps rephrase
>>>      // A control in alert, toolbar or dialog should use its accessible
>>>
>> name
>>
>>  as
>>>
>>
>>       // A control inside of alert, toolbar or dialog should use that
>>>
>> container's
>>
>>> accessible name
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/79273002/diff/30001/
>> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
>> File
>> chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc
>> (right):
>>
>> https://codereview.chromium.org/79273002/diff/30001/
>> chrome/browser/ui/views/accessibility/accessibility_
>> event_router_views.cc#newcode406
>> chrome/browser/ui/views/accessibility/accessibility_
>> event_router_views.cc:406:
>> state.role == ui::AccessibilityTypes::ROLE_TOOLBAR) &&
>> On 2013/11/20 22:13:27, David Tseng wrote:
>>
>>> Did you test this with infobars? It looks like the below block makes a
>>>
>> best
>>
>>> guess at the context based on the first static text child view for
>>>
>> alerts and
>>
>>> dialogs.
>>>
>>
>> Well, this statement ensures that if a parent of such UI elements
>> explicitly defines the context, that's the one we should use instead of
>> drilling through its children nodes for the first static text. From what
>> I can see in the code, not many top level view containers (i.e. dialog,
>> alerts) actually override GetAccessibleState().
>>
>>  <https://codereview.chromium.org/79273002/>
>
> Yes understood. I just wanted to verify that we still speak the alert text
> as context followed by the actual buttons (since we would prefer it to or
> in addition to the name of the alert, if any).
>
>
> https://codereview.chromium.org/79273002/
>>
>
>

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

[dmazzoni, 2013-11-21 06:54:06]

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

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:296:
views::TreeView* tree = static_cast<views::TreeView*>(view);
Why is the view with a role of OUTLINEITEM the one with type TreeView? 

Is it possible to get this information directly from the accessibility info on
generic Views rather than by casting to a TreeView?

If you iterate over the children of the tree view, the selected item's view
should have STATE_SELECTED on it, and you can also use STATE_HASPOPUP to
indicate if it has a submenu.

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:302:
int count = -1;
Can we set the count even if there isn't a selected node?

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:302: state->role =
ui::AccessibilityTypes::ROLE_DIALOG;
Do this in BubbleDelegateView instead (and call the inherited function at the
top, here).

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

https://codereview.chromium.org/79273002/diff/140001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:409: state->role =
ui::AccessibilityTypes::ROLE_OUTLINEITEM;
Oh! I think I understand - the whole tree is represented by just a single view?
Are there not views for the individual items within the tree that we can set
properties on? That's inconvenient.

[zel, 2013-11-21 07:02:26]

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

https://codereview.chromium.org/79273002/diff/140001/ui/views/controls/tree/t...
ui/views/controls/tree/tree_view.cc:409: state->role =
ui::AccessibilityTypes::ROLE_OUTLINEITEM;
On 2013/11/21 06:54:07, Dominic Mazzoni wrote:
> Oh! I think I understand - the whole tree is represented by just a single
view?
> Are there not views for the individual items within the tree that we can set
> properties on? That's inconvenient.

Yep, the whole TreeView is one big fat View.

[dmazzoni, 2013-11-21 07:05:50]

On 2013/11/21 07:02:26, zel wrote:
>
https://codereview.chromium.org/79273002/diff/140001/ui/views/controls/tree/t...
> File ui/views/controls/tree/tree_view.cc (right):
> 
>
https://codereview.chromium.org/79273002/diff/140001/ui/views/controls/tree/t...
> ui/views/controls/tree/tree_view.cc:409: state->role =
> ui::AccessibilityTypes::ROLE_OUTLINEITEM;
> On 2013/11/21 06:54:07, Dominic Mazzoni wrote:
> > Oh! I think I understand - the whole tree is represented by just a single
> view?
> > Are there not views for the individual items within the tree that we can set
> > properties on? That's inconvenient.
> 
> Yep, the whole TreeView is one big fat View.

Ugh. OK, ignore my other related comment then - this is fine for now, but it
means we have a lot more work to do for Windows and for the next-gen API.

[zel, 2013-11-22 02:34:01]

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

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:296:
views::TreeView* tree = static_cast<views::TreeView*>(view);
On 2013/11/21 06:54:07, Dominic Mazzoni wrote:
> Why is the view with a role of OUTLINEITEM the one with type TreeView? 
> 
> Is it possible to get this information directly from the accessibility info on
> generic Views rather than by casting to a TreeView?
> 
> If you iterate over the children of the tree view, the selected item's view
> should have STATE_SELECTED on it, and you can also use STATE_HASPOPUP to
> indicate if it has a submenu.

moved to another CL, will be addresses there

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/accessibility/accessibility_event_router_views.cc:302:
int count = -1;
On 2013/11/21 06:54:07, Dominic Mazzoni wrote:
> Can we set the count even if there isn't a selected node?

moved to another CL, will be addresses there

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/79273002/diff/140001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:302: state->role =
ui::AccessibilityTypes::ROLE_DIALOG;
On 2013/11/21 06:54:07, Dominic Mazzoni wrote:
> Do this in BubbleDelegateView instead (and call the inherited function at the
> top, here).

Done.

[dmazzoni, 2013-11-22 05:04:23]

lgtm

[sky, 2013-11-22 18:34:11]

ui/views LGTM

[commit-bot: I haz the power, 2013-11-22 18:38:10]

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

[commit-bot: I haz the power, 2013-11-22 19:31:27]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-22 19:53:06]

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

[commit-bot: I haz the power, 2013-11-22 22:19:03]

Change committed as 236841