Page Info (Views): remove images from accessibility order

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/website_settings/permission_selector_row.h"
 
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/views/website_settings/website_settings_popup_view.h"
 #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"
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/button/menu_button.h"
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/combobox/combobox_listener.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
 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
 // permissions type.
 class PermissionMenuButton : public views::MenuButton,
                              public views::MenuButtonListener {
  public:
   // Creates a new |PermissionMenuButton| with the passed |text|. The ownership
   // of the |model| remains with the caller and is not transfered to the
   // |PermissionMenuButton|. If the |show_menu_marker| flag is true, then a
@@ skipping 201 matching lines @@
                         0);
   column_set->AddPaddingColumn(0, kPermissionIconMarginLeft);
   column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
                         views::GridLayout::USE_PREF, 0, 0);
   column_set->AddPaddingColumn(1, kMinSeparationBetweenLabelAndMenu);
   column_set->AddColumn(views::GridLayout::TRAILING, views::GridLayout::FILL, 0,
                         views::GridLayout::USE_PREF, 0, 0);
 
   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_,
                   1,
                   1,
                   views::GridLayout::CENTER,
                   views::GridLayout::CENTER);
   // Create the label that displays the permission type.
   views::Label* label = new views::Label(
       WebsiteSettingsUI::PermissionTypeToUIString(permission.type));
@@ skipping 97 matching lines @@
         permission.default_setting, content_settings::SETTING_SOURCE_USER));
     menu_button_->SizeToPreferredSize();
   } else if (combobox_) {
     bool use_default = permission.setting == CONTENT_SETTING_DEFAULT;
     combobox_->UpdateSelectedIndex(use_default);
   }
 
   for (PermissionSelectorRowObserver& observer : observer_list_)
     observer.OnPermissionChanged(permission);
 }