Skip to content

Commit

Permalink
desktop-sh: use menu item colors for share hub action buttons
Browse files Browse the repository at this point in the history
Previously, these entries were generic HoverButtons, but they are
presented in a menu-like container and have other menu behaviors like
arrow keys traversing them. This change gives them the background and
foreground color of menu items instead.

There is one remaining visual oddity in high contrast: vector icons for
1P actions draw in the normal menu foreground color. I investigated a
bit, and all other menus in Chrome also seem to have this behavior,
which is odd but I guess probably what users are used to. In any case,
there's no way to recolor the 3P icons, so we might as well leave the 1P
ones as they are too.

Fixed: 1234478
Change-Id: I8787cb00e6c49170fe422da2a5e3f6bab9f46fb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3126176
Reviewed-by: Kristi Park <kristipark@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#916507}
  • Loading branch information
Elly Fong-Jones authored and Chromium LUCI CQ committed Aug 30, 2021
1 parent 5e23ac9 commit 29fceb6
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/base/models/image_model.h"
#include "ui/views/animation/ink_drop.h"
#include "ui/views/border.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/controls/styled_label.h"

namespace sharing_hub {

Expand Down Expand Up @@ -63,10 +65,37 @@ SharingHubBubbleActionButton::SharingHubBubbleActionButton(
SetAccessibleName(l10n_util::GetStringFUTF16(
IDS_SHARING_HUB_SHARE_LABEL_ACCESSIBILITY, action_info.title));
}

// This class wants to pretend to be a menu item visually, so it does its own
// hover effects by overriding LabelButton::UpdateBackgroundColor (below). It
// isn't sufficient to simply swap out the ink drop color - menu backgrounds
// are drawn below the text/icon but the ink drop would be drawn above them,
// which looks wrong.
// TODO(ellyjones): This removes ~all the benefit of being a HoverButton -
// should this class instead subclass LabelButton?
views::InkDrop::Get(this)->SetMode(views::InkDropHost::InkDropMode::OFF);
title()->SetTextContext(views::style::CONTEXT_MENU);
}

SharingHubBubbleActionButton::~SharingHubBubbleActionButton() = default;

void SharingHubBubbleActionButton::UpdateBackgroundColor() {
// Pretend to be a menu item:
SkColor bg_color = GetNativeTheme()->GetSystemColor(
GetVisualState() == STATE_HOVERED
? ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor
: ui::NativeTheme::kColorId_MenuBackgroundColor);

SetBackground(views::CreateSolidBackground(bg_color));
SetTitleTextStyle(
// Give the hovered element the "selected" menu styling - otherwise the
// text color won't change appropriately to keep up with the background
// color changing in high contrast mode.
GetVisualState() == STATE_HOVERED ? views::style::STYLE_SELECTED
: views::style::STYLE_PRIMARY,
bg_color);
}

BEGIN_METADATA(SharingHubBubbleActionButton, HoverButton)
END_METADATA

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class SharingHubBubbleActionButton : public HoverButton {
return action_name_for_metrics_;
}

void UpdateBackgroundColor() override;

private:
const int action_command_id_;
const bool action_is_first_party_;
Expand Down

0 comments on commit 29fceb6

Please sign in to comment.