From 558ef7352d314f681215739f7458149eef5e9cd8 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Sat, 17 Mar 2018 06:37:36 +0900 Subject: [PATCH] Better GTK+ Menu color support (#12300) * Better GTK+ Menu color support * Fix 'invisible menu' issue (#12275) * Now updates menu text color when focus changes! * Better caching of colors when system theme changes * Removed all GTK+ deprecation warnings from menubar * Don't highlight menu text on mouseover in GTK+ * Fix textColor declaration scope error * Simplify FocusManager connection management a bit * Make the linter happy * Decouple MenuBar view recoloring from rebuilding This way we don't need to rebuild the subview each time a recolor is needed, e.g. when window focus changes or the system theme changes * Don't iterate child views if we don't need to * Move variable declaration outside of a loop * More efficient iteration of MenuBar children * Cleaner MenuButton bounds testing * Fix oops * Add a nullptr check in MenuBar::GetItemCount() * Simplify iteration in MenuBar::RebuildChildren() * Make the linter happy * Fix signed-unsigned comparison * Remove declarations of nonexistent methods * Make SubmenuButton accessor const * Cleaner accelerator iteration * Windows fixes --- atom/browser/native_window_views.cc | 9 +- atom/browser/ui/views/menu_bar.cc | 192 ++++++++++++------------ atom/browser/ui/views/menu_bar.h | 29 +++- atom/browser/ui/views/submenu_button.cc | 4 +- atom/browser/ui/views/submenu_button.h | 7 +- 5 files changed, 126 insertions(+), 115 deletions(-) diff --git a/atom/browser/native_window_views.cc b/atom/browser/native_window_views.cc index 2231c4f448..c40cc025c0 100644 --- a/atom/browser/native_window_views.cc +++ b/atom/browser/native_window_views.cc @@ -1337,7 +1337,7 @@ void NativeWindowViews::HandleKeyboardEvent( if (event.GetType() == blink::WebInputEvent::kRawKeyDown && !IsAltKey(event) && IsAltModifier(event)) { if (!menu_bar_visible_ && - (menu_bar_->GetAcceleratorIndex(event.windows_key_code) != -1)) + (menu_bar_->HasAccelerator(event.windows_key_code))) SetMenuBarVisibility(true); menu_bar_->ActivateAccelerator(event.windows_key_code); return; @@ -1445,12 +1445,9 @@ void NativeWindowViews::RegisterAccelerators(AtomMenuModel* menu_model) { // Register accelerators with focus manager. accelerator_util::GenerateAcceleratorTable(&accelerator_table_, menu_model); - accelerator_util::AcceleratorTable::const_iterator iter; - for (iter = accelerator_table_.begin(); - iter != accelerator_table_.end(); - ++iter) { + for (const auto& iter : accelerator_table_) { focus_manager->RegisterAccelerator( - iter->first, ui::AcceleratorManager::kNormalPriority, this); + iter.first, ui::AcceleratorManager::kNormalPriority, this); } } diff --git a/atom/browser/ui/views/menu_bar.cc b/atom/browser/ui/views/menu_bar.cc index bf9801a145..d963c121bb 100644 --- a/atom/browser/ui/views/menu_bar.cc +++ b/atom/browser/ui/views/menu_bar.cc @@ -4,10 +4,6 @@ #include "atom/browser/ui/views/menu_bar.h" -#if defined(USE_X11) -#include "gtk/gtk.h" -#endif - #include "atom/browser/ui/views/menu_delegate.h" #include "atom/browser/ui/views/submenu_button.h" #include "ui/base/models/menu_model.h" @@ -16,52 +12,12 @@ #if defined(OS_WIN) #include "ui/gfx/color_utils.h" -#elif defined(USE_X11) -#include "chrome/browser/ui/libgtkui/skia_utils_gtk.h" #endif namespace atom { namespace { -#if defined(USE_X11) - -SkColor GdkRgbaToSkColor(const GdkRGBA& rgba) { - return SkColorSetARGB(rgba.alpha * 255, rgba.red * 255, rgba.green * 255, - rgba.blue * 255); -} - -SkColor GetStyleContextFgColor(GtkStyleContext* style_context, - GtkStateFlags state) { - GdkRGBA rgba; - gtk_style_context_get_color(style_context, state, &rgba); - return GdkRgbaToSkColor(rgba); -} - -SkColor GetStyleContextBgColor(GtkStyleContext* style_context, - GtkStateFlags state) { - GdkRGBA rgba; - gtk_style_context_get_background_color(style_context, state, &rgba); - return GdkRgbaToSkColor(rgba); -} - -void GetMenuBarColor(SkColor* enabled, - SkColor* disabled, - SkColor* highlight, - SkColor* hover, - SkColor* background) { - GtkWidget* menu_bar = gtk_menu_bar_new(); - GtkStyleContext* sc = gtk_widget_get_style_context(menu_bar); - *enabled = GetStyleContextFgColor(sc, GTK_STATE_FLAG_NORMAL); - *disabled = GetStyleContextFgColor(sc, GTK_STATE_FLAG_INSENSITIVE); - *highlight = GetStyleContextFgColor(sc, GTK_STATE_FLAG_SELECTED); - *hover = GetStyleContextFgColor(sc, GTK_STATE_FLAG_PRELIGHT); - *background = GetStyleContextBgColor(sc, GTK_STATE_FLAG_NORMAL); - gtk_widget_destroy(GTK_WIDGET(menu_bar)); -} - -#endif // USE_X11 - const char kViewClassName[] = "ElectronMenuBar"; // Default color of the menu bar. @@ -71,75 +27,70 @@ const SkColor kDefaultColor = SkColorSetARGB(255, 233, 233, 233); MenuBar::MenuBar(NativeWindow* window) : background_color_(kDefaultColor), menu_model_(NULL), window_(window) { - UpdateMenuBarColor(); + RefreshColorCache(); + UpdateViewColors(); SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal)); } MenuBar::~MenuBar() {} +void MenuBar::AddedToWidget() { + auto fm = GetFocusManager(); + fm->AddFocusChangeListener(this); + // Note that we don't own fm -- this manages the _connection_ + focus_manager_.reset(fm, [this](views::FocusManager* fm) { + fm->RemoveFocusChangeListener(this); + }); +} + +void MenuBar::RemovedFromWidget() { + focus_manager_.reset(); +} + void MenuBar::SetMenu(AtomMenuModel* model) { menu_model_ = model; - RemoveAllChildViews(true); - - for (int i = 0; i < model->GetItemCount(); ++i) { - SubmenuButton* button = - new SubmenuButton(model->GetLabelAt(i), this, background_color_); - button->set_tag(i); - -#if defined(USE_X11) - button->SetTextColor(views::Button::STATE_NORMAL, enabled_color_); - button->SetTextColor(views::Button::STATE_DISABLED, disabled_color_); - button->SetTextColor(views::Button::STATE_PRESSED, highlight_color_); - button->SetTextColor(views::Button::STATE_HOVERED, hover_color_); - button->SetUnderlineColor(enabled_color_); -#elif defined(OS_WIN) - button->SetUnderlineColor(color_utils::GetSysSkColor(COLOR_GRAYTEXT)); -#endif - - AddChildView(button); - } + RebuildChildren(); } void MenuBar::SetAcceleratorVisibility(bool visible) { - for (int i = 0; i < child_count(); ++i) - static_cast(child_at(i))->SetAcceleratorVisibility(visible); + for (auto* child : GetChildrenInZOrder()) + static_cast(child)->SetAcceleratorVisibility(visible); } -int MenuBar::GetAcceleratorIndex(base::char16 key) { - for (int i = 0; i < child_count(); ++i) { - SubmenuButton* button = static_cast(child_at(i)); - if (button->accelerator() == key) - return i; +MenuBar::View* MenuBar::FindAccelChild(base::char16 key) { + for (auto* child : GetChildrenInZOrder()) { + if (static_cast(child)->accelerator() == key) + return child; } - return -1; + return nullptr; +} + +bool MenuBar::HasAccelerator(base::char16 key) { + return FindAccelChild(key) != nullptr; } void MenuBar::ActivateAccelerator(base::char16 key) { - int i = GetAcceleratorIndex(key); - if (i != -1) - static_cast(child_at(i))->Activate(nullptr); + auto child = FindAccelChild(key); + if (child) + static_cast(child)->Activate(nullptr); } int MenuBar::GetItemCount() const { - return menu_model_->GetItemCount(); + return menu_model_ ? menu_model_->GetItemCount() : 0; } -bool MenuBar::GetMenuButtonFromScreenPoint(const gfx::Point& point, +bool MenuBar::GetMenuButtonFromScreenPoint(const gfx::Point& screenPoint, AtomMenuModel** menu_model, views::MenuButton** button) { - gfx::Point location(point); - views::View::ConvertPointFromScreen(this, &location); - - if (location.x() < 0 || location.x() >= width() || location.y() < 0 || - location.y() >= height()) + if (!GetBoundsInScreen().Contains(screenPoint)) return false; - for (int i = 0; i < child_count(); ++i) { - views::View* view = child_at(i); - if (view->GetMirroredBounds().Contains(location) && + auto children = GetChildrenInZOrder(); + for (int i = 0, n = children.size(); i < n; ++i) { + if (children[i]->GetBoundsInScreen().Contains(screenPoint) && (menu_model_->GetTypeAt(i) == AtomMenuModel::TYPE_SUBMENU)) { *menu_model = menu_model_->GetSubmenuModelAt(i); - *button = static_cast(view); + *button = static_cast(children[i]); return true; } } @@ -175,18 +126,71 @@ void MenuBar::OnMenuButtonClicked(views::MenuButton* source, menu_delegate->RunMenu(menu_model_->GetSubmenuModelAt(id), source); } -void MenuBar::OnNativeThemeChanged(const ui::NativeTheme* theme) { - UpdateMenuBarColor(); -} - -void MenuBar::UpdateMenuBarColor() { +void MenuBar::RefreshColorCache(const ui::NativeTheme* theme) { + if (!theme) + theme = ui::NativeTheme::GetInstanceForNativeUi(); + if (theme) { + background_color_ = + theme->GetSystemColor(ui::NativeTheme::kColorId_MenuBackgroundColor); +#if defined(USE_X11) + enabled_color_ = theme->GetSystemColor( + ui::NativeTheme::kColorId_EnabledMenuItemForegroundColor); + disabled_color_ = theme->GetSystemColor( + ui::NativeTheme::kColorId_DisabledMenuItemForegroundColor); +#endif + } #if defined(OS_WIN) background_color_ = color_utils::GetSysSkColor(COLOR_MENUBAR); -#elif defined(USE_X11) - GetMenuBarColor(&enabled_color_, &disabled_color_, &highlight_color_, - &hover_color_, &background_color_); #endif +} + +void MenuBar::OnNativeThemeChanged(const ui::NativeTheme* theme) { + RefreshColorCache(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) { + auto button = + new SubmenuButton(menu_model_->GetLabelAt(i), this, background_color_); + button->set_tag(i); + AddChildView(button); + } + UpdateViewColors(); +} + +void MenuBar::UpdateViewColors() { + // set menubar background color SetBackground(views::CreateSolidBackground(background_color_)); + + // set child colors + if (menu_model_ == nullptr) + return; +#if defined(USE_X11) + const auto& textColor = has_focus_ ? enabled_color_ : disabled_color_; + for (auto* child : GetChildrenInZOrder()) { + auto button = static_cast(child); + button->SetTextColor(views::Button::STATE_NORMAL, textColor); + button->SetTextColor(views::Button::STATE_DISABLED, disabled_color_); + button->SetTextColor(views::Button::STATE_PRESSED, enabled_color_); + button->SetTextColor(views::Button::STATE_HOVERED, textColor); + button->SetUnderlineColor(textColor); + } +#elif defined(OS_WIN) + for (auto* child : GetChildrenInZOrder()) { + auto button = static_cast(child); + button->SetUnderlineColor(color_utils::GetSysSkColor(COLOR_MENUTEXT)); + } +#endif } } // namespace atom diff --git a/atom/browser/ui/views/menu_bar.h b/atom/browser/ui/views/menu_bar.h index 761e31b98e..13cfa32f39 100644 --- a/atom/browser/ui/views/menu_bar.h +++ b/atom/browser/ui/views/menu_bar.h @@ -5,9 +5,12 @@ #ifndef ATOM_BROWSER_UI_VIEWS_MENU_BAR_H_ #define ATOM_BROWSER_UI_VIEWS_MENU_BAR_H_ +#include + #include "atom/browser/native_window.h" #include "atom/browser/ui/atom_menu_model.h" #include "ui/views/controls/button/menu_button_listener.h" +#include "ui/views/focus/focus_manager.h" #include "ui/views/view.h" namespace views { @@ -19,7 +22,8 @@ namespace atom { class MenuDelegate; class MenuBar : public views::View, - public views::MenuButtonListener { + public views::MenuButtonListener, + public views::FocusChangeListener { public: explicit MenuBar(NativeWindow* window); virtual ~MenuBar(); @@ -30,9 +34,8 @@ class MenuBar : public views::View, // Shows underline under accelerators. void SetAcceleratorVisibility(bool visible); - // Returns which submenu has accelerator |key|, -1 would be returned when - // there is no matching submenu. - int GetAcceleratorIndex(base::char16 key); + // Returns true if the submenu has accelerator |key| + bool HasAccelerator(base::char16 key); // Shows the submenu whose accelerator is |key|. void ActivateAccelerator(base::char16 key); @@ -47,7 +50,9 @@ class MenuBar : public views::View, protected: // views::View: + void AddedToWidget() override; const char* GetClassName() const override; + void RemovedFromWidget() override; // views::MenuButtonListener: void OnMenuButtonClicked(views::MenuButton* source, @@ -55,21 +60,29 @@ 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: - void UpdateMenuBarColor(); + void RebuildChildren(); + void UpdateViewColors(); + void RefreshColorCache(const ui::NativeTheme* theme = nullptr); SkColor background_color_; - #if defined(USE_X11) SkColor enabled_color_; SkColor disabled_color_; - SkColor highlight_color_; - SkColor hover_color_; #endif NativeWindow* window_; AtomMenuModel* menu_model_; + View* FindAccelChild(base::char16 key); + + std::shared_ptr focus_manager_; + bool has_focus_ = true; + DISALLOW_COPY_AND_ASSIGN(MenuBar); }; diff --git a/atom/browser/ui/views/submenu_button.cc b/atom/browser/ui/views/submenu_button.cc index 5ae2aae1ce..b3116706bc 100644 --- a/atom/browser/ui/views/submenu_button.cc +++ b/atom/browser/ui/views/submenu_button.cc @@ -91,7 +91,7 @@ void SubmenuButton::PaintButtonContents(gfx::Canvas* canvas) { bool SubmenuButton::GetUnderlinePosition(const base::string16& text, base::char16* accelerator, - int* start, int* end) { + int* start, int* end) const { int pos, span; base::string16 trimmed = gfx::RemoveAcceleratorChar(text, '&', &pos, &span); if (pos > -1 && span != 0) { @@ -105,7 +105,7 @@ bool SubmenuButton::GetUnderlinePosition(const base::string16& text, } void SubmenuButton::GetCharacterPosition( - const base::string16& text, int index, int* pos) { + const base::string16& text, int index, int* pos) const { int height = 0; gfx::Canvas::SizeStringInt(text.substr(0, index), gfx::FontList(), pos, &height, 0, 0); diff --git a/atom/browser/ui/views/submenu_button.h b/atom/browser/ui/views/submenu_button.h index 5c05481cbb..0f7eddf4c9 100644 --- a/atom/browser/ui/views/submenu_button.h +++ b/atom/browser/ui/views/submenu_button.h @@ -21,9 +21,6 @@ class SubmenuButton : public views::MenuButton { void SetAcceleratorVisibility(bool visible); void SetUnderlineColor(SkColor color); - void SetEnabledColor(SkColor color); - void SetBackgroundColor(SkColor color); - base::char16 accelerator() const { return accelerator_; } // views::MenuButton: @@ -36,9 +33,9 @@ class SubmenuButton : public views::MenuButton { private: bool GetUnderlinePosition(const base::string16& text, base::char16* accelerator, - int* start, int* end); + int* start, int* end) const; void GetCharacterPosition( - const base::string16& text, int index, int* pos); + const base::string16& text, int index, int* pos) const; base::char16 accelerator_;