Fix screen reader not reading extensions and zoom buttons in app menu.

At least on Windows, the screen reader will not read non-menu-item
Views in a menu. Currently, extensions hidden in the app menu and
the zoom plus and minus buttons are not read. This patch tags them as
kMenuItem, which fixes the bug.

Bug: 835738
Change-Id: I507c9595f6aa02d8768330f18a8b725ef1b440e4
Reviewed-on: https://chromium-review.googlesource.com/c/1354482Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612302}

chrome/browser/ui/views/toolbar/app_menu.cc

@@ -250,10 +250,6 @@ class InMenuButton : public LabelButton {
       : LabelButton(listener, text) {}
   ~InMenuButton() override {}
 
-  void set_role_is_button(bool role_is_button) {
-    role_is_button_ = role_is_button;
-  }
-
   void Init(InMenuButtonBackground::ButtonType type) {
     // An InMenuButton should always be focusable regardless of the platform.
     // Hence we don't use SetFocusForPlatform().
@@ -268,8 +264,7 @@ class InMenuButton : public LabelButton {
 
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     LabelButton::GetAccessibleNodeData(node_data);
-    if (!role_is_button_)
-      node_data->role = ax::mojom::Role::kMenuItem;
+    node_data->role = ax::mojom::Role::kMenuItem;
   }
 
   // views::LabelButton
@@ -295,10 +290,6 @@ class InMenuButton : public LabelButton {
   }
 
  private:
-  // Indicates whether to expose this to accessibility as a Button.  If it is a
-  // button, the accelerator will not be added to the accessible label.
-  bool role_is_button_ = false;
-
   DISALLOW_COPY_AND_ASSIGN(InMenuButton);
 };
 
@@ -337,14 +328,15 @@ class AppMenuView : public views::View,
       int string_id,
       InMenuButtonBackground::ButtonType type,
       int index) {
-    return CreateButtonWithAccName(string_id, type, index, string_id, false);
+    return CreateButtonWithAccName(string_id, type, index, string_id,
+                                   /*add_accelerator_text*/ true);
   }
 
   InMenuButton* CreateButtonWithAccName(int string_id,
                                         InMenuButtonBackground::ButtonType type,
                                         int index,
                                         int acc_string_id,
-                                        bool role_is_button) {
+                                        bool add_accelerator_text) {
     // Should only be invoked during construction when |menu_| is valid.
     DCHECK(menu_);
     InMenuButton* button = new InMenuButton(
@@ -352,10 +344,9 @@ class AppMenuView : public views::View,
                                          '&', nullptr, nullptr));
     button->Init(type);
     button->SetAccessibleName(GetAccessibleNameForAppMenuItem(
-        menu_model_, index, acc_string_id, !role_is_button));
+        menu_model_, index, acc_string_id, add_accelerator_text));
     button->set_tag(index);
     button->SetEnabled(menu_model_->IsEnabledAt(index));
-    button->set_role_is_button(role_is_button);
 
     AddChildView(button);
     // all buttons on menu should must be a custom button in order for
@@ -502,7 +493,8 @@ class AppMenu::ZoomView : public AppMenuView {
 
     decrement_button_ = CreateButtonWithAccName(
         IDS_ZOOM_MINUS2, InMenuButtonBackground::LEADING_BORDER,
-        decrement_index, IDS_ACCNAME_ZOOM_MINUS2, true);
+        decrement_index, IDS_ACCNAME_ZOOM_MINUS2,
+        /*add_accelerator_text*/ false);
 
     zoom_label_ = new Label(base::FormatPercent(100));
     zoom_label_->SetAutoColorReadabilityEnabled(false);
@@ -520,7 +512,7 @@ class AppMenu::ZoomView : public AppMenuView {
 
     increment_button_ = CreateButtonWithAccName(
         IDS_ZOOM_PLUS2, InMenuButtonBackground::NO_BORDER, increment_index,
-        IDS_ACCNAME_ZOOM_PLUS2, true);
+        IDS_ACCNAME_ZOOM_PLUS2, /*add_accelerator_text*/ false);
 
     fullscreen_button_ = new FullscreenButton(this);
     // all buttons on menu should must be a custom button in order for
@@ -540,7 +532,8 @@ class AppMenu::ZoomView : public AppMenuView {
     fullscreen_button_->SetBackground(std::make_unique<InMenuButtonBackground>(
         InMenuButtonBackground::LEADING_BORDER));
     fullscreen_button_->SetAccessibleName(GetAccessibleNameForAppMenuItem(
-        menu_model, fullscreen_index, IDS_ACCNAME_FULLSCREEN, true));
+        menu_model, fullscreen_index, IDS_ACCNAME_FULLSCREEN,
+        /*add_accelerator_text*/ true));
     AddChildView(fullscreen_button_);
 
     // Need to set a font list for the zoom label width calculations.

chrome/browser/ui/views/toolbar/browser_actions_container.h

@@ -164,7 +164,7 @@ class BrowserActionsContainer : public views::View,
   std::string GetIdAt(size_t index) const;
 
   // Returns the ToolbarActionView* associated with the given |extension|, or
-  // NULL if none exists.
+  // nullptr if none exists.
   ToolbarActionView* GetViewForId(const std::string& id);
 
   // Update the views to reflect the state of the toolbar actions.
@@ -258,8 +258,6 @@ class BrowserActionsContainer : public views::View,
   // A struct representing the position at which an action will be dropped.
   struct DropPosition;
 
-  typedef std::vector<std::unique_ptr<ToolbarActionView>> ToolbarActionViews;
-
   // Clears the |active_bubble_|, and unregisters the container as an observer.
   void ClearActiveBubble(views::Widget* widget);
 
@@ -285,8 +283,8 @@ class BrowserActionsContainer : public views::View,
   // The controlling ToolbarActionsBar, which handles most non-view logic.
   std::unique_ptr<ToolbarActionsBar> toolbar_actions_bar_;
 
-  // The vector of toolbar actions (icons/image buttons for each action).
-  ToolbarActionViews toolbar_action_views_;
+  // Child toolbar action buttons.
+  std::vector<std::unique_ptr<ToolbarActionView>> toolbar_action_views_;
 
   // The Browser object the container is associated with.
   Browser* const browser_;

chrome/browser/ui/views/toolbar/toolbar_action_view.cc

@@ -97,7 +97,8 @@ gfx::Rect ToolbarActionView::GetAnchorBoundsInScreen() const {
 
 void ToolbarActionView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   views::MenuButton::GetAccessibleNodeData(node_data);
-  node_data->role = ax::mojom::Role::kButton;
+  node_data->role = delegate_->ShownInsideMenu() ? ax::mojom::Role::kMenuItem
+                                                 : ax::mojom::Role::kButton;
 }
 
 std::unique_ptr<LabelButtonBorder> ToolbarActionView::CreateDefaultBorder()