Skip to content

Commit

Permalink
desktop-share-hub: align with latest redlines
Browse files Browse the repository at this point in the history 
The exact specs (and a screenshot) are given on the linked bug. Much of
this change is adjustments of layout constants to match those.

This change also makes one structural adjustment:
SharingHubBubbleActionButton is no longer a HoverButton, but now a
regular Button. This is required to allow for local control over the
layout within the button, but has two happy effects:
* High contrast mode now works properly - before it did not work because
  of a limitation in HoverButton
* The name hack previously required because of another limitation in
  HoverButton has been removed

I tested:
* Light & dark mode
* High contrast mode
* Keyboard traversal with tab & arrow keys
* Screenreader navigation

Fixed: 1314486
Change-Id: I28a46226db6a04f232bb46ca88f831ae294899e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3576701
Reviewed-by: Kristi Park <kristipark@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#990180}
  • Loading branch information
Elly Fong-Jones authored and Chromium LUCI CQ committed Apr 7, 2022
1 parent aea9fcc commit d7788ae
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 92 deletions.
13 changes: 7 additions & 6 deletions chrome/browser/ui/views/sharing_hub/preview_view.cc
View file
Expand Up @@ -14,7 +14,11 @@
namespace sharing_hub {

namespace {
// These values are all directly from the Figma redlines. See
// https://crbug.com/1314486.
constexpr gfx::Size kImageSize{32, 32};
constexpr gfx::Insets kInteriorMargin = gfx::Insets::VH(12, 8);
constexpr gfx::Insets kDefaultMargin = gfx::Insets::VH(0, 8);
} // namespace

// This view uses two nested FlexLayouts, a horizontal outer one and a vertical
Expand All @@ -30,12 +34,9 @@ PreviewView::PreviewView(std::u16string title, GURL url, ui::ImageModel image) {
layout->SetOrientation(views::LayoutOrientation::kHorizontal)
.SetMainAxisAlignment(views::LayoutAlignment::kStart)
.SetCrossAxisAlignment(views::LayoutAlignment::kCenter)
// TODO(ellyjones): Check these against the redlines in the mocks, and
// once they're finalized, move them to the constant block above. Getting
// things to line up with the mocks may require a bit of a refactor in
// HoverButton itself.
.SetInteriorMargin(gfx::Insets::VH(12, 8))
.SetDefault(views::kMarginsKey, gfx::Insets::VH(0, 8));
.SetInteriorMargin(kInteriorMargin)
.SetDefault(views::kMarginsKey, kDefaultMargin)
.SetCollapseMargins(true);

image_ = AddChildView(std::make_unique<views::ImageView>(image));
image_->SetPreferredSize(kImageSize);
Expand Down
148 changes: 78 additions & 70 deletions chrome/browser/ui/views/sharing_hub/sharing_hub_bubble_action_button.cc
View file
Expand Up @@ -6,112 +6,120 @@

#include "chrome/app/vector_icons/vector_icons.h"
#include "chrome/browser/sharing_hub/sharing_hub_model.h"
#include "chrome/browser/ui/views/hover_button.h"
#include "chrome/browser/ui/views/sharing_hub/sharing_hub_bubble_view_impl.h"
#include "chrome/grit/generated_resources.h"
#include "ui/accessibility/ax_node_data.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/base/models/image_model.h"
#include "ui/color/color_id.h"
#include "ui/color/color_provider.h"
#include "ui/views/animation/ink_drop.h"
#include "ui/views/border.h"
#include "ui/views/accessibility/view_accessibility.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/controls/styled_label.h"
#include "ui/views/controls/label.h"
#include "ui/views/layout/flex_layout.h"

namespace sharing_hub {

namespace {

static constexpr int kPrimaryIconSize = 20;
constexpr auto kPrimaryIconBorder = gfx::Insets(6);

std::unique_ptr<views::ImageView> CreateIconFromVector(
const gfx::VectorIcon& vector_icon) {
auto icon = std::make_unique<views::ImageView>(ui::ImageModel::FromVectorIcon(
vector_icon, ui::kColorMenuIcon, kPrimaryIconSize));
icon->SetBorder(views::CreateEmptyBorder(kPrimaryIconBorder));
return icon;
}

std::unique_ptr<views::ImageView> CreateIconFromImageSkia(
const gfx::ImageSkia& png_icon) {
// The icon size has to be defined later if the image will be visible.
auto icon = std::make_unique<views::ImageView>();
icon->SetImageSize(gfx::Size(kPrimaryIconSize, kPrimaryIconSize));
icon->SetImage(png_icon);
icon->SetBorder(views::CreateEmptyBorder(kPrimaryIconBorder));
return icon;
// These values values come directly from the Figma redlines. See
// https://crbug.com/1314486.
static constexpr gfx::Insets kInteriorMargin = gfx::Insets::VH(8, 12);
static constexpr gfx::Insets kDefaultMargin = gfx::Insets::VH(0, 12);
static constexpr gfx::Size kPrimaryIconSize{24, 24};

// The layout will break if this icon isn't square - you may need to adjust the
// vector icon creation below.
static_assert(kPrimaryIconSize.width() == kPrimaryIconSize.height());

ui::ImageModel ImageForAction(const SharingHubAction& action_info) {
if (!action_info.third_party_icon.isNull())
return ui::ImageModel::FromImageSkia(action_info.third_party_icon);
return ui::ImageModel::FromVectorIcon(*action_info.icon, ui::kColorMenuIcon,
kPrimaryIconSize.width());
}

} // namespace

SharingHubBubbleActionButton::SharingHubBubbleActionButton(
SharingHubBubbleViewImpl* bubble,
const SharingHubAction& action_info)
: HoverButton(
base::BindRepeating(&SharingHubBubbleViewImpl::OnActionSelected,
base::Unretained(bubble),
base::Unretained(this)),

action_info.third_party_icon.isNull()
? CreateIconFromVector(*action_info.icon)
: CreateIconFromImageSkia(action_info.third_party_icon),
action_info.title),
action_command_id_(action_info.command_id),
: action_command_id_(action_info.command_id),
action_is_first_party_(action_info.is_first_party),
action_name_for_metrics_(action_info.feature_name_for_metrics) {
auto* layout = SetLayoutManager(std::make_unique<views::FlexLayout>());
layout->SetOrientation(views::LayoutOrientation::kHorizontal)
.SetMainAxisAlignment(views::LayoutAlignment::kStart)
.SetCrossAxisAlignment(views::LayoutAlignment::kCenter)
.SetInteriorMargin(kInteriorMargin)
.SetDefault(views::kMarginsKey, kDefaultMargin)
.SetCollapseMargins(true);

SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
SetEnabled(true);

title()->SetTextContext(views::style::CONTEXT_MENU);
SetBackground(
views::CreateThemedSolidBackground(this, ui::kColorMenuBackground));
SetCallback(base::BindRepeating(&SharingHubBubbleViewImpl::OnActionSelected,
base::Unretained(bubble),
base::Unretained(this)));

image_ = AddChildView(
std::make_unique<views::ImageView>(ImageForAction(action_info)));
image_->SetImageSize(kPrimaryIconSize);
image_->SetCanProcessEventsWithinSubtree(false);

title_ = AddChildView(std::make_unique<views::Label>(
action_info.title, views::style::CONTEXT_MENU));
title_->SetCanProcessEventsWithinSubtree(false);

if (action_is_first_party_) {
GetViewAccessibility().OverrideName(title_->GetText());
} else {
GetViewAccessibility().OverrideName(l10n_util::GetStringFUTF16(
IDS_SHARING_HUB_SHARE_LABEL_ACCESSIBILITY, title_->GetText()));
}
}

SharingHubBubbleActionButton::~SharingHubBubbleActionButton() = default;

void SharingHubBubbleActionButton::UpdateBackgroundColor() {
// Pretend to be a menu item:
SkColor bg_color =
GetColorProvider()->GetColor(GetVisualState() == STATE_HOVERED
? ui::kColorMenuItemBackgroundHighlighted
: ui::kColorMenuBackground);
void SharingHubBubbleActionButton::OnThemeChanged() {
views::Button::OnThemeChanged();
UpdateColors();
}

SetBackground(views::CreateSolidBackground(bg_color));
SetTitleTextStyle(
// Give the hovered element the "highlighted" 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_HIGHLIGHTED
: views::style::STYLE_PRIMARY,
bg_color);
void SharingHubBubbleActionButton::OnFocus() {
views::Button::OnFocus();
UpdateColors();
}

void SharingHubBubbleActionButton::GetAccessibleNodeData(
ui::AXNodeData* node_data) {
HoverButton::GetAccessibleNodeData(node_data);

// TODO(ellyjones): Yikes!
// This hack works around https://crbug.com/1245744, which is a design flaw in
// HoverButton. In particular, HoverButton overwrites the accessible name its
// client configured on it whenever it bounds change (!) with the plain
// concatenation of its title and subtitle, which makes it impossible to set
// an accessible name with extra context, as needed for
// https://crbug.com/1230178.
// TODO(https://crbug.com/1245744): Remove this.
if (!action_is_first_party_) {
node_data->SetName(l10n_util::GetStringFUTF16(
IDS_SHARING_HUB_SHARE_LABEL_ACCESSIBILITY, title()->GetText()));
}
void SharingHubBubbleActionButton::OnBlur() {
views::Button::OnBlur();
UpdateColors();
}

void SharingHubBubbleActionButton::OnThemeChanged() {
HoverButton::OnThemeChanged();
UpdateBackgroundColor();
void SharingHubBubbleActionButton::StateChanged(
views::Button::ButtonState old_state) {
views::Button::StateChanged(old_state);
UpdateColors();
}

void SharingHubBubbleActionButton::UpdateColors() {
bool draw_focus = GetState() == STATE_HOVERED || HasFocus();
// Pretend to be a menu item:
SkColor bg_color = GetColorProvider()->GetColor(
// Note: selected vs highlighted seems strange here; highlighted is more
// in line with what's happening. The two colors are almost the same but
// selected gives better behavior in high contrast.
draw_focus ? ui::kColorMenuItemBackgroundSelected
: ui::kColorMenuBackground);

SetBackground(views::CreateSolidBackground(bg_color));
title_->SetBackgroundColor(bg_color);
title_->SetTextStyle(draw_focus ? views::style::STYLE_SELECTED
: views::style::STYLE_PRIMARY);
}

BEGIN_METADATA(SharingHubBubbleActionButton, HoverButton)
BEGIN_METADATA(SharingHubBubbleActionButton, Button)
END_METADATA

} // namespace sharing_hub
21 changes: 17 additions & 4 deletions chrome/browser/ui/views/sharing_hub/sharing_hub_bubble_action_button.h
View file
Expand Up @@ -6,15 +6,20 @@
#define CHROME_BROWSER_UI_VIEWS_SHARING_HUB_SHARING_HUB_BUBBLE_ACTION_BUTTON_H_

#include "base/bind.h"
#include "chrome/browser/ui/views/hover_button.h"
#include "ui/views/controls/button/button.h"

namespace views {
class ImageView;
class Label;
} // namespace views

namespace sharing_hub {

class SharingHubBubbleViewImpl;
struct SharingHubAction;

// A button representing an action in the Sharing Hub bubble.
class SharingHubBubbleActionButton : public HoverButton {
class SharingHubBubbleActionButton : public views::Button {
public:
METADATA_HEADER(SharingHubBubbleActionButton);
SharingHubBubbleActionButton(SharingHubBubbleViewImpl* bubble,
Expand All @@ -31,14 +36,22 @@ class SharingHubBubbleActionButton : public HoverButton {
}

// views::Button:
void UpdateBackgroundColor() override;
void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
// Listeners for various events, which this class uses to keep its visuals
// consistent.
void OnThemeChanged() override;
void StateChanged(views::Button::ButtonState old_state) override;
void OnFocus() override;
void OnBlur() override;

private:
const int action_command_id_;
const bool action_is_first_party_;
const std::string action_name_for_metrics_;

views::Label* title_;
views::ImageView* image_;

void UpdateColors();
};

} // namespace sharing_hub
Expand Down
21 changes: 9 additions & 12 deletions chrome/browser/ui/views/sharing_hub/sharing_hub_bubble_view_impl.cc
View file
Expand Up @@ -153,25 +153,22 @@ void SharingHubBubbleViewImpl::PopulateScrollView(

auto* separator =
action_list_view->AddChildView(std::make_unique<views::Separator>());
constexpr int kIndent = 16;
constexpr int kPadding = 8;
separator->SetBorder(views::CreateEmptyBorder(
gfx::Insets::TLBR(kPadding, kIndent, 0, kIndent)));
constexpr int kIndent = 12;
constexpr int kPadding = 4;
separator->SetBorder(
views::CreateEmptyBorder(gfx::Insets::TLBR(kPadding, 0, kPadding, 0)));

constexpr int kLabelLineHeight = 22;
constexpr int kLabelLinePaddingTop = 8;
constexpr int kLabelLinePaddingBottom = 4;
constexpr int kLabelLineHeight = 32;

auto* share_link_label =
new views::Label(l10n_util::GetStringUTF16(IDS_SHARING_HUB_SHARE_LABEL));
share_link_label->SetFontList(gfx::FontList("GoogleSans, 13px"));
new views::Label(l10n_util::GetStringUTF16(IDS_SHARING_HUB_SHARE_LABEL),
views::style::CONTEXT_DIALOG_TITLE);
share_link_label->SetLineHeight(kLabelLineHeight);
share_link_label->SetMultiLine(true);
share_link_label->SetHorizontalAlignment(gfx::ALIGN_TO_HEAD);
share_link_label->SizeToFit(views::DISTANCE_BUBBLE_PREFERRED_WIDTH);
constexpr auto kPrimaryIconBorder = gfx::Insets::TLBR(
kLabelLinePaddingTop, kIndent, kLabelLinePaddingBottom, kIndent);
share_link_label->SetBorder(views::CreateEmptyBorder(kPrimaryIconBorder));
share_link_label->SetBorder(
views::CreateEmptyBorder(gfx::Insets::TLBR(0, kIndent, 0, 0)));
share_link_label_ = action_list_view->AddChildView(share_link_label);

for (const auto& action : third_party_actions) {
Expand Down

0 comments on commit d7788ae

Please sign in to comment.