Remove mnemonic and add keyboard shortcut text to the accessible name of text buttons in the wrench menu.

Add breaks to GetAccessibleState switch statements.

BUG=51738
TEST=Verify in NVDA and JAWS that text buttons are read without mnemonic and with keyboard shortcut.

Review URL: http://codereview.chromium.org/3612001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61201 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 121 insertions, 81 deletions

diff --git a/chrome/browser/views/wrench_menu.cc b/chrome/browser/views/wrench_menu.cc
index 2aa4b9b..8236492 100644
--- a/chrome/browser/views/wrench_menu.cc
+++ b/chrome/browser/views/wrench_menu.cc
@@ -244,61 +244,98 @@ class ScheduleAllView : public views::View {
   DISALLOW_COPY_AND_ASSIGN(ScheduleAllView);
 };
 
-TextButton* CreateAndConfigureButton(View* parent,
-                                     views::ButtonListener* listener,
-                                     int string_id,
-                                     MenuButtonBackground::ButtonType type,
-                                     MenuModel* model,
-                                     int index,
-                                     MenuButtonBackground** background) {
-  TextButton* button =
-      new TextButton(listener, l10n_util::GetString(string_id));
-  button->SetFocusable(true);
-  button->set_request_focus_on_press(false);
-  button->set_tag(index);
-  button->SetEnabled(model->IsEnabledAt(index));
-  button->set_prefix_type(TextButton::PREFIX_HIDE);
-  MenuButtonBackground* bg = new MenuButtonBackground(type);
-  button->set_background(bg);
-  button->SetEnabledColor(MenuConfig::instance().text_color);
-  if (background)
-    *background = bg;
-  button->set_border(new MenuButtonBorder());
-  button->set_alignment(TextButton::ALIGN_CENTER);
-  button->SetNormalHasBorder(true);
-  button->SetFont(views::MenuConfig::instance().font);
-  button->ClearMaxTextSize();
-  parent->AddChildView(button);
-  return button;
+std::wstring GetAccessibleNameForWrenchMenuItem(
+      MenuModel* model, int item_index, int accessible_string_id) {
+  std::wstring accessible_name = l10n_util::GetString(accessible_string_id);
+  std::wstring accelerator_text;
+
+  menus::Accelerator menu_accelerator;
+  if (model->GetAcceleratorAt(item_index, &menu_accelerator)) {
+    accelerator_text =
+        views::Accelerator(menu_accelerator.GetKeyCode(),
+                           menu_accelerator.modifiers()).GetShortcutText();
+  }
+
+  return MenuItemView::GetAccessibleNameForMenuItem(
+      accessible_name, accelerator_text);
 }
 
+// WrenchMenuView is a view that can contain text buttons.
+class WrenchMenuView : public ScheduleAllView, public views::ButtonListener {
+ public:
+  WrenchMenuView(WrenchMenu* menu, MenuModel* menu_model)
+      : menu_(menu), menu_model_(menu_model) { }
+
+  TextButton* CreateAndConfigureButton(int string_id,
+                                       MenuButtonBackground::ButtonType type,
+                                       int index,
+                                       MenuButtonBackground** background) {
+    return CreateButtonWithAccName(
+      string_id, type, index, background, string_id);
+  }
+
+  TextButton* CreateButtonWithAccName(int string_id,
+                                      MenuButtonBackground::ButtonType type,
+                                      int index,
+                                      MenuButtonBackground** background,
+                                      int acc_string_id) {
+    TextButton* button =
+        new TextButton(this, l10n_util::GetString(string_id));
+    button->SetAccessibleName(
+        GetAccessibleNameForWrenchMenuItem(menu_model_, index, acc_string_id));
+    button->SetFocusable(true);
+    button->set_request_focus_on_press(false);
+    button->set_tag(index);
+    button->SetEnabled(menu_model_->IsEnabledAt(index));
+    button->set_prefix_type(TextButton::PREFIX_HIDE);
+    MenuButtonBackground* bg = new MenuButtonBackground(type);
+    button->set_background(bg);
+    button->SetEnabledColor(MenuConfig::instance().text_color);
+    if (background)
+      *background = bg;
+    button->set_border(new MenuButtonBorder());
+    button->set_alignment(TextButton::ALIGN_CENTER);
+    button->SetNormalHasBorder(true);
+    button->SetFont(views::MenuConfig::instance().font);
+    button->ClearMaxTextSize();
+    AddChildView(button);
+    return button;
+  }
+
+ protected:
+  // Hosting WrenchMenu.
+  WrenchMenu* menu_;
+
+  // The menu model containing the increment/decrement/reset items.
+  MenuModel* menu_model_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WrenchMenuView);
+};
+
 }  // namespace
 
 // CutCopyPasteView ------------------------------------------------------------
 
 // CutCopyPasteView is the view containing the cut/copy/paste buttons.
-class WrenchMenu::CutCopyPasteView : public ScheduleAllView,
-                                     public views::ButtonListener {
+class WrenchMenu::CutCopyPasteView : public WrenchMenuView {
  public:
   CutCopyPasteView(WrenchMenu* menu,
                    MenuModel* menu_model,
                    int cut_index,
                    int copy_index,
                    int paste_index)
-      : menu_(menu),
-        menu_model_(menu_model) {
+      : WrenchMenuView(menu, menu_model) {
     TextButton* cut = CreateAndConfigureButton(
-        this, this, IDS_CUT, MenuButtonBackground::LEFT_BUTTON, menu_model,
-        cut_index, NULL);
+        IDS_CUT, MenuButtonBackground::LEFT_BUTTON, cut_index, NULL);
 
     MenuButtonBackground* copy_background = NULL;
     CreateAndConfigureButton(
-        this, this, IDS_COPY, MenuButtonBackground::CENTER_BUTTON, menu_model,
-        copy_index, &copy_background);
+        IDS_COPY, MenuButtonBackground::CENTER_BUTTON, copy_index,
+        &copy_background);
 
     TextButton* paste = CreateAndConfigureButton(
-        this, this, IDS_PASTE, MenuButtonBackground::RIGHT_BUTTON, menu_model,
-        paste_index, NULL);
+        IDS_PASTE, MenuButtonBackground::RIGHT_BUTTON, paste_index, NULL);
 
     copy_background->SetOtherButtons(cut, paste);
   }
@@ -330,9 +367,6 @@ class WrenchMenu::CutCopyPasteView : public ScheduleAllView,
     return width;
   }
 
-  WrenchMenu* menu_;
-  MenuModel* menu_model_;
-
   DISALLOW_COPY_AND_ASSIGN(CutCopyPasteView);
 };
 
@@ -344,8 +378,7 @@ static const int kZoomPadding = 6;
 // ZoomView contains the various zoom controls: two buttons to increase/decrease
 // the zoom, a label showing the current zoom percent, and a button to go
 // full-screen.
-class WrenchMenu::ZoomView : public ScheduleAllView,
-                             public views::ButtonListener,
+class WrenchMenu::ZoomView : public WrenchMenuView,
                              public NotificationObserver {
  public:
   ZoomView(WrenchMenu* menu,
@@ -353,19 +386,16 @@ class WrenchMenu::ZoomView : public ScheduleAllView,
            int decrement_index,
            int increment_index,
            int fullscreen_index)
-      : menu_(menu),
-        menu_model_(menu_model),
+      : WrenchMenuView(menu, menu_model),
         fullscreen_index_(fullscreen_index),
         increment_button_(NULL),
         zoom_label_(NULL),
         decrement_button_(NULL),
         fullscreen_button_(NULL),
         zoom_label_width_(0) {
-    decrement_button_ = CreateAndConfigureButton(
-        this, this, IDS_ZOOM_MINUS2, MenuButtonBackground::LEFT_BUTTON,
-        menu_model, decrement_index, NULL);
-    decrement_button_->SetAccessibleName(
-        l10n_util::GetString(IDS_ACCNAME_ZOOM_MINUS2));
+    decrement_button_ = CreateButtonWithAccName(
+        IDS_ZOOM_MINUS2, MenuButtonBackground::LEFT_BUTTON, decrement_index,
+        NULL, IDS_ACCNAME_ZOOM_MINUS2);
 
     zoom_label_ = new Label(l10n_util::GetStringF(IDS_ZOOM_PERCENT, L"100"));
     zoom_label_->SetColor(MenuConfig::instance().text_color);
@@ -378,11 +408,9 @@ class WrenchMenu::ZoomView : public ScheduleAllView,
     AddChildView(zoom_label_);
     zoom_label_width_ = MaxWidthForZoomLabel();
 
-    increment_button_ = CreateAndConfigureButton(
-        this, this, IDS_ZOOM_PLUS2, MenuButtonBackground::RIGHT_BUTTON,
-        menu_model, increment_index, NULL);
-    increment_button_->SetAccessibleName(
-        l10n_util::GetString(IDS_ACCNAME_ZOOM_PLUS2));
+    increment_button_ = CreateButtonWithAccName(
+        IDS_ZOOM_PLUS2, MenuButtonBackground::RIGHT_BUTTON, increment_index,
+        NULL, IDS_ACCNAME_ZOOM_PLUS2);
 
     center_bg->SetOtherButtons(decrement_button_, increment_button_);
 
@@ -401,7 +429,8 @@ class WrenchMenu::ZoomView : public ScheduleAllView,
     fullscreen_button_->set_background(
         new MenuButtonBackground(MenuButtonBackground::SINGLE_BUTTON));
     fullscreen_button_->SetAccessibleName(
-        l10n_util::GetString(IDS_ACCNAME_FULLSCREEN));
+        GetAccessibleNameForWrenchMenuItem(
+            menu_model, fullscreen_index, IDS_ACCNAME_FULLSCREEN));
     AddChildView(fullscreen_button_);
 
     UpdateZoomControls();
@@ -506,12 +535,6 @@ class WrenchMenu::ZoomView : public ScheduleAllView,
     return max_w + insets.width();
   }
 
-  // Hosting WrenchMenu.
-  WrenchMenu* menu_;
-
-  // The menu model containing the increment/decrement/reset items.
-  MenuModel* menu_model_;
-
   // Index of the fullscreen menu item in the model.
   const int fullscreen_index_;
 
diff --git a/views/controls/button/custom_button.cc b/views/controls/button/custom_button.cc
index 4ab2fae..a1f4e62 100644
--- a/views/controls/button/custom_button.cc
+++ b/views/controls/button/custom_button.cc
@@ -58,10 +58,13 @@ AccessibilityTypes::State CustomButton::GetAccessibleState() {
   switch (state_) {
     case BS_HOT:
       state = AccessibilityTypes::STATE_HOTTRACKED;
+      break;
     case BS_PUSHED:
       state = AccessibilityTypes::STATE_PRESSED;
+      break;
     case BS_DISABLED:
       state = AccessibilityTypes::STATE_UNAVAILABLE;
+      break;
     case BS_NORMAL:
     case BS_COUNT:
       // No additional accessibility state set for this button state.
diff --git a/views/controls/menu/menu_item_view.cc b/views/controls/menu/menu_item_view.cc
index c903a0f..bd1fbaf 100644
--- a/views/controls/menu/menu_item_view.cc
+++ b/views/controls/menu/menu_item_view.cc
@@ -108,10 +108,12 @@ AccessibilityTypes::State MenuItemView::GetAccessibleState() {
   switch (GetType()) {
     case SUBMENU:
       state |= AccessibilityTypes::STATE_HASPOPUP;
+      break;
     case CHECKBOX:
     case RADIO:
       state |= GetDelegate()->IsItemChecked(GetCommand()) ?
           AccessibilityTypes::STATE_CHECKED : 0;
+      break;
     case NORMAL:
     case SEPARATOR:
       // No additional accessibility states currently for these menu states.
@@ -120,6 +122,33 @@ AccessibilityTypes::State MenuItemView::GetAccessibleState() {
   return state;
 }
 
+// static
+std::wstring MenuItemView::GetAccessibleNameForMenuItem(
+      const std::wstring& item_text, const std::wstring& accelerator_text) {
+  std::wstring accessible_name = item_text;
+
+  // Filter out the "&" for accessibility clients.
+  size_t index = 0;
+  while ((index = accessible_name.find(L"&", index)) != std::wstring::npos &&
+         index + 1 < accessible_name.length()) {
+    accessible_name.replace(index, accessible_name.length() - index,
+                            accessible_name.substr(index + 1));
+
+    // Special case for "&&" (escaped for "&").
+    if (accessible_name[index] == '&')
+      ++index;
+  }
+
+  // Append accelerator text.
+  menus::Accelerator menu_accelerator;
+  if (!accelerator_text.empty()) {
+    accessible_name.append(L" ");
+    accessible_name.append(accelerator_text);
+  }
+
+  return accessible_name;
+}
+
 void MenuItemView::RunMenuAt(gfx::NativeWindow parent,
                              MenuButton* button,
                              const gfx::Rect& bounds,
@@ -237,27 +266,7 @@ SubmenuView* MenuItemView::CreateSubmenu() {
 
 void MenuItemView::SetTitle(const std::wstring& title) {
   title_ = title;
-  std::wstring accessible_title(title);
-
-  // Filter out the "&" for accessibility clients.
-  size_t index = 0;
-  while ((index = accessible_title.find(L"&", index)) != std::wstring::npos &&
-         index + 1 < accessible_title.length()) {
-    accessible_title.replace(index, accessible_title.length() - index,
-                             accessible_title.substr(index + 1));
-
-    // Special case for "&&" (escaped for "&").
-    if (accessible_title[index] == '&')
-      ++index;
-  }
-
-  if (!GetAcceleratorText().empty()) {
-    std::wstring accelerator_text(L" ");
-    accelerator_text.append(GetAcceleratorText());
-    accessible_title.append(accelerator_text);
-  }
-
-  SetAccessibleName(accessible_title);
+  SetAccessibleName(GetAccessibleNameForMenuItem(title, GetAcceleratorText()));
 }
 
 void MenuItemView::SetSelected(bool selected) {
diff --git a/views/controls/menu/menu_item_view.h b/views/controls/menu/menu_item_view.h
index 0430e34..66540b7 100644
--- a/views/controls/menu/menu_item_view.h
+++ b/views/controls/menu/menu_item_view.h
@@ -98,6 +98,11 @@ class MenuItemView : public View {
   // X-coordinate of where the label starts.
   static int label_start() { return label_start_; }
 
+  // Returns the accessible name to be used with screen readers. Mnemonics are
+  // removed and the menu item accelerator text is appended.
+  static std::wstring GetAccessibleNameForMenuItem(
+      const std::wstring& item_text, const std::wstring& accelerator_text);
+
   // Run methods. See description above class for details. Both Run methods take
   // a rectangle, which is used to position the menu. |has_mnemonics| indicates
   // whether the items have mnemonics. Mnemonics are identified by way of the