Skip to content

Commit

Permalink
refactor: Improve accessibility of menus (#15302)
Browse files Browse the repository at this point in the history 
* refactor: improve menubar keyboard accessibility

* fix: create a temporary widget for tray icon context menu

* fix: focus menu bar with Alt when autohide is off

* fix: make menu bar focus work more like the native menus

* fix: only focus menu bar if it's not already focused

* fix: track accelerator registration to avoid duplicates

* docs: add docs for & notation in app menu item names

* fix: only try to activate accelerator if it's registered

* fix: add friend to monitor window focus change

* style: add <memory> include
  • Loading branch information
@brenca @ckerr
brenca authored and ckerr committed Oct 29, 2018
1 parent 00daff6 commit 894ae1b
Show file tree
Hide file tree
Showing 13 changed files with 405 additions and 140 deletions.
159 changes: 145 additions & 14 deletions atom/browser/ui/views/menu_bar.cc
View file
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
44 changes: 35 additions & 9 deletions atom/browser/ui/views/menu_bar.h
View file
Expand Up @@ -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"
Expand All @@ -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.
Expand All @@ -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;
Expand All @@ -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();

Expand All @@ -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);
};

Expand Down
26 changes: 22 additions & 4 deletions atom/browser/ui/views/menu_delegate.cc
View file
Expand Up @@ -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));

Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down

0 comments on commit 894ae1b

Please sign in to comment.