Views: Add a11y information for IconLabelBubbleView and LocationIconView.

Views: Add a11y information for IconLabelBubbleView and LocationIconView.

IconLabelBubbleView is a View for displaying an icon + label; LocationIconView
specifically is the icon + text shown on the left on the Omnibox URL (typically
when a site is deemed to be secure). Neither of these classes have any
accessibility information set on them, despite LocationIconView being a
focusable and clickable target. This change adds a11y information to both.

BUG=668930
Committed: https://crrev.com/2d053e7da4da085c320a6a821377f429523afd63
Cr-Commit-Position: refs/heads/master@{#436832}

[Patti Lor, 2016-11-28 06:57:26]

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

[commit-bot: I haz the power, 2016-11-28 06:57:34]

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-11-28 09:22:53]

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

[commit-bot: I haz the power, 2016-11-28 09:22:54]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-11-28 23:30:53]

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

[commit-bot: I haz the power, 2016-11-28 23:33:28]

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

[Patti Lor, 2016-11-28 23:39:26]

Description was changed from

==========
Views: Add a11y information for LocationIconView.

BUG=668930
==========

to

==========
Views: Add a11y information for IconLabelBubbleView and LocationIconView.

IconLabelBubbleView is a View for displaying an icon + label; LocationIconView
specifically is the icon + text shown on the left on the Omnibox URL (typically
when a site is deemed to be secure). Neither of these classes have any
accessibility information set on them, despite LocationIconView being a
focusable and clickable target. This change adds a11y information to both.

BUG=668930
==========

[Patti Lor, 2016-11-28 23:41:36]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2016-11-28 23:41:37]

Hi Trent, PTAL. This is the fix for the LocationIconView being invisible to a11y
clients after changes in https://codereview.chromium.org/2119413004/.

[commit-bot: I haz the power, 2016-11-29 01:48:06]

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

[commit-bot: I haz the power, 2016-11-29 01:48:06]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-11-29 03:22:52]

lgtm % nits

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:70: //
views::InkDropHostView:
this should still be View - we name the class that declares the methods usually

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:77: void
AddInkDropLayer(ui::Layer* ink_drop_layer) override;
However, there should be a comment before this -- these are overrides of
InkDropHost:

[Patti Lor, 2016-11-30 05:06:11]

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

[commit-bot: I haz the power, 2016-11-30 05:06:57]

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

[Patti Lor, 2016-11-30 05:11:33]

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

[Patti Lor, 2016-11-30 05:11:34]

Hi dmazzoni - could you please take a quick look at this before I send to
owners? The thing I'm not sure about is whether we should be also adding some
description about the icon to be displayed and whether it's OK to label the
entire thing "static text" when that's not entirely true. Are there other
precedents for these kinds of situations?

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:70: //
views::InkDropHostView:
On 2016/11/29 03:22:51, tapted wrote:
> this should still be View - we name the class that declares the methods
usually

Done - thanks, I was a little confused about this.

https://codereview.chromium.org/2530353002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:77: void
AddInkDropLayer(ui::Layer* ink_drop_layer) override;
On 2016/11/29 03:22:51, tapted wrote:
> However, there should be a comment before this -- these are overrides of
> InkDropHost:

Done.

[commit-bot: I haz the power, 2016-11-30 08:36:00]

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

[commit-bot: I haz the power, 2016-11-30 08:36:01]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-11-30 23:14:50]

lgtm, but I'm not entirely clear about what you were
asking about the whole thing being static text. A role
of pop-up button sounds great.

[Patti Lor, 2016-11-30 23:18:54]

On 2016/11/30 23:14:50, dmazzoni wrote:
> lgtm, but I'm not entirely clear about what you were
> asking about the whole thing being static text. A role
> of pop-up button sounds great.

Sorry - I meant IconLabelBubbleView will now be using Label's accessibility
information. I wasn't sure if this was totally appropriate if there's
technically meant to be an icon in there as well?

[dmazzoni, 2016-12-01 21:30:33]

On 2016/11/30 23:18:54, Patti Lor wrote:
> On 2016/11/30 23:14:50, dmazzoni wrote:
> > lgtm, but I'm not entirely clear about what you were
> > asking about the whole thing being static text. A role
> > of pop-up button sounds great.
> 
> Sorry - I meant IconLabelBubbleView will now be using Label's accessibility
> information. I wasn't sure if this was totally appropriate if there's
> technically meant to be an icon in there as well?

Oh! That sounds fine.

[Patti Lor, 2016-12-05 01:07:19]

patricialor@chromium.org changed reviewers:
+ pkasting@chromium.org

[Patti Lor, 2016-12-05 01:07:20]

Thanks for the advice dmazzoni.

pkasting@chromium.org: Please review changes in 
chrome/browser/ui/views/location_bar/

Thanks!

[Peter Kasting, 2016-12-06 04:20:28]

Not reviewing the actual accessibility functionality, assuming dmazzoni's review
is sufficient for that.

LGTM

If you move the declarations of methods, make sure you move the definitions to
match.

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:76: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
Nit: This goes above OnNativeThemeChanged (match superclass declaration order).

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:78: //
views::InkDropHostView:
Nit: Don't add this separate divider, instead reword the "views::View" comment
above to "views::InkDropHostView", since that's the only direct base class of
this class (it doesn't directly subclass View).

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:39: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
Nit: Goes above GetTextColor() (matches superclasses' declaration order)

[Patti Lor, 2016-12-06 05:11:34]

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

[commit-bot: I haz the power, 2016-12-06 05:11:42]

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-12-06 05:56:02]

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

[commit-bot: I haz the power, 2016-12-06 05:56:03]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2016-12-06 07:07:21]

Thanks for the review! Please check my reply for
https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
? Thanks.

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:76: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
On 2016/12/06 04:20:28, Peter Kasting wrote:
> Nit: This goes above OnNativeThemeChanged (match superclass declaration
order).

Thanks for picking that up, fixed!

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:78: //
views::InkDropHostView:
On 2016/12/06 04:20:28, Peter Kasting wrote:
> Nit: Don't add this separate divider, instead reword the "views::View" comment
> above to "views::InkDropHostView", since that's the only direct base class of
> this class (it doesn't directly subclass View).

Done.

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:39: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
On 2016/12/06 04:20:28, Peter Kasting wrote:
> Nit: Goes above GetTextColor() (matches superclasses' declaration order)

I'm a bit confused here, isn't GetAccessibleNodeData() declared after
IconLabelBubbleView::GetTextColor() and IconLabelBubbleView::OnActivate()? I've
left as is for now, but please clarify if I'm misunderstanding.

[Peter Kasting, 2016-12-06 07:22:35]

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:39: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
On 2016/12/06 07:07:21, Patti Lor wrote:
> On 2016/12/06 04:20:28, Peter Kasting wrote:
> > Nit: Goes above GetTextColor() (matches superclasses' declaration order)
> 
> I'm a bit confused here, isn't GetAccessibleNodeData() declared after
> IconLabelBubbleView::GetTextColor() and IconLabelBubbleView::OnActivate()?
I've
> left as is for now, but please clarify if I'm misunderstanding.

This is a tricky case, since IconLabelBubbleView overrides that method after
declaring the others, and the actual override itself comes from a base class.

Since there are no formal rules governing this order (the "keep in superclass
order" is an informal, best-effort rule in some parts of Views), the real answer
is "do whatever you want".

My informal answer would be that GetAccessibleNodeData() was originally declared
in View rather than IconLabelBubbleView, it will appear first in the overall
vtable, and thus should go first.  This will tend to order things in groups:
"Methods originally declared in View, Methods originally declared in
IconLabelBubbleView, etc." But Views code itself is not consistent about this,
so don't worry too much.

[Patti Lor, 2016-12-07 01:56:59]

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_icon_view.h (right):

https://codereview.chromium.org/2530353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_icon_view.h:39: void
GetAccessibleNodeData(ui::AXNodeData* node_data) override;
On 2016/12/06 07:22:34, Peter Kasting wrote:
> On 2016/12/06 07:07:21, Patti Lor wrote:
> > On 2016/12/06 04:20:28, Peter Kasting wrote:
> > > Nit: Goes above GetTextColor() (matches superclasses' declaration order)
> > 
> > I'm a bit confused here, isn't GetAccessibleNodeData() declared after
> > IconLabelBubbleView::GetTextColor() and IconLabelBubbleView::OnActivate()?
> I've
> > left as is for now, but please clarify if I'm misunderstanding.
> 
> This is a tricky case, since IconLabelBubbleView overrides that method after
> declaring the others, and the actual override itself comes from a base class.
> 
> Since there are no formal rules governing this order (the "keep in superclass
> order" is an informal, best-effort rule in some parts of Views), the real
answer
> is "do whatever you want".
> 
> My informal answer would be that GetAccessibleNodeData() was originally
declared
> in View rather than IconLabelBubbleView, it will appear first in the overall
> vtable, and thus should go first.  This will tend to order things in groups:
> "Methods originally declared in View, Methods originally declared in
> IconLabelBubbleView, etc." But Views code itself is not consistent about this,
> so don't worry too much.

Ok, thanks for your explanation!

[Patti Lor, 2016-12-07 01:57:09]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2016-12-07 01:57:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
dmazzoni@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2530353002/#ps60001
(title: "Reorder method declaration to match superclass.")

[commit-bot: I haz the power, 2016-12-07 01:57:38]

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-12-07 02:04:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481075829268540,
"parent_rev": "78e67e6fc49cd13c7c7abbf90a5cee0339398b6b", "commit_rev":
"baf0f89f8883ff987eea5fdfaa2375f38b93f9cc"}

[commit-bot: I haz the power, 2016-12-07 02:05:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-07 02:10:12]

Description was changed from

==========
Views: Add a11y information for IconLabelBubbleView and LocationIconView.

IconLabelBubbleView is a View for displaying an icon + label; LocationIconView
specifically is the icon + text shown on the left on the Omnibox URL (typically
when a site is deemed to be secure). Neither of these classes have any
accessibility information set on them, despite LocationIconView being a
focusable and clickable target. This change adds a11y information to both.

BUG=668930
==========

to

==========
Views: Add a11y information for IconLabelBubbleView and LocationIconView.

IconLabelBubbleView is a View for displaying an icon + label; LocationIconView
specifically is the icon + text shown on the left on the Omnibox URL (typically
when a site is deemed to be secure). Neither of these classes have any
accessibility information set on them, despite LocationIconView being a
focusable and clickable target. This change adds a11y information to both.

BUG=668930

Committed: https://crrev.com/2d053e7da4da085c320a6a821377f429523afd63
Cr-Commit-Position: refs/heads/master@{#436832}
==========

[commit-bot: I haz the power, 2016-12-07 02:10:13]

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