Page Info (Views): remove images from accessibility order

chrome/browser/ui/views/website_settings/permission_selector_row.cc

Index: chrome/browser/ui/views/website_settings/permission_selector_row.cc
diff --git a/chrome/browser/ui/views/website_settings/permission_selector_row.cc b/chrome/browser/ui/views/website_settings/permission_selector_row.cc
index a5792608069330abb39ce653fde76cf59828a7d1..5f36c7366287cf9106392e574ed6ddc08649cd21 100644
--- a/chrome/browser/ui/views/website_settings/permission_selector_row.cc
+++ b/chrome/browser/ui/views/website_settings/permission_selector_row.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/ui/website_settings/permission_menu_model.h"
 #include "chrome/browser/ui/website_settings/website_settings_ui.h"
 #include "chrome/grit/generated_resources.h"
+#include "ui/accessibility/ax_enums.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/models/combobox_model.h"
@@ -28,8 +29,18 @@
 namespace {
 // Minimum distance between the label and its corresponding menu.
 const int kMinSeparationBetweenLabelAndMenu = 16;
+
+class NonAccessibleImageView : public views::ImageView {

[msw, 2016/11/10 03:04:15]

Yeah, even though it's a fairly simple subclass, it makes sense to consolidate
the two impls in a
chrome/browser/ui/views/website_settings/non_accessible_image_view.[h|cc] or
similar.

+  // Overridden from views::View.

[msw, 2016/11/10 03:04:14]

nit: s/View/ImageView/, views::ImageView itself has an override.

+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
+};

[msw, 2016/11/10 03:04:14]

DISALLOW_COPY_AND_ASSIGN (please add an explicit private: visibility label).

+
+void NonAccessibleImageView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  node_data->role = ui::AX_ROLE_IGNORED;

[msw, 2016/11/10 03:04:14]

I wonder if SetFocusBehavior(NEVER) would suffice, but probably not...
+dmazzoni@ for review; is this okay, do you see a better approach (eg. add a
mutable AXRole member and setter to views::ImageView?).

[dmazzoni, 2016/11/10 18:13:59]

How about node_data->AddStateFlag(ui::AX_STATE_INVISIBLE)?

I think making it invisible will already do the right thing on Windows and
Chrome OS.

If it doesn't work for MacViews, file a bug and assign it to patricialor@.
Semantically, marking it as invisible is more clear. The right way to achieve
that on Mac is an implementation detail - once we figure it out that can go
into the MacViews code.

If you want to avoid the subclass and there are other examples of ImageViews
that have this need, I'd add something specific to ImageView, like
SetDecorativeImageOnlyForAccessibility(true) and have that set a flag
that affects ImageView::GetAccessibleNodeData.

[lgarron, 2016/11/10 22:59:54]

The following does not work:

    icon->SetFocusBehavior(views::View::FocusBehavior::NEVER);

I've filed https://crbug.com/664320

dmazzoni@, how do you feel about NonAccessibleImageView for now? I can file a
followup bug to change it once we have a standard approach.

 }
 
+}  // namespace
+
 namespace internal {
 
 // The |PermissionMenuButton| provides a menu for selecting a setting a
@@ -251,7 +262,7 @@ PermissionSelectorRow::PermissionSelectorRow(
 
   layout->StartRow(1, column_set_id);
   // Create the permission icon.
-  icon_ = new views::ImageView();
+  icon_ = new NonAccessibleImageView();
   const gfx::Image& image = WebsiteSettingsUI::GetPermissionIcon(permission);
   icon_->SetImage(image.ToImageSkia());
   layout->AddView(icon_,