Notification View color refactor

Modified Notification View to take out hardcoded colors.  Moved
them to NativeTheme::ColorId and used the GetSystemColor call at the
different references to it.

Replaced NotificationButtonMD with MDTextButton.

Changed accent_color to base::Optional<SkColor>, updated associated
calls to account for that

Bug: 1056948
Change-Id: I28345c6b7f64301d1706531dc7df817348414d10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141215
Commit-Queue: Matthew Porter <matt.porter@microsoft.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762528}

ash/system/message_center/metrics_utils.cc

@@ -98,14 +98,18 @@ NotificationTypeDetailed GetNotificationTypeForCrosSystemPriority(
     const message_center::Notification& notification) {
   // The warning level is not stored in the notification data, so we need to
   // infer it from the accent color.
-  SkColor accent_color = notification.rich_notification_data().accent_color;
+  base::Optional<SkColor> accent_color =
+      notification.rich_notification_data().accent_color;
   message_center::SystemNotificationWarningLevel warning_level =
       message_center::SystemNotificationWarningLevel::NORMAL;
-  if (accent_color == kSystemNotificationColorWarning) {
-    warning_level = message_center::SystemNotificationWarningLevel::WARNING;
-  } else if (accent_color == kSystemNotificationColorCriticalWarning) {
-    warning_level =
-        message_center::SystemNotificationWarningLevel::CRITICAL_WARNING;
+  if (accent_color.has_value()) {
+    if (accent_color.value() == kSystemNotificationColorWarning) {
+      warning_level = message_center::SystemNotificationWarningLevel::WARNING;
+    } else if (accent_color.value() ==
+               kSystemNotificationColorCriticalWarning) {
+      warning_level =
+          message_center::SystemNotificationWarningLevel::CRITICAL_WARNING;
+    }
   }
 
   bool pinned = notification.rich_notification_data().pinned;

ash/system/network/auto_connect_notifier.cc

@@ -162,7 +162,8 @@ void AutoConnectNotifier::DisplayNotification(
       message_center::SystemNotificationWarningLevel::NORMAL);
 
   notification->set_small_image(gfx::Image(network_icon::GetImageForWifiNetwork(
-      notification->accent_color(),
+      notification->accent_color().value_or(
+          ash::kSystemNotificationColorNormal),
       gfx::Size(message_center::kSmallImageSizeMD,
                 message_center::kSmallImageSizeMD))));
 

ui/message_center/public/cpp/notification.h

@@ -168,7 +168,7 @@ class MESSAGE_CENTER_PUBLIC_EXPORT RichNotificationData {
   // Usually, it should not be set directly.
   // For system notification, ash::CreateSystemNotification with
   // SystemNotificationWarningLevel should be used.
-  SkColor accent_color = SK_ColorTRANSPARENT;
+  base::Optional<SkColor> accent_color;
 
   // Controls whether a settings button should appear on the notification. See
   // enum definition. TODO(estade): turn this into a boolean. See
@@ -392,8 +392,10 @@ class MESSAGE_CENTER_PUBLIC_EXPORT Notification {
   const base::string16& accessible_name() const {
     return optional_fields_.accessible_name;
   }
-
-  SkColor accent_color() const { return optional_fields_.accent_color; }
+  // If no value, set accent_color to a default value
+  base::Optional<SkColor> accent_color() const {
+    return optional_fields_.accent_color;
+  }
   void set_accent_color(SkColor accent_color) {
     optional_fields_.accent_color = accent_color;
   }

ui/message_center/views/notification_view_md.cc

@@ -43,6 +43,7 @@
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/button/radio_button.h"
 #include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/controls/image_view.h"
@@ -53,6 +54,7 @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/native_cursor.h"
+#include "ui/views/style/typography.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
@@ -81,13 +83,6 @@ constexpr gfx::Insets kSettingsRowPadding(8, 0, 0, 0);
 constexpr gfx::Insets kSettingsRadioButtonPadding(14, 18, 14, 18);
 constexpr gfx::Insets kSettingsButtonRowPadding(8);
 
-// Ripple ink drop opacity of action buttons.
-const float kActionButtonInkDropRippleVisibleOpacity = 0.08f;
-// Highlight (hover) ink drop opacity of action buttons.
-const float kActionButtonInkDropHighlightVisibleOpacity = 0.08f;
-// Text color of action button.
-constexpr SkColor kActionButtonTextColor = gfx::kGoogleBlue600;
-
 // The icon size of inline reply input field.
 constexpr int kInputReplyButtonSize = 20;
 
@@ -316,46 +311,28 @@ gfx::Size LargeImageView::GetResizedImageSize() {
   return resized_size;
 }
 
-// NotificationButtonMD ////////////////////////////////////////////////////////
+// NotificationMDTextButton ////////////////////////////////////////////////
 
-NotificationButtonMD::NotificationButtonMD(
+NotificationMdTextButton::NotificationMdTextButton(
     views::ButtonListener* listener,
     const base::string16& label,
     const base::Optional<base::string16>& placeholder)
-    : views::LabelButton(listener,
-                         base::i18n::ToUpper(label),
-                         views::style::CONTEXT_BUTTON_MD),
+    : views::MdTextButton(listener, views::style::CONTEXT_BUTTON_MD),
       placeholder_(placeholder) {
-  SetHorizontalAlignment(gfx::ALIGN_CENTER);
-  SetInkDropMode(InkDropMode::ON);
-  set_has_ink_drop_action_on_click(true);
-  set_ink_drop_base_color(SK_ColorBLACK);
-  set_ink_drop_visible_opacity(kActionButtonInkDropRippleVisibleOpacity);
-  SetEnabledTextColors(kActionButtonTextColor);
+  SetText(label);
   SetBorder(views::CreateEmptyBorder(kActionButtonPadding));
   SetMinSize(kActionButtonMinSize);
-  SetFocusForPlatform();
-
   views::InstallRectHighlightPathGenerator(this);
+  SetTextSubpixelRenderingEnabled(false);
 }
 
-NotificationButtonMD::~NotificationButtonMD() = default;
+NotificationMdTextButton::~NotificationMdTextButton() = default;
 
-void NotificationButtonMD::SetText(const base::string16& text) {
+void NotificationMdTextButton::SetText(const base::string16& text) {
   views::LabelButton::SetText(base::i18n::ToUpper(text));
 }
 
-const char* NotificationButtonMD::GetClassName() const {
-  return "NotificationButtonMD";
-}
-
-std::unique_ptr<views::InkDropHighlight>
-NotificationButtonMD::CreateInkDropHighlight() const {
-  std::unique_ptr<views::InkDropHighlight> highlight =
-      views::LabelButton::CreateInkDropHighlight();
-  highlight->set_visible_opacity(kActionButtonInkDropHighlightVisibleOpacity);
-  return highlight;
-}
+void NotificationMdTextButton::UpdateColors() {}
 
 // NotificationInputContainerMD ////////////////////////////////////////////////
 
@@ -424,7 +401,8 @@ NotificationInputContainerMD::CreateInkDropRipple() const {
 }
 
 SkColor NotificationInputContainerMD::GetInkDropBaseColor() const {
-  return gfx::kGoogleBlue600;
+  return GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_NotificationInkDropBase);
 }
 
 void NotificationInputContainerMD::OnThemeChanged() {
@@ -492,13 +470,6 @@ class InlineSettingsRadioButton : public views::RadioButton {
     label()->SetBackgroundColor(GetNativeTheme()->GetSystemColor(
         ui::NativeTheme::kColorId_NotificationInlineSettingsBackground));
   }
-
- private:
-  // views::RadioButton:
-  SkColor GetIconImageColor(int icon_state) const override {
-    return (icon_state & IconState::CHECKED) ? kActionButtonTextColor
-                                             : kRegularTextColorMD;
-  }
 };
 
 // NotificationInkDropImpl /////////////////////////////////////////////////////
@@ -864,9 +835,8 @@ void NotificationViewMD::OnNotificationInputSubmit(size_t index,
 
 void NotificationViewMD::CreateOrUpdateContextTitleView(
     const Notification& notification) {
-  header_row_->SetAccentColor(notification.accent_color() == SK_ColorTRANSPARENT
-                                  ? kNotificationDefaultAccentColor
-                                  : notification.accent_color());
+  header_row_->SetAccentColor(
+      notification.accent_color().value_or(kNotificationDefaultAccentColor));
   header_row_->SetTimestamp(notification.timestamp());
   header_row_->SetAppNameElideBehavior(gfx::ELIDE_TAIL);
   header_row_->SetSummaryText(base::string16());
@@ -998,7 +968,6 @@ void NotificationViewMD::CreateOrUpdateProgressBarView(
                                                 /* allow_round_corner */ false);
     progress_bar_view_->SetBorder(
         views::CreateEmptyBorder(kProgressBarTopPadding, 0, 0, 0));
-    progress_bar_view_->SetForegroundColor(kActionButtonTextColor);
     left_content_->AddChildViewAt(progress_bar_view_, left_content_count_);
   }
 
@@ -1091,10 +1060,8 @@ void NotificationViewMD::CreateOrUpdateSmallIconView(
   // TODO(knollr): figure out if this has a performance impact and
   // cache images if so. (crbug.com/768748)
   gfx::Image masked_small_icon = notification.GenerateMaskedSmallIcon(
-      kSmallImageSizeMD, notification.accent_color() == SK_ColorTRANSPARENT
-                             ? message_center::kNotificationDefaultAccentColor
-                             : notification.accent_color());
-
+      kSmallImageSizeMD, notification.accent_color().value_or(
+                             message_center::kNotificationDefaultAccentColor));
   if (masked_small_icon.IsEmpty()) {
     header_row_->ClearAppIcon();
   } else {
@@ -1158,10 +1125,9 @@ void NotificationViewMD::CreateOrUpdateActionButtonViews(
   for (size_t i = 0; i < buttons.size(); ++i) {
     ButtonInfo button_info = buttons[i];
     if (new_buttons) {
-      NotificationButtonMD* button = new NotificationButtonMD(
-          this, button_info.title, button_info.placeholder);
-      action_buttons_.push_back(button);
-      action_buttons_row_->AddChildView(button);
+      action_buttons_.push_back(action_buttons_row_->AddChildView(
+          std::make_unique<NotificationMdTextButton>(this, button_info.title,
+                                                     button_info.placeholder)));
     } else {
       action_buttons_[i]->SetText(button_info.title);
       action_buttons_[i]->set_placeholder(button_info.placeholder);
@@ -1170,10 +1136,10 @@ void NotificationViewMD::CreateOrUpdateActionButtonViews(
     }
 
     // Change action button color to the accent color.
-    action_buttons_[i]->SetEnabledTextColors(notification.accent_color() ==
-                                                     SK_ColorTRANSPARENT
-                                                 ? kActionButtonTextColor
-                                                 : notification.accent_color());
+    action_buttons_[i]->SetEnabledTextColors(
+        notification.accent_color().value_or(
+            views::style::GetColor(*this, views::style::CONTEXT_BUTTON_MD,
+                                   views::style::STYLE_PRIMARY)));
   }
 
   // Inherit mouse hover state when action button views reset.
@@ -1242,10 +1208,9 @@ void NotificationViewMD::CreateOrUpdateInlineSettingsViews(
   settings_row_->AddChildView(dont_block_button_);
   settings_row_->SetVisible(false);
 
-  settings_done_button_ = new NotificationButtonMD(
+  settings_done_button_ = new NotificationMdTextButton(
       this, l10n_util::GetStringUTF16(IDS_MESSAGE_CENTER_SETTINGS_DONE),
       base::nullopt);
-  settings_done_button_->SetTextSubpixelRenderingEnabled(false);
 
   auto* settings_button_row = new views::View;
   auto settings_button_layout = std::make_unique<views::BoxLayout>(

ui/message_center/views/notification_view_md.h

@@ -15,13 +15,12 @@
 #include "ui/message_center/views/message_view.h"
 #include "ui/views/animation/ink_drop_observer.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/button/label_button.h"
+#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
 
 namespace views {
 class ImageButton;
 class Label;
-class LabelButton;
 class ProgressBar;
 class RadioButton;
 class Textfield;
@@ -32,6 +31,33 @@ namespace message_center {
 class NotificationHeaderView;
 class ProportionalImageView;
 
+// NotificationMdTextButton extends MdText button to allow for placeholder text
+// as well as capitalizing the given label string.
+class NotificationMdTextButton : public views::MdTextButton {
+ public:
+  NotificationMdTextButton(views::ButtonListener* listener,
+                           const base::string16& label,
+                           const base::Optional<base::string16>& placeholder);
+  ~NotificationMdTextButton() override;
+
+  // Overridden from MdTextButton
+  void UpdateColors() override;
+  void SetText(const base::string16& text) override;
+
+  const base::Optional<base::string16>& placeholder() const {
+    return placeholder_;
+  }
+  void set_placeholder(base::Optional<base::string16> placeholder) {
+    placeholder_ = std::move(placeholder);
+  }
+  SkColor enabled_color_for_testing() const {
+    return label()->GetEnabledColor();
+  }
+
+ private:
+  base::Optional<base::string16> placeholder_;
+};
+
 // CompactTitleMessageView shows notification title and message in a single
 // line. This view is used for NOTIFICATION_TYPE_PROGRESS.
 class CompactTitleMessageView : public views::View {
@@ -73,42 +99,6 @@ class LargeImageView : public views::View {
   DISALLOW_COPY_AND_ASSIGN(LargeImageView);
 };
 
-// This class is needed in addition to LabelButton mainly becuase we want to set
-// visible_opacity of InkDropHighlight.
-// This button capitalizes the given label string.
-class NotificationButtonMD : public views::LabelButton {
- public:
-  // |is_inline_reply| is true when the notification action takes text as the
-  // return value i.e. the notification action is inline reply.
-  // The input field would be shown when the button is clicked.
-  // |placeholder| is placeholder text shown on the input field. Only used when
-  // |is_inline_reply| is true.
-  NotificationButtonMD(views::ButtonListener* listener,
-                       const base::string16& label,
-                       const base::Optional<base::string16>& placeholder);
-  ~NotificationButtonMD() override;
-
-  void SetText(const base::string16& text) override;
-  const char* GetClassName() const override;
-
-  std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
-      const override;
-
-  SkColor enabled_color_for_testing() { return label()->GetEnabledColor(); }
-
-  const base::Optional<base::string16>& placeholder() const {
-    return placeholder_;
-  }
-  void set_placeholder(const base::Optional<base::string16>& placeholder) {
-    placeholder_ = placeholder;
-  }
-
- private:
-  base::Optional<base::string16> placeholder_;
-
-  DISALLOW_COPY_AND_ASSIGN(NotificationButtonMD);
-};
-
 class NotificationInputDelegate {
  public:
   virtual void OnNotificationInputSubmit(size_t index,
@@ -310,7 +300,7 @@ class MESSAGE_CENTER_EXPORT NotificationViewMD
   views::Label* status_view_ = nullptr;
   ProportionalImageView* icon_view_ = nullptr;
   views::View* image_container_view_ = nullptr;
-  std::vector<NotificationButtonMD*> action_buttons_;
+  std::vector<NotificationMdTextButton*> action_buttons_;
   std::vector<views::View*> item_views_;
   views::ProgressBar* progress_bar_view_ = nullptr;
   CompactTitleMessageView* compact_title_message_view_ = nullptr;
@@ -324,7 +314,7 @@ class MESSAGE_CENTER_EXPORT NotificationViewMD
   // Views for inline settings.
   views::RadioButton* block_all_button_ = nullptr;
   views::RadioButton* dont_block_button_ = nullptr;
-  views::LabelButton* settings_done_button_ = nullptr;
+  NotificationMdTextButton* settings_done_button_ = nullptr;
 
   // Owned by views properties. Guaranteed to be not null for the lifetime of
   // |this| because views properties are the last thing cleaned up.

ui/native_theme/common_theme.cc

@@ -386,6 +386,8 @@ SkColor GetDefaultColor(NativeTheme::ColorId color_id,
       return SkColorSetRGB(0xf5, 0xf5, 0xf5);
     case NativeTheme::kColorId_NotificationEmptyPlaceholderIconColor:
       return SkColorSetA(SK_ColorWHITE, 0x60);
+    case NativeTheme::kColorId_NotificationInkDropBase:
+      return gfx::kGoogleBlue600;
 
     // Scrollbar
     case NativeTheme::kColorId_OverlayScrollbarThumbBackground:

ui/native_theme/native_theme_color_id.h

@@ -84,6 +84,7 @@
   OP(kColorId_NotificationLargeImageBackground),                               \
   OP(kColorId_NotificationPlaceholderIconColor),                               \
   OP(kColorId_NotificationEmptyPlaceholderIconColor),                          \
+  OP(kColorId_NotificationInkDropBase),                                        \
   /* Slider */                                                                 \
   OP(kColorId_SliderThumbDefault),                                             \
   OP(kColorId_SliderTroughDefault),                                            \

ui/views/controls/button/md_text_button.cc

@@ -151,7 +151,7 @@ std::unique_ptr<views::InkDropHighlight> MdTextButton::CreateInkDropHighlight()
       std::make_unique<BorderShadowLayerDelegate>(
           shadows, GetLocalBounds(), theme->GetSystemColor(fill_color_id),
           corner_radius_));
-  highlight->set_visible_opacity(1.0f);
+  highlight->set_visible_opacity(visible_opacity_);
   return highlight;
 }
 

ui/views/controls/button/md_text_button.h

@@ -57,6 +57,10 @@ class VIEWS_EXPORT MdTextButton : public LabelButton {
   PropertyEffects UpdateStyleToIndicateDefaultStatus() override;
   void StateChanged(ButtonState old_state) override;
 
+  void set_visible_opacity(float visible_opacity) {
+    visible_opacity_ = visible_opacity;
+  }
+
  protected:
   // View:
   void OnFocus() override;
@@ -66,7 +70,7 @@ class VIEWS_EXPORT MdTextButton : public LabelButton {
 
  private:
   void UpdatePadding();
-  void UpdateColors();
+  virtual void UpdateColors();
 
   // True if this button uses prominent styling (blue fill, etc.).
   bool is_prominent_;
@@ -74,6 +78,9 @@ class VIEWS_EXPORT MdTextButton : public LabelButton {
   // When set, this provides the background color.
   base::Optional<SkColor> bg_color_override_;
 
+  // This provides the visible opacity of the InkDropHighlight
+  float visible_opacity_ = 1.0f;
+
   float corner_radius_;
 
   DISALLOW_COPY_AND_ASSIGN(MdTextButton);