refactor: Improve accessibility of menus

atom/browser/ui/views/menu_bar.cc

@@ -5,13 +5,16 @@
 #include "atom/browser/ui/views/menu_bar.h"
 
 #include <memory>
+#include <set>
+#include <sstream>
 #include <string>
 
-#include "atom/browser/ui/views/menu_delegate.h"
 #include "atom/browser/ui/views/submenu_button.h"
+#include "atom/common/keyboard_util.h"
 #include "ui/base/models/menu_model.h"
 #include "ui/views/background.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/widget/widget.h"
 
 #if defined(USE_X11)
 #include "chrome/browser/ui/libgtkui/gtk_util.h"
@@ -32,17 +35,36 @@ const SkColor kDefaultColor = SkColorSetARGB(255, 233, 233, 233);
 
 const char MenuBar::kViewClassName[] = "ElectronMenuBar";
 
-MenuBar::MenuBar(views::View* window)
-    : background_color_(kDefaultColor), window_(window) {
+MenuBarColorUpdater::MenuBarColorUpdater(MenuBar* menu_bar)
+    : menu_bar_(menu_bar) {}
+
+MenuBarColorUpdater::~MenuBarColorUpdater() {}
+
+void MenuBarColorUpdater::OnDidChangeFocus(views::View* focused_before,
+                                           views::View* focused_now) {
+  if (menu_bar_) {
+    // if we've changed window focus, update menu bar colors
+    const auto had_focus = menu_bar_->has_focus_;
+    menu_bar_->has_focus_ = focused_now != nullptr;
+    if (menu_bar_->has_focus_ != had_focus)
+      menu_bar_->UpdateViewColors();
+  }
+}
+
+MenuBar::MenuBar(RootView* window)
+    : background_color_(kDefaultColor),
+      window_(window),
+      color_updater_(new MenuBarColorUpdater(this)) {
   RefreshColorCache();
   UpdateViewColors();
+  SetFocusBehavior(FocusBehavior::ALWAYS);
   SetLayoutManager(
       std::make_unique<views::BoxLayout>(views::BoxLayout::kHorizontal));
-  window_->GetFocusManager()->AddFocusChangeListener(this);
+  window_->GetFocusManager()->AddFocusChangeListener(color_updater_.get());
 }
 
 MenuBar::~MenuBar() {
-  window_->GetFocusManager()->RemoveFocusChangeListener(this);
+  window_->GetFocusManager()->RemoveFocusChangeListener(color_updater_.get());
 }
 
 void MenuBar::SetMenu(AtomMenuModel* model) {
@@ -96,6 +118,116 @@ bool MenuBar::GetMenuButtonFromScreenPoint(const gfx::Point& screenPoint,
   return false;
 }
 
+void MenuBar::OnBeforeExecuteCommand() {
+  RemovePaneFocus();
+  window_->RestoreFocus();
+}
+
+void MenuBar::OnMenuClosed() {
+  SetAcceleratorVisibility(true);
+}
+
+bool MenuBar::AcceleratorPressed(const ui::Accelerator& accelerator) {
+  views::View* focused_view = GetFocusManager()->GetFocusedView();
+  if (!ContainsForFocusSearch(this, focused_view))
+    return false;
+
+  switch (accelerator.key_code()) {
+    case ui::VKEY_MENU:
+    case ui::VKEY_ESCAPE: {
+      RemovePaneFocus();
+      window_->RestoreFocus();
+      return true;
+    }
+    case ui::VKEY_LEFT:
+      GetFocusManager()->AdvanceFocus(true);
+      return true;
+    case ui::VKEY_RIGHT:
+      GetFocusManager()->AdvanceFocus(false);
+      return true;
+    case ui::VKEY_HOME:
+      GetFocusManager()->SetFocusedViewWithReason(
+          GetFirstFocusableChild(), views::FocusManager::kReasonFocusTraversal);
+      return true;
+    case ui::VKEY_END:
+      GetFocusManager()->SetFocusedViewWithReason(
+          GetLastFocusableChild(), views::FocusManager::kReasonFocusTraversal);
+      return true;
+    default: {
+      auto children = GetChildrenInZOrder();
+      for (int i = 0, n = children.size(); i < n; ++i) {
+        auto* button = static_cast<SubmenuButton*>(children[i]);
+        bool shifted = false;
+        auto keycode =
+            atom::KeyboardCodeFromCharCode(button->accelerator(), &shifted);
+
+        if (keycode == accelerator.key_code()) {
+          const gfx::Point p(0, 0);
+          auto event = accelerator.ToKeyEvent();
+          OnMenuButtonClicked(button, p, &event);
+          return true;
+        }
+      }
+
+      return false;
+    }
+  }
+}
+
+bool MenuBar::SetPaneFocus(views::View* initial_focus) {
+  bool result = views::AccessiblePaneView::SetPaneFocus(initial_focus);
+
+  if (result) {
+    auto children = GetChildrenInZOrder();
+    std::set<ui::KeyboardCode> reg;
+    for (int i = 0, n = children.size(); i < n; ++i) {
+      auto* button = static_cast<SubmenuButton*>(children[i]);
+      bool shifted = false;
+      auto keycode =
+          atom::KeyboardCodeFromCharCode(button->accelerator(), &shifted);
+
+      // We want the menu items to activate if the user presses the accelerator
+      // key, even without alt, since we are now focused on the menu bar
+      if (keycode != ui::VKEY_UNKNOWN && reg.find(keycode) != reg.end()) {
+        reg.insert(keycode);
+        focus_manager()->RegisterAccelerator(
+            ui::Accelerator(keycode, ui::EF_NONE),
+            ui::AcceleratorManager::kNormalPriority, this);
+      }
+    }
+
+    // We want to remove focus / hide menu bar when alt is pressed again
+    focus_manager()->RegisterAccelerator(
+        ui::Accelerator(ui::VKEY_MENU, ui::EF_ALT_DOWN),
+        ui::AcceleratorManager::kNormalPriority, this);
+  }
+
+  return result;
+}
+
+void MenuBar::RemovePaneFocus() {
+  views::AccessiblePaneView::RemovePaneFocus();
+  SetAcceleratorVisibility(false);
+
+  auto children = GetChildrenInZOrder();
+  std::set<ui::KeyboardCode> unreg;
+  for (int i = 0, n = children.size(); i < n; ++i) {
+    auto* button = static_cast<SubmenuButton*>(children[i]);
+    bool shifted = false;
+    auto keycode =
+        atom::KeyboardCodeFromCharCode(button->accelerator(), &shifted);
+
+    if (keycode != ui::VKEY_UNKNOWN && unreg.find(keycode) != unreg.end()) {
+      unreg.insert(keycode);
+      focus_manager()->UnregisterAccelerator(
+          ui::Accelerator(keycode, ui::EF_NONE), this);
+    }
+  }
+
+  focus_manager()->UnregisterAccelerator(
+      ui::Accelerator(ui::VKEY_MENU, ui::EF_ALT_DOWN), this);
+}
+
 const char* MenuBar::GetClassName() const {
   return kViewClassName;
 }
@@ -119,9 +251,16 @@ void MenuBar::OnMenuButtonClicked(views::MenuButton* source,
     return;
   }
 
+  GetFocusManager()->SetFocusedViewWithReason(
+      source, views::FocusManager::kReasonFocusTraversal);
+
   // Deleted in MenuDelegate::OnMenuClosed
   MenuDelegate* menu_delegate = new MenuDelegate(this);
-  menu_delegate->RunMenu(menu_model_->GetSubmenuModelAt(id), source);
+  menu_delegate->RunMenu(menu_model_->GetSubmenuModelAt(id), source,
+                         event != nullptr && event->IsKeyEvent()
+                             ? ui::MENU_SOURCE_KEYBOARD
+                             : ui::MENU_SOURCE_MOUSE);
+  menu_delegate->AddObserver(this);
 }
 
 void MenuBar::RefreshColorCache(const ui::NativeTheme* theme) {
@@ -151,14 +290,6 @@ void MenuBar::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   UpdateViewColors();
 }
 
-void MenuBar::OnDidChangeFocus(View* focused_before, View* focused_now) {
-  // if we've changed focus, update our view
-  const auto had_focus = has_focus_;
-  has_focus_ = focused_now != nullptr;
-  if (has_focus_ != had_focus)
-    UpdateViewColors();
-}
-
 void MenuBar::RebuildChildren() {
   RemoveAllChildViews(true);
   for (int i = 0, n = GetItemCount(); i < n; ++i) {

atom/browser/ui/views/menu_bar.h

@@ -5,7 +5,12 @@
 #ifndef ATOM_BROWSER_UI_VIEWS_MENU_BAR_H_
 #define ATOM_BROWSER_UI_VIEWS_MENU_BAR_H_
 
+#include <memory>
+
 #include "atom/browser/ui/atom_menu_model.h"
+#include "atom/browser/ui/views/menu_delegate.h"
+#include "atom/browser/ui/views/root_view.h"
+#include "ui/views/accessible_pane_view.h"
 #include "ui/views/controls/button/menu_button_listener.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/view.h"
@@ -16,15 +21,27 @@ class MenuButton;
 
 namespace atom {
 
-class MenuDelegate;
+class MenuBarColorUpdater : public views::FocusChangeListener {
+ public:
+  explicit MenuBarColorUpdater(MenuBar* menu_bar);
+  ~MenuBarColorUpdater() override;
+
+  void OnDidChangeFocus(views::View* focused_before,
+                        views::View* focused_now) override;
+  void OnWillChangeFocus(views::View* focused_before,
+                         views::View* focused_now) override {}
+
+ private:
+  MenuBar* menu_bar_;
+};
 
-class MenuBar : public views::View,
+class MenuBar : public views::AccessiblePaneView,
                 public views::MenuButtonListener,
-                public views::FocusChangeListener {
+                public atom::MenuDelegate::Observer {
  public:
   static const char kViewClassName[];
 
-  explicit MenuBar(views::View* window);
+  explicit MenuBar(RootView* window);
   ~MenuBar() override;
 
   // Replaces current menu with a new one.
@@ -47,6 +64,15 @@ class MenuBar : public views::View,
                                     AtomMenuModel** menu_model,
                                     views::MenuButton** button);
 
+  // atom::MenuDelegate::Observer:
+  void OnBeforeExecuteCommand() override;
+  void OnMenuClosed() override;
+
+  // views::AccessiblePaneView:
+  bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
+  bool SetPaneFocus(views::View* initial_focus) override;
+  void RemovePaneFocus() override;
+
  protected:
   // views::View:
   const char* GetClassName() const override;
@@ -57,11 +83,9 @@ class MenuBar : public views::View,
                            const ui::Event* event) override;
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override;
 
-  // views::FocusChangeListener:
-  void OnDidChangeFocus(View* focused_before, View* focused_now) override;
-  void OnWillChangeFocus(View* focused_before, View* focused_now) override {}
-
  private:
+  friend class MenuBarColorUpdater;
+
   void RebuildChildren();
   void UpdateViewColors();
 
@@ -72,13 +96,15 @@ class MenuBar : public views::View,
   SkColor disabled_color_;
 #endif
 
-  views::View* window_ = nullptr;
+  RootView* window_ = nullptr;
   AtomMenuModel* menu_model_ = nullptr;
 
   View* FindAccelChild(base::char16 key);
 
   bool has_focus_ = true;
 
+  std::unique_ptr<MenuBarColorUpdater> color_updater_;
+
   DISALLOW_COPY_AND_ASSIGN(MenuBar);
 };
 

atom/browser/ui/views/menu_delegate.cc

@@ -14,17 +14,24 @@
 
 namespace atom {
 
-MenuDelegate::MenuDelegate(MenuBar* menu_bar) : menu_bar_(menu_bar), id_(-1) {}
+MenuDelegate::MenuDelegate(MenuBar* menu_bar)
+    : menu_bar_(menu_bar), id_(-1), hold_first_switch_(false) {}
 
 MenuDelegate::~MenuDelegate() {}
 
-void MenuDelegate::RunMenu(AtomMenuModel* model, views::MenuButton* button) {
+void MenuDelegate::RunMenu(AtomMenuModel* model,
+                           views::MenuButton* button,
+                           ui::MenuSourceType source_type) {
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(button, &screen_loc);
   // Subtract 1 from the height to make the popup flush with the button border.
   gfx::Rect bounds(screen_loc.x(), screen_loc.y(), button->width(),
                    button->height() - 1);
 
+  if (source_type == ui::MENU_SOURCE_KEYBOARD) {
+    hold_first_switch_ = true;
+  }
+
   id_ = button->tag();
   adapter_.reset(new MenuModelAdapter(model));
 
@@ -35,15 +42,18 @@ void MenuDelegate::RunMenu(AtomMenuModel* model, views::MenuButton* button) {
       item,
       views::MenuRunner::CONTEXT_MENU | views::MenuRunner::HAS_MNEMONICS));
   menu_runner_->RunMenuAt(button->GetWidget()->GetTopLevelWidget(), button,
-                          bounds, views::MENU_ANCHOR_TOPRIGHT,
-                          ui::MENU_SOURCE_MOUSE);
+                          bounds, views::MENU_ANCHOR_TOPRIGHT, source_type);
 }
 
 void MenuDelegate::ExecuteCommand(int id) {
+  for (Observer& obs : observers_)
+    obs.OnBeforeExecuteCommand();
   adapter_->ExecuteCommand(id);
 }
 
 void MenuDelegate::ExecuteCommand(int id, int mouse_event_flags) {
+  for (Observer& obs : observers_)
+    obs.OnBeforeExecuteCommand();
   adapter_->ExecuteCommand(id, mouse_event_flags);
 }
 
@@ -89,6 +99,9 @@ void MenuDelegate::WillHideMenu(views::MenuItemView* menu) {
 }
 
 void MenuDelegate::OnMenuClosed(views::MenuItemView* menu) {
+  for (Observer& obs : observers_)
+    obs.OnMenuClosed();
+
   // Only switch to new menu when current menu is closed.
   if (button_to_open_)
     button_to_open_->Activate(nullptr);
@@ -101,6 +114,11 @@ views::MenuItemView* MenuDelegate::GetSiblingMenu(
     views::MenuAnchorPosition* anchor,
     bool* has_mnemonics,
     views::MenuButton**) {
+  if (hold_first_switch_) {
+    hold_first_switch_ = false;
+    return nullptr;
+  }
+
   // TODO(zcbenz): We should follow Chromium's logics on implementing the
   // sibling menu switches, this code is almost a hack.
   views::MenuButton* button;