Fix icon highlights for hosted apps with dark theme colors

This CL fixes invisible icon highlights for hosted apps with
dark theme colors.

This CL updates the ink highlight base color logic of indicator
icons like content settings, page actions and extension actions
to be based on which container they're in.

This allows hosted app title bars to set the icon highlights
relative to the icon colors (and theme color) while leaving the
location bar to continue using its existing color logic.

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360311&signed_aid=D7ev8tHi_uOFeRdmHnjC4w==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360312&signed_aid=A_FQTmAn6KoTeq63KBhU7Q==&inline=1

A side effect of this CL is that content settings in the location
bar now use the same highlight color logic as everything else in
the location bar. Their highlight logic changed accidentally in
https://chromium-review.googlesource.com/1195721 when the location
bar started using the same code path as the hosted app button
container does for setting the icon's color, triggering a different
codepath in ContentSettingImageView::GetInkDropBaseColor().

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360308&signed_aid=sdSbBOu-p_YHCps3f1QFbw==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=360309&signed_aid=8gOEBWYKu8_ZCLpyzbHHdA==&inline=1

Bug: 876271
Change-Id: I610500fcd2362d371a82b6da79c9edc6a03d42c1
Reviewed-on: https://chromium-review.googlesource.com/c/1248521
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597810}

chrome/browser/ui/views/frame/hosted_app_button_container.cc

@@ -80,11 +80,10 @@ constexpr base::TimeDelta HostedAppButtonContainer::kOriginFadeOutDuration;
 const base::TimeDelta HostedAppButtonContainer::kOriginTotalDuration =
     kOriginFadeInDuration + kOriginPauseDuration + kOriginFadeOutDuration;
 
-class HostedAppButtonContainer::ContentSettingsContainer
-    : public views::View,
-      public ContentSettingImageView::Delegate {
+class HostedAppButtonContainer::ContentSettingsContainer : public views::View {
  public:
-  explicit ContentSettingsContainer(BrowserView* browser_view);
+  explicit ContentSettingsContainer(
+      ContentSettingImageView::Delegate* delegate);
   ~ContentSettingsContainer() override = default;
 
   void UpdateContentSettingViewsVisibility() {
@@ -132,26 +131,9 @@ class HostedAppButtonContainer::ContentSettingsContainer
     PreferredSizeChanged();
   }
 
-  // ContentSettingsImageView::Delegate:
-  content::WebContents* GetContentSettingWebContents() override {
-    return browser_view_->GetActiveWebContents();
-  }
-  ContentSettingBubbleModelDelegate* GetContentSettingBubbleModelDelegate()
-      override {
-    return browser_view_->browser()->content_setting_bubble_model_delegate();
-  }
-  void OnContentSettingImageBubbleShown(
-      ContentSettingImageModel::ImageType type) const override {
-    UMA_HISTOGRAM_ENUMERATION(
-        "HostedAppFrame.ContentSettings.ImagePressed", type,
-        ContentSettingImageModel::ImageType::NUM_IMAGE_TYPES);
-  }
-
   // Owned by the views hierarchy.
   std::vector<ContentSettingImageView*> content_setting_views_;
 
-  BrowserView* browser_view_;
-
   DISALLOW_COPY_AND_ASSIGN(ContentSettingsContainer);
 };
 
@@ -165,12 +147,7 @@ HostedAppButtonContainer::GetContentSettingViewsForTesting() const {
 }
 
 HostedAppButtonContainer::ContentSettingsContainer::ContentSettingsContainer(
-    BrowserView* browser_view)
-    : browser_view_(browser_view) {
-  DCHECK(
-      extensions::HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
-          browser_view->browser()));
-
+    ContentSettingImageView::Delegate* delegate) {
   views::BoxLayout& layout =
       *SetLayoutManager(std::make_unique<views::BoxLayout>(
           views::BoxLayout::kHorizontal, gfx::Insets(),
@@ -183,7 +160,7 @@ HostedAppButtonContainer::ContentSettingsContainer::ContentSettingsContainer(
       ContentSettingImageModel::GenerateContentSettingImageModels();
   for (auto& model : models) {
     auto image_view = std::make_unique<ContentSettingImageView>(
-        std::move(model), this,
+        std::move(model), delegate,
         views::NativeWidgetAura::GetWindowTitleFontList());
     // Padding around content setting icons.
     constexpr int kContentSettingIconInteriorPadding = 4;
@@ -205,7 +182,7 @@ HostedAppButtonContainer::HostedAppButtonContainer(views::Widget* widget,
       active_color_(active_color),
       inactive_color_(inactive_color),
       hosted_app_origin_text_(new HostedAppOriginText(browser_view->browser())),
-      content_settings_container_(new ContentSettingsContainer(browser_view)),
+      content_settings_container_(new ContentSettingsContainer(this)),
       page_action_icon_container_view_(new PageActionIconContainerView(
           {PageActionIconType::kFind, PageActionIconType::kZoom},
           GetLayoutConstant(HOSTED_APP_PAGE_ACTION_ICON_SIZE),
@@ -220,6 +197,9 @@ HostedAppButtonContainer::HostedAppButtonContainer(views::Widget* widget,
                                       false /* interactive */)),
       app_menu_button_(new HostedAppMenuButton(browser_view)) {
   DCHECK(browser_view_);
+  DCHECK(
+      extensions::HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
+          browser_view_->browser()));
   views::BoxLayout& layout =
       *SetLayoutManager(std::make_unique<views::BoxLayout>(
           views::BoxLayout::kHorizontal,
@@ -314,12 +294,20 @@ void HostedAppButtonContainer::DisableAnimationForTesting() {
   g_animation_disabled_for_testing = true;
 }
 
+SkColor HostedAppButtonContainer::GetIconColor() const {
+  return paint_as_active_ ? active_color_ : inactive_color_;
+}
+
+SkColor HostedAppButtonContainer::GetIconInkDropColor() const {
+  return color_utils::IsDark(GetIconColor()) ? SK_ColorBLACK : SK_ColorWHITE;
+}
+
 void HostedAppButtonContainer::UpdateChildrenColor() {
-  SkColor color = paint_as_active_ ? active_color_ : inactive_color_;
-  hosted_app_origin_text_->SetTextColor(color);
-  content_settings_container_->SetIconColor(color);
-  page_action_icon_container_view_->SetIconColor(color);
-  app_menu_button_->SetIconColor(color);
+  SkColor icon_color = GetIconColor();
+  hosted_app_origin_text_->SetTextColor(icon_color);
+  content_settings_container_->SetIconColor(icon_color);
+  page_action_icon_container_view_->SetIconColor(icon_color);
+  app_menu_button_->SetColors(icon_color, GetIconInkDropColor());
 }
 
 gfx::Size HostedAppButtonContainer::CalculatePreferredSize() const {
@@ -371,11 +359,35 @@ HostedAppButtonContainer::CreateToolbarActionsBar(
                                                       main_bar);
 }
 
+SkColor HostedAppButtonContainer::GetPageActionInkDropColor() const {
+  return GetIconInkDropColor();
+}
+
 content::WebContents*
 HostedAppButtonContainer::GetWebContentsForPageActionIconView() {
   return browser_view_->GetActiveWebContents();
 }
 
+SkColor HostedAppButtonContainer::GetContentSettingInkDropColor() const {
+  return GetIconInkDropColor();
+}
+
+content::WebContents* HostedAppButtonContainer::GetContentSettingWebContents() {
+  return browser_view_->GetActiveWebContents();
+}
+
+ContentSettingBubbleModelDelegate*
+HostedAppButtonContainer::GetContentSettingBubbleModelDelegate() {
+  return browser_view_->browser()->content_setting_bubble_model_delegate();
+}
+
+void HostedAppButtonContainer::OnContentSettingImageBubbleShown(
+    ContentSettingImageModel::ImageType type) const {
+  UMA_HISTOGRAM_ENUMERATION(
+      "HostedAppFrame.ContentSettings.ImagePressed", type,
+      ContentSettingImageModel::ImageType::NUM_IMAGE_TYPES);
+}
+
 BrowserActionsContainer*
 HostedAppButtonContainer::GetBrowserActionsContainer() {
   return browser_actions_container_;

chrome/browser/ui/views/frame/hosted_app_button_container.h

@@ -40,6 +40,7 @@ class Widget;
 class HostedAppButtonContainer : public views::AccessiblePaneView,
                                  public BrowserActionsContainer::Delegate,
                                  public PageActionIconView::Delegate,
+                                 public ContentSettingImageView::Delegate,
                                  public ToolbarButtonProvider,
                                  public ImmersiveModeController::Observer,
                                  public views::WidgetObserver {
@@ -103,6 +104,8 @@ class HostedAppButtonContainer : public views::AccessiblePaneView,
   const std::vector<ContentSettingImageView*>&
   GetContentSettingViewsForTesting() const;
 
+  SkColor GetIconColor() const;
+  SkColor GetIconInkDropColor() const;
   void UpdateChildrenColor();
 
   // views::View:
@@ -120,8 +123,17 @@ class HostedAppButtonContainer : public views::AccessiblePaneView,
       ToolbarActionsBar* main_bar) const override;
 
   // PageActionIconView::Delegate:
+  SkColor GetPageActionInkDropColor() const override;
   content::WebContents* GetWebContentsForPageActionIconView() override;
 
+  // ContentSettingImageView::Delegate:
+  SkColor GetContentSettingInkDropColor() const override;
+  content::WebContents* GetContentSettingWebContents() override;
+  ContentSettingBubbleModelDelegate* GetContentSettingBubbleModelDelegate()
+      override;
+  void OnContentSettingImageBubbleShown(
+      ContentSettingImageModel::ImageType type) const override;
+
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;
   PageActionIconContainerView* GetPageActionIconContainerView() override;

chrome/browser/ui/views/frame/hosted_app_menu_button.cc

@@ -49,9 +49,11 @@ HostedAppMenuButton::HostedAppMenuButton(BrowserView* browser_view)
 
 HostedAppMenuButton::~HostedAppMenuButton() {}
 
-void HostedAppMenuButton::SetIconColor(SkColor color) {
+void HostedAppMenuButton::SetColors(SkColor icon_color,
+                                    SkColor ink_drop_color) {
   SetImage(views::Button::STATE_NORMAL,
-           gfx::CreateVectorIcon(kBrowserToolsIcon, color));
+           gfx::CreateVectorIcon(kBrowserToolsIcon, icon_color));
+  ink_drop_color_ = ink_drop_color;
 }
 
 void HostedAppMenuButton::StartHighlightAnimation() {
@@ -80,6 +82,10 @@ void HostedAppMenuButton::OnMenuButtonClicked(views::MenuButton* source,
       base::UserMetricsAction("HostedAppMenuButtonButton_Clicked"));
 }
 
+SkColor HostedAppMenuButton::GetInkDropBaseColor() const {
+  return ink_drop_color_;
+}
+
 void HostedAppMenuButton::FadeHighlightOff() {
   if (!ShouldEnterHoveredState()) {
     GetInkDrop()->SetHoverHighlightFadeDurationMs(

chrome/browser/ui/views/frame/hosted_app_menu_button.h

@@ -8,6 +8,7 @@
 #include "base/timer/timer.h"
 #include "chrome/browser/ui/views/frame/app_menu_button.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/color_palette.h"
 #include "ui/views/controls/button/menu_button_listener.h"
 
 class BrowserView;
@@ -19,8 +20,8 @@ class HostedAppMenuButton : public AppMenuButton,
   explicit HostedAppMenuButton(BrowserView* browser_view);
   ~HostedAppMenuButton() override;
 
-  // Sets the color of the menu button icon.
-  void SetIconColor(SkColor color);
+  // Sets the color of the menu button icon and highlight.
+  void SetColors(SkColor icon_color, SkColor ink_drop_color);
 
   // Fades the menu button highlight on and off.
   void StartHighlightAnimation();
@@ -30,6 +31,9 @@ class HostedAppMenuButton : public AppMenuButton,
                            const gfx::Point& point,
                            const ui::Event* event) override;
 
+  // InkDropHostView:
+  SkColor GetInkDropBaseColor() const override;
+
  private:
   void FadeHighlightOff();
 
@@ -39,6 +43,8 @@ class HostedAppMenuButton : public AppMenuButton,
   // The containing browser view.
   BrowserView* browser_view_;
 
+  SkColor ink_drop_color_ = gfx::kPlaceholderColor;
+
   base::OneShotTimer highlight_off_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(HostedAppMenuButton);

chrome/browser/ui/views/location_bar/content_setting_image_view.cc

@@ -153,16 +153,15 @@ bool ContentSettingImageView::IsBubbleShowing() const {
   return bubble_view_ != nullptr;
 }
 
+SkColor ContentSettingImageView::GetInkDropBaseColor() const {
+  return delegate_->GetContentSettingInkDropColor();
+}
+
 ContentSettingImageModel::ImageType ContentSettingImageView::GetTypeForTesting()
     const {
   return content_setting_image_model_->image_type();
 }
 
-SkColor ContentSettingImageView::GetInkDropBaseColor() const {
-  return icon_color_ ? icon_color_.value()
-                     : IconLabelBubbleView::GetInkDropBaseColor();
-}
-
 void ContentSettingImageView::OnWidgetDestroying(views::Widget* widget) {
   DCHECK(bubble_view_);
   DCHECK_EQ(bubble_view_->GetWidget(), widget);

chrome/browser/ui/views/location_bar/content_setting_image_view.h

@@ -38,6 +38,9 @@ class ContentSettingImageView : public IconLabelBubbleView,
  public:
   class Delegate {
    public:
+    // Gets the color to use for the ink highlight.
+    virtual SkColor GetContentSettingInkDropColor() const = 0;
+
     // Gets the web contents the ContentSettingImageView is for.
     virtual content::WebContents* GetContentSettingWebContents() = 0;
 
@@ -49,9 +52,6 @@ class ContentSettingImageView : public IconLabelBubbleView,
     // Invoked when a bubble is shown.
     virtual void OnContentSettingImageBubbleShown(
         ContentSettingImageModel::ImageType type) const {}
-
-   protected:
-    virtual ~Delegate() {}
   };
 
   ContentSettingImageView(std::unique_ptr<ContentSettingImageModel> image_model,
@@ -75,11 +75,11 @@ class ContentSettingImageView : public IconLabelBubbleView,
   bool OnMousePressed(const ui::MouseEvent& event) override;
   bool OnKeyPressed(const ui::KeyEvent& event) override;
   void OnNativeThemeChanged(const ui::NativeTheme* native_theme) override;
-  SkColor GetInkDropBaseColor() const override;
   SkColor GetTextColor() const override;
   bool ShouldShowSeparator() const override;
   bool ShowBubble(const ui::Event& event) override;
   bool IsBubbleShowing() const override;
+  SkColor GetInkDropBaseColor() const override;
 
   ContentSettingImageModel::ImageType GetTypeForTesting() const;
 

chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

@@ -357,16 +357,6 @@ IconLabelBubbleView::CreateInkDropHighlight() const {
   return highlight;
 }
 
-SkColor IconLabelBubbleView::GetInkDropBaseColor() const {
-  const SkColor ink_color_opaque = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_TextfieldDefaultColor);
-  if (ui::MaterialDesignController::IsNewerMaterialUi()) {
-    // Opacity of the ink drop is set elsewhere, so just use full opacity here.
-    return ink_color_opaque;
-  }
-  return color_utils::DeriveDefaultIconColor(ink_color_opaque);
-}
-
 std::unique_ptr<views::InkDropMask> IconLabelBubbleView::CreateInkDropMask()
     const {
   if (!LocationBarView::IsRounded())

chrome/browser/ui/views/location_bar/icon_label_bubble_view.h

@@ -141,8 +141,8 @@ class IconLabelBubbleView : public views::InkDropObserver,
   std::unique_ptr<views::InkDropRipple> CreateInkDropRipple() const override;
   std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
       const override;
-  SkColor GetInkDropBaseColor() const override;
   std::unique_ptr<views::InkDropMask> CreateInkDropMask() const override;
+  SkColor GetInkDropBaseColor() const override = 0;
 
   // views::Button:
   bool IsTriggerableEvent(const ui::Event& event) override;

chrome/browser/ui/views/location_bar/icon_label_bubble_view_unittest.cc

@@ -84,6 +84,7 @@ class TestIconLabelBubbleView : public IconLabelBubbleView {
  protected:
   // IconLabelBubbleView:
   SkColor GetTextColor() const override { return kTestColor; }
+  SkColor GetInkDropBaseColor() const override { return kTestColor; }
 
   bool ShouldShowLabel() const override {
     return !IsShrinking() ||

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -333,6 +333,11 @@ SkColor LocationBarView::GetSecurityChipColor(
                          state);
 }
 
+SkColor LocationBarView::GetIconInkDropColor() const {
+  return GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_TextfieldDefaultColor);
+}
+
 void LocationBarView::SetStarToggled(bool on) {
   if (star_view_)
     star_view_->SetToggled(on);
@@ -749,6 +754,10 @@ WebContents* LocationBarView::GetWebContents() {
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, public ContentSettingImageView::Delegate implementation:
 
+SkColor LocationBarView::GetContentSettingInkDropColor() const {
+  return GetIconInkDropColor();
+}
+
 content::WebContents* LocationBarView::GetContentSettingWebContents() {
   return GetToolbarModel()->input_in_progress() ? nullptr : GetWebContents();
 }
@@ -760,6 +769,11 @@ LocationBarView::GetContentSettingBubbleModelDelegate() {
 
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, public PageActionIconView::Delegate implementation:
+
+SkColor LocationBarView::GetPageActionInkDropColor() const {
+  return GetIconInkDropColor();
+}
+
 WebContents* LocationBarView::GetWebContentsForPageActionIconView() {
   return GetWebContents();
 }

chrome/browser/ui/views/location_bar/location_bar_view.h

@@ -137,6 +137,9 @@ class LocationBarView : public LocationBar,
   SkColor GetSecurityChipColor(
       security_state::SecurityLevel security_level) const;
 
+  // Returns the color to use for icon ink highlights.
+  SkColor GetIconInkDropColor() const;
+
   // Returns the cached theme color tint for the location bar and results.
   OmniboxTint tint() const { return tint_; }
 
@@ -239,6 +242,7 @@ class LocationBarView : public LocationBar,
   content::WebContents* GetWebContents() override;
 
   // ContentSettingImageView::Delegate:
+  SkColor GetContentSettingInkDropColor() const override;
   content::WebContents* GetContentSettingWebContents() override;
   ContentSettingBubbleModelDelegate* GetContentSettingBubbleModelDelegate()
       override;
@@ -373,6 +377,7 @@ class LocationBarView : public LocationBar,
                            const gfx::Point& p) override;
 
   // PageActionIconView::Delegate:
+  SkColor GetPageActionInkDropColor() const override;
   content::WebContents* GetWebContentsForPageActionIconView() override;
 
   // gfx::AnimationDelegate: