GMC: Add colored background to dimiss button to fix contrast

This CL adds a circular colored background to dismiss buttons in the
GMC dialog. This fixes a contrast issue where the X was not always
visible against the media artwork.

Bug: 1006049
Change-Id: Ib56353437fce6e4ec1ad8e15268e5bbbb284607c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1829537Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701611}

ash/media/media_notification_container_impl.h

@@ -45,7 +45,7 @@ class ASH_EXPORT MediaNotificationContainerImpl
       const std::set<media_session::mojom::MediaSessionAction>& actions)
       override {}
   void OnMediaArtworkChanged(const gfx::ImageSkia& image) override {}
-  void OnForegoundColorChanged(SkColor color) override {}
+  void OnColorsChanged(SkColor foreground, SkColor background) override {}
 
   // views::View:
   void OnMouseEvent(ui::MouseEvent* event) override;

chrome/browser/ui/views/global_media_controls/media_notification_container_impl.cc

@@ -9,6 +9,8 @@
 #include "chrome/grit/generated_resources.h"
 #include "components/vector_icons/vector_icons.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/views/animation/ink_drop_mask.h"
+#include "ui/views/background.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/layout/fill_layout.h"
@@ -19,9 +21,11 @@ namespace {
 constexpr int kWidth = 400;
 constexpr gfx::Size kNormalSize = gfx::Size(kWidth, 100);
 constexpr gfx::Size kExpandedSize = gfx::Size(kWidth, 150);
-constexpr gfx::Size kDismissButtonSize = gfx::Size(24, 24);
+constexpr gfx::Size kDismissButtonSize = gfx::Size(30, 30);
 constexpr int kDismissButtonIconSize = 20;
+constexpr int kDismissButtonBackgroundRadius = 15;
 constexpr SkColor kDefaultForegroundColor = SK_ColorBLACK;
+constexpr SkColor kDefaultBackgroundColor = SK_ColorTRANSPARENT;
 
 // The minimum number of enabled and visible user actions such that we should
 // force the MediaNotificationView to be expanded.
@@ -29,6 +33,25 @@ constexpr int kMinVisibleActionsForExpanding = 4;
 
 }  // anonymous namespace
 
+class MediaNotificationContainerImpl::DismissButton
+    : public views::ImageButton {
+ public:
+  explicit DismissButton(views::ButtonListener* listener)
+      : views::ImageButton(listener) {
+    views::ConfigureVectorImageButton(this);
+  }
+
+  ~DismissButton() override = default;
+
+  std::unique_ptr<views::InkDropMask> CreateInkDropMask() const override {
+    return std::make_unique<views::CircleInkDropMask>(
+        size(), GetLocalBounds().CenterPoint(), kDismissButtonBackgroundRadius);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DismissButton);
+};
+
 MediaNotificationContainerImpl::MediaNotificationContainerImpl(
     MediaDialogView* parent,
     MediaToolbarButtonController* controller,
@@ -37,7 +60,8 @@ MediaNotificationContainerImpl::MediaNotificationContainerImpl(
     : parent_(parent),
       controller_(controller),
       id_(id),
-      foreground_color_(kDefaultForegroundColor) {
+      foreground_color_(kDefaultForegroundColor),
+      background_color_(kDefaultBackgroundColor) {
   DCHECK(parent_);
   DCHECK(controller_);
 
@@ -45,16 +69,23 @@ MediaNotificationContainerImpl::MediaNotificationContainerImpl(
 
   SetPreferredSize(kNormalSize);
 
-  dismiss_button_ = views::CreateVectorImageButton(this);
-  dismiss_button_->SetPreferredSize(kDismissButtonSize);
-  dismiss_button_->SetTooltipText(l10n_util::GetStringUTF16(
+  dismiss_button_container_ = std::make_unique<views::View>();
+  dismiss_button_container_->set_owned_by_client();
+  dismiss_button_container_->SetPreferredSize(kDismissButtonSize);
+  dismiss_button_container_->SetLayoutManager(
+      std::make_unique<views::FillLayout>());
+  dismiss_button_container_->SetVisible(true);
+
+  auto dismiss_button = std::make_unique<DismissButton>(this);
+  dismiss_button->SetPreferredSize(kDismissButtonSize);
+  dismiss_button->SetTooltipText(l10n_util::GetStringUTF16(
       IDS_GLOBAL_MEDIA_CONTROLS_DISMISS_ICON_TOOLTIP_TEXT));
-  dismiss_button_->set_owned_by_client();
-  dismiss_button_->SetVisible(false);
+  dismiss_button_ =
+      dismiss_button_container_->AddChildView(std::move(dismiss_button));
   UpdateDismissButtonIcon();
 
   view_ = std::make_unique<media_message_center::MediaNotificationView>(
-      this, std::move(item), dismiss_button_.get(), base::string16());
+      this, std::move(item), dismiss_button_container_.get(), base::string16());
   view_->set_owned_by_client();
   ForceExpandedState();
 
@@ -71,7 +102,7 @@ void MediaNotificationContainerImpl::OnExpanded(bool expanded) {
 
 void MediaNotificationContainerImpl::OnMediaSessionInfoChanged(
     const media_session::mojom::MediaSessionInfoPtr& session_info) {
-  dismiss_button_->SetVisible(
+  dismiss_button_container_->SetVisible(
       !session_info || session_info->playback_state !=
                            media_session::mojom::MediaPlaybackState::kPlaying);
 }
@@ -85,24 +116,43 @@ void MediaNotificationContainerImpl::OnVisibleActionsChanged(
 void MediaNotificationContainerImpl::OnMediaArtworkChanged(
     const gfx::ImageSkia& image) {
   has_artwork_ = !image.isNull();
+
+  UpdateDismissButtonBackground();
   ForceExpandedState();
 }
 
-void MediaNotificationContainerImpl::OnForegoundColorChanged(SkColor color) {
-  foreground_color_ = color;
-  UpdateDismissButtonIcon();
+void MediaNotificationContainerImpl::OnColorsChanged(SkColor foreground,
+                                                     SkColor background) {
+  if (foreground_color_ != foreground) {
+    foreground_color_ = foreground;
+    UpdateDismissButtonIcon();
+  }
+  if (background_color_ != background) {
+    background_color_ = background;
+    UpdateDismissButtonBackground();
+  }
 }
 
 void MediaNotificationContainerImpl::ButtonPressed(views::Button* sender,
                                                    const ui::Event& event) {
-  DCHECK_EQ(dismiss_button_.get(), sender);
+  DCHECK_EQ(dismiss_button_, sender);
   controller_->OnDismissButtonClicked(id_);
 }
 
 void MediaNotificationContainerImpl::UpdateDismissButtonIcon() {
-  views::SetImageFromVectorIcon(dismiss_button_.get(),
-                                vector_icons::kCloseRoundedIcon,
-                                kDismissButtonIconSize, foreground_color_);
+  views::SetImageFromVectorIconWithColor(
+      dismiss_button_, vector_icons::kCloseRoundedIcon, kDismissButtonIconSize,
+      foreground_color_);
+}
+
+void MediaNotificationContainerImpl::UpdateDismissButtonBackground() {
+  if (!has_artwork_) {
+    dismiss_button_container_->SetBackground(nullptr);
+    return;
+  }
+
+  dismiss_button_container_->SetBackground(views::CreateRoundedRectBackground(
+      background_color_, kDismissButtonBackgroundRadius));
 }
 
 void MediaNotificationContainerImpl::ForceExpandedState() {

chrome/browser/ui/views/global_media_controls/media_notification_container_impl.h

@@ -17,10 +17,6 @@ namespace media_message_center {
 class MediaNotificationItem;
 }  // namespace media_message_center
 
-namespace views {
-class ImageButton;
-}  // namespace views
-
 class MediaDialogView;
 class MediaToolbarButtonController;
 
@@ -47,14 +43,18 @@ class MediaNotificationContainerImpl
       const std::set<media_session::mojom::MediaSessionAction>& actions)
       override;
   void OnMediaArtworkChanged(const gfx::ImageSkia& image) override;
-  void OnForegoundColorChanged(SkColor color) override;
+  void OnColorsChanged(SkColor foreground, SkColor background) override;
 
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
  private:
+  class DismissButton;
+
   void UpdateDismissButtonIcon();
 
+  void UpdateDismissButtonBackground();
+
   // Updates the forced expanded state of |view_|.
   void ForceExpandedState();
 
@@ -69,10 +69,12 @@ class MediaNotificationContainerImpl
   MediaToolbarButtonController* const controller_;
 
   const std::string id_;
-  std::unique_ptr<views::ImageButton> dismiss_button_;
+  std::unique_ptr<views::View> dismiss_button_container_;
+  DismissButton* dismiss_button_;
   std::unique_ptr<media_message_center::MediaNotificationView> view_;
 
   SkColor foreground_color_;
+  SkColor background_color_;
 
   bool has_artwork_ = false;
   bool has_many_actions_ = false;

components/media_message_center/media_notification_container.h

@@ -36,8 +36,8 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationContainer {
   // Called when the media artwork changes.
   virtual void OnMediaArtworkChanged(const gfx::ImageSkia& image) = 0;
 
-  // Called when MediaNotificationView's foreground color changes.
-  virtual void OnForegoundColorChanged(SkColor color) = 0;
+  // Called when MediaNotificationView's colors change.
+  virtual void OnColorsChanged(SkColor foreground, SkColor background) = 0;
 
  protected:
   virtual ~MediaNotificationContainer() = default;

components/media_message_center/media_notification_view.cc

@@ -510,7 +510,7 @@ void MediaNotificationView::UpdateForegroundColor() {
     button->SchedulePaint();
   }
 
-  container_->OnForegoundColorChanged(foreground);
+  container_->OnColorsChanged(foreground, background);
 }
 
 }  // namespace media_message_center

components/media_message_center/media_notification_view_unittest.cc

@@ -93,7 +93,7 @@ class MockMediaNotificationContainer : public MediaNotificationContainer {
   MOCK_METHOD1(OnVisibleActionsChanged,
                void(const std::set<MediaSessionAction>& actions));
   MOCK_METHOD1(OnMediaArtworkChanged, void(const gfx::ImageSkia& image));
-  MOCK_METHOD1(OnForegoundColorChanged, void(SkColor color));
+  MOCK_METHOD2(OnColorsChanged, void(SkColor foreground, SkColor background));
 
   MediaNotificationView* view() const { return view_.get(); }
   void SetView(std::unique_ptr<MediaNotificationView> view) {
@@ -763,7 +763,7 @@ TEST_F(MAYBE_MediaNotificationViewTest, UpdateArtworkFromItem) {
   const SkColor accent = header_row()->accent_color_for_testing();
   gfx::Size size = view()->size();
   EXPECT_CALL(container(), OnMediaArtworkChanged(_)).Times(2);
-  EXPECT_CALL(container(), OnForegoundColorChanged(_)).Times(2);
+  EXPECT_CALL(container(), OnColorsChanged(_, _)).Times(2);
 
   SkBitmap image;
   image.allocN32Pixels(10, 10);
@@ -974,7 +974,7 @@ TEST_F(MAYBE_MediaNotificationViewTest, Freezing_DoNotUpdateImage) {
   image.allocN32Pixels(10, 10);
   image.eraseColor(SK_ColorMAGENTA);
   EXPECT_CALL(container(), OnMediaArtworkChanged(_)).Times(0);
-  EXPECT_CALL(container(), OnForegoundColorChanged(_)).Times(0);
+  EXPECT_CALL(container(), OnColorsChanged(_, _)).Times(0);
 
   GetItem()->Freeze();
   GetItem()->MediaControllerImageChanged(