Fix IME and Stylus tray text colors.

Previously, TrayPopupItemStyle colors are just switched by the flag
--enable-features=SystemTrayUnified. However, we had some other tray
bubbles (PaletteTray and ImeMenuTray) that use TrayPopupItemStyle,
and in these places we don't want the colors to be changed.

To prevent this, we add |use_unified_theme| flag to TrayPopupItemStyle,
and explicitly set false in PaletteTray and ImeMenuTray.

TEST=manual
BUG=847319

Change-Id: I183f9b9ddaca1adf2063b7e40bef9438c451ae48
Reviewed-on: https://chromium-review.googlesource.com/1075844Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563167}

ash/system/ime_menu/ime_list_view.cc

@@ -6,6 +6,7 @@
 
 #include "ash/ime/ime_controller.h"
 #include "ash/ime/ime_switch_type.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/public/interfaces/ime_info.mojom.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
@@ -49,7 +50,8 @@ class ImeListItemView : public ActionableView {
                   const base::string16& id,
                   const base::string16& label,
                   bool selected,
-                  const SkColor button_color)
+                  const SkColor button_color,
+                  bool use_unified_theme)
       : ActionableView(nullptr, TrayPopupInkDropStyle::FILL_BOUNDS),
         ime_list_view_(list_view),
         selected_(selected) {
@@ -82,8 +84,8 @@ class ImeListItemView : public ActionableView {
     // The label shows the IME full name.
     auto* label_view = TrayPopupUtils::CreateDefaultLabel();
     label_view->SetText(label);
-    TrayPopupItemStyle style(
-        TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL,
+                             use_unified_theme);
     style.SetupLabel(label_view);
 
     label_view->SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -144,7 +146,7 @@ class KeyboardStatusRow : public views::View {
   views::ToggleButton* toggle() const { return toggle_; }
   bool is_toggled() const { return toggle_->is_on(); }
 
-  void Init(views::ButtonListener* listener) {
+  void Init(views::ButtonListener* listener, bool use_unified_theme) {
     TrayPopupUtils::ConfigureAsStickyHeader(this);
     SetLayoutManager(std::make_unique<views::FillLayout>());
 
@@ -161,8 +163,8 @@ class KeyboardStatusRow : public views::View {
     auto* label = TrayPopupUtils::CreateDefaultLabel();
     label->SetText(ui::ResourceBundle::GetSharedInstance().GetLocalizedString(
         IDS_ASH_STATUS_TRAY_ACCESSIBILITY_VIRTUAL_KEYBOARD));
-    TrayPopupItemStyle style(
-        TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL,
+                             use_unified_theme);
     style.SetupLabel(label);
     tri_view->AddView(TriView::Container::CENTER, label);
 
@@ -181,10 +183,14 @@ class KeyboardStatusRow : public views::View {
 };
 
 ImeListView::ImeListView(DetailedViewDelegate* delegate)
+    : ImeListView(delegate, features::IsSystemTrayUnifiedEnabled()) {}
+
+ImeListView::ImeListView(DetailedViewDelegate* delegate, bool use_unified_theme)
     : TrayDetailedView(delegate),
       last_item_selected_with_keyboard_(false),
       should_focus_ime_after_selection_with_keyboard_(false),
-      current_ime_view_(nullptr) {}
+      current_ime_view_(nullptr),
+      use_unified_theme_(use_unified_theme) {}
 
 ImeListView::~ImeListView() = default;
 
@@ -249,8 +255,9 @@ void ImeListView::AppendImeListAndProperties(
   DCHECK(ime_map_.empty());
   for (size_t i = 0; i < list.size(); i++) {
     const bool selected = current_ime_id == list[i].id;
-    views::View* ime_view = new ImeListItemView(
-        this, list[i].short_name, list[i].name, selected, gfx::kGoogleGreen700);
+    views::View* ime_view =
+        new ImeListItemView(this, list[i].short_name, list[i].name, selected,
+                            gfx::kGoogleGreen700, use_unified_theme_);
     scroll_content()->AddChildView(ime_view);
     ime_map_[ime_view] = list[i].id;
 
@@ -265,9 +272,9 @@ void ImeListView::AppendImeListAndProperties(
 
       // Adds the property items.
       for (size_t i = 0; i < property_list.size(); i++) {
-        ImeListItemView* property_view =
-            new ImeListItemView(this, base::string16(), property_list[i].label,
-                                property_list[i].checked, kMenuIconColor);
+        ImeListItemView* property_view = new ImeListItemView(
+            this, base::string16(), property_list[i].label,
+            property_list[i].checked, kMenuIconColor, use_unified_theme_);
         scroll_content()->AddChildView(property_view);
         property_map_[property_view] = property_list[i].key;
       }
@@ -284,7 +291,7 @@ void ImeListView::AppendImeListAndProperties(
 void ImeListView::PrependKeyboardStatusRow() {
   DCHECK(!keyboard_status_row_);
   keyboard_status_row_ = new KeyboardStatusRow;
-  keyboard_status_row_->Init(this);
+  keyboard_status_row_->Init(this, use_unified_theme_);
   scroll_content()->AddChildViewAt(keyboard_status_row_, 0);
 }
 

ash/system/ime_menu/ime_list_view.h

@@ -32,7 +32,9 @@ class ImeListView : public TrayDetailedView {
     HIDE_SINGLE_IME
   };
 
+  // The former uses default for |use_unified_theme|.
   explicit ImeListView(DetailedViewDelegate* delegate);
+  ImeListView(DetailedViewDelegate* delegate, bool use_unified_theme);
 
   ~ImeListView() override;
 
@@ -111,6 +113,8 @@ class ImeListView : public TrayDetailedView {
   // The item view of the current selected IME.
   views::View* current_ime_view_;
 
+  const bool use_unified_theme_;
+
   DISALLOW_COPY_AND_ASSIGN(ImeListView);
 };
 

ash/system/ime_menu/ime_menu_tray.cc

@@ -122,7 +122,8 @@ class ImeTitleView : public views::View, public views::ButtonListener {
     title_label->SetBorder(
         views::CreateEmptyBorder(0, kMenuEdgeEffectivePadding, 1, 0));
     title_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::TITLE);
+    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::TITLE,
+                             false /* use_unified_theme */);
     style.SetupLabel(title_label);
 
     AddChildView(title_label);
@@ -254,7 +255,7 @@ class ImeButtonsView : public views::View, public views::ButtonListener {
 // height depending on the number of IMEs in the list.
 class ImeMenuListView : public ImeListView, public DetailedViewDelegate {
  public:
-  ImeMenuListView() : ImeListView(this) {
+  ImeMenuListView() : ImeListView(this, false /* use_unified_theme */) {
     set_should_focus_ime_after_selection_with_keyboard(true);
   }
 

ash/system/palette/common_palette_tool.cc

@@ -76,7 +76,7 @@ void CommonPaletteTool::OnViewClicked(views::View* sender) {
 views::View* CommonPaletteTool::CreateDefaultView(const base::string16& name) {
   gfx::ImageSkia icon =
       CreateVectorIcon(GetPaletteIcon(), kMenuIconSize, gfx::kChromeIconGrey);
-  highlight_view_ = new HoverHighlightView(this);
+  highlight_view_ = new HoverHighlightView(this, false /* use_unified_theme */);
   highlight_view_->AddIconAndLabel(icon, name);
   return highlight_view_;
 }

ash/system/palette/palette_tray.cc

@@ -95,7 +95,8 @@ class TitleView : public views::View, public views::ButtonListener {
         new views::Label(l10n_util::GetStringUTF16(IDS_ASH_STYLUS_TOOLS_TITLE));
     title_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     AddChildView(title_label);
-    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::TITLE);
+    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::TITLE,
+                             false /* use_unified_theme */);
     style.SetupLabel(title_label);
     layout_ptr->SetFlexForView(title_label, 1);
     help_button_ = new SystemMenuButton(this, kSystemMenuHelpIcon,

ash/system/palette/tools/metalayer_mode.cc

@@ -206,7 +206,8 @@ void MetalayerMode::UpdateView() {
 
   highlight_view_->SetEnabled(selectable());
 
-  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL,
+                           false /* use_unified_theme */);
   style.set_color_style(highlight_view_->enabled()
                             ? TrayPopupItemStyle::ColorStyle::ACTIVE
                             : TrayPopupItemStyle::ColorStyle::DISABLED);

ash/system/tray/hover_highlight_view.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/tray/hover_highlight_view.h"
 
+#include "ash/public/cpp/ash_features.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_popup_utils.h"
@@ -20,8 +21,13 @@
 namespace ash {
 
 HoverHighlightView::HoverHighlightView(ViewClickListener* listener)
+    : HoverHighlightView(listener, features::IsSystemTrayUnifiedEnabled()) {}
+
+HoverHighlightView::HoverHighlightView(ViewClickListener* listener,
+                                       bool use_unified_theme)
     : ActionableView(nullptr, TrayPopupInkDropStyle::FILL_BOUNDS),
-      listener_(listener) {
+      listener_(listener),
+      use_unified_theme_(use_unified_theme) {
   set_notify_enter_exit_on_child(true);
   SetInkDropMode(InkDropHostView::InkDropMode::ON);
 }
@@ -76,7 +82,8 @@ void HoverHighlightView::SetSubText(const base::string16& sub_text) {
     tri_view_->AddView(TriView::Container::CENTER, sub_text_label_);
   }
 
-  TrayPopupItemStyle sub_style(TrayPopupItemStyle::FontStyle::CAPTION);
+  TrayPopupItemStyle sub_style(TrayPopupItemStyle::FontStyle::CAPTION,
+                               use_unified_theme_);
   sub_style.set_color_style(TrayPopupItemStyle::ColorStyle::INACTIVE);
   sub_style.SetupLabel(sub_text_label_);
   sub_text_label_->SetText(sub_text);
@@ -130,7 +137,7 @@ void HoverHighlightView::DoAddIconAndLabels(
   text_label_ = TrayPopupUtils::CreateDefaultLabel();
   text_label_->SetText(text);
   text_label_->SetEnabled(enabled());
-  TrayPopupItemStyle style(font_style);
+  TrayPopupItemStyle style(font_style, use_unified_theme_);
   style.SetupLabel(text_label_);
   tri_view_->AddView(TriView::Container::CENTER, text_label_);
   // By default, END container is invisible, so labels in the CENTER should have
@@ -158,7 +165,8 @@ void HoverHighlightView::AddLabelRow(const base::string16& text) {
   text_label_ = TrayPopupUtils::CreateDefaultLabel();
   text_label_->SetText(text);
 
-  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
+  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL,
+                           use_unified_theme_);
   style.SetupLabel(text_label_);
   tri_view_->AddView(TriView::Container::CENTER, text_label_);
 

ash/system/tray/hover_highlight_view.h

@@ -38,7 +38,9 @@ class HoverHighlightView : public ActionableView {
   };
 
   // If |listener| is null then no action is taken on click.
+  // The former uses default for |use_unified_theme|.
   explicit HoverHighlightView(ViewClickListener* listener);
+  HoverHighlightView(ViewClickListener* listener, bool use_unified_theme);
   ~HoverHighlightView() override;
 
   // Convenience function for populating the view with an icon and a label. This
@@ -136,6 +138,7 @@ class HoverHighlightView : public ActionableView {
   views::View* right_view_ = nullptr;
   TriView* tri_view_ = nullptr;
   bool expandable_ = false;
+  const bool use_unified_theme_;
   AccessibilityState accessibility_state_ = AccessibilityState::DEFAULT;
 
   DISALLOW_COPY_AND_ASSIGN(HoverHighlightView);

ash/system/tray/system_menu_button.cc

@@ -52,13 +52,9 @@ SystemMenuButton::SystemMenuButton(views::ButtonListener* listener,
 
 void SystemMenuButton::SetVectorIcon(const gfx::VectorIcon& icon) {
   SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(icon, features::IsSystemTrayUnifiedEnabled()
-                                           ? kUnifiedMenuIconColor
-                                           : kMenuIconColor));
+           gfx::CreateVectorIcon(icon, kMenuIconColor));
   SetImage(views::Button::STATE_DISABLED,
-           gfx::CreateVectorIcon(icon, features::IsSystemTrayUnifiedEnabled()
-                                           ? kUnifiedMenuIconColorDisabled
-                                           : kMenuIconColorDisabled));
+           gfx::CreateVectorIcon(icon, kMenuIconColorDisabled));
 }
 
 SystemMenuButton::~SystemMenuButton() = default;

ash/system/tray/tray_popup_item_style.cc

@@ -22,10 +22,10 @@ constexpr int kDisabledAlpha = 0x61;
 }  // namespace
 
 // static
-SkColor TrayPopupItemStyle::GetIconColor(ColorStyle color_style) {
-  const SkColor kBaseIconColor = features::IsSystemTrayUnifiedEnabled()
-                                     ? kUnifiedMenuIconColor
-                                     : gfx::kChromeIconGrey;
+SkColor TrayPopupItemStyle::GetIconColor(ColorStyle color_style,
+                                         bool use_unified_theme) {
+  const SkColor kBaseIconColor =
+      use_unified_theme ? kUnifiedMenuIconColor : gfx::kChromeIconGrey;
   switch (color_style) {
     case ColorStyle::ACTIVE:
       return kBaseIconColor;
@@ -41,7 +41,13 @@ SkColor TrayPopupItemStyle::GetIconColor(ColorStyle color_style) {
 }
 
 TrayPopupItemStyle::TrayPopupItemStyle(FontStyle font_style)
-    : font_style_(font_style), color_style_(ColorStyle::ACTIVE) {
+    : TrayPopupItemStyle(font_style, features::IsSystemTrayUnifiedEnabled()) {}
+
+TrayPopupItemStyle::TrayPopupItemStyle(FontStyle font_style,
+                                       bool use_unified_theme)
+    : font_style_(font_style),
+      color_style_(ColorStyle::ACTIVE),
+      use_unified_theme_(use_unified_theme) {
   if (font_style_ == FontStyle::SYSTEM_INFO)
     color_style_ = ColorStyle::INACTIVE;
 }
@@ -49,7 +55,7 @@ TrayPopupItemStyle::TrayPopupItemStyle(FontStyle font_style)
 TrayPopupItemStyle::~TrayPopupItemStyle() = default;
 
 SkColor TrayPopupItemStyle::GetTextColor() const {
-  const SkColor kBaseTextColor = features::IsSystemTrayUnifiedEnabled()
+  const SkColor kBaseTextColor = use_unified_theme_
                                      ? kUnifiedMenuTextColor
                                      : SkColorSetA(SK_ColorBLACK, 0xDE);
 
@@ -68,7 +74,7 @@ SkColor TrayPopupItemStyle::GetTextColor() const {
 }
 
 SkColor TrayPopupItemStyle::GetIconColor() const {
-  return GetIconColor(color_style_);
+  return GetIconColor(color_style_, use_unified_theme_);
 }
 
 void TrayPopupItemStyle::SetupLabel(views::Label* label) const {

ash/system/tray/tray_popup_item_style.h

@@ -17,6 +17,7 @@ namespace ash {
 // Central style provider for the system tray menu. Makes it easier to ensure
 // all visuals are consistent and easily updated in one spot instead of being
 // defined in multiple places throughout the code.
+// TODO(tetsui): Clean up this class after UnifiedSystemTray is launched.
 class TrayPopupItemStyle {
  public:
   // The different visual styles that a row can have.
@@ -52,9 +53,13 @@ class TrayPopupItemStyle {
 
   static constexpr double kInactiveIconAlpha = 0.54;
 
-  static SkColor GetIconColor(ColorStyle color_style);
+  static SkColor GetIconColor(ColorStyle color_style,
+                              bool use_unified_theme = false);
 
+  // The first constructor initializes |use_unified_theme_| with default. See
+  // the comment below.
   explicit TrayPopupItemStyle(FontStyle font_style);
+  TrayPopupItemStyle(FontStyle font_style, bool use_unified_theme);
   ~TrayPopupItemStyle();
 
   void set_color_style(ColorStyle color_style) { color_style_ = color_style; }
@@ -75,6 +80,11 @@ class TrayPopupItemStyle {
 
   ColorStyle color_style_;
 
+  // Use base colors for UnifiedSystemTray. If IsSystemTrayUnifiedEnabled() is
+  // true, the value is true by default.
+  // TODO(tetsui): Clean up this after UnifiedSystemTray is launched.
+  const bool use_unified_theme_;
+
   DISALLOW_COPY_AND_ASSIGN(TrayPopupItemStyle);
 };
 

ash/system/tray/tray_popup_utils.cc

@@ -160,14 +160,15 @@ TriView* TrayPopupUtils::CreateMultiTargetRowView() {
 views::Label* TrayPopupUtils::CreateDefaultLabel() {
   views::Label* label = new views::Label();
   label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  // Frequently the label will paint to a layer that's non-opaque, so subpixel
-  // rendering won't work unless we explicitly set a background. See
-  // crbug.com/686363
-  label->SetBackground(
-      features::IsSystemTrayUnifiedEnabled()
-          ? views::CreateSolidBackground(kUnifiedMenuBackgroundColor)
-          : views::CreateThemedSolidBackground(
-                label, ui::NativeTheme::kColorId_BubbleBackground));
+  if (features::IsSystemTrayUnifiedEnabled()) {
+    label->SetSubpixelRenderingEnabled(false);
+  } else {
+    // Frequently the label will paint to a layer that's non-opaque, so subpixel
+    // rendering won't work unless we explicitly set a background. See
+    // https://crbug.com/686363
+    label->SetBackground(views::CreateThemedSolidBackground(
+        label, ui::NativeTheme::kColorId_BubbleBackground));
+  }
   return label;
 }