Improve appearance of higlighting in profile chooser view.

chrome/browser/ui/views/profiles/profile_chooser_view.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/profiles/profile_chooser_view.h"
 
 #include "base/macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/vector_icons/vector_icons.h"
@@ skipping 163 matching lines @@
 }
 
 // BackgroundColorHoverButton -------------------------------------------------
 
 // A custom button that allows for setting a background color when hovered over.
 class BackgroundColorHoverButton : public views::LabelButton {
  public:
   BackgroundColorHoverButton(ProfileChooserView* profile_chooser_view,
                              const base::string16& text)
       : views::LabelButton(profile_chooser_view, text),
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-        profile_chooser_view_(profile_chooser_view),
-#endif
         title_(nullptr),
         subtitle_(nullptr) {
     DCHECK(profile_chooser_view);
     SetImageLabelSpacing(kMenuEdgeMargin - 2);
     SetBorder(views::CreateEmptyBorder(0, kMenuEdgeMargin, 0, kMenuEdgeMargin));
     SetFocusForPlatform();
+    SetFocusPainter(nullptr);
 
     label()->SetHandlesTooltips(false);
   }
 
   BackgroundColorHoverButton(ProfileChooserView* profile_chooser_view,
                              const base::string16& text,
                              const gfx::ImageSkia& icon)
       : BackgroundColorHoverButton(profile_chooser_view, text) {
     SetMinSize(gfx::Size(
         icon.width(), kButtonHeight + views::kRelatedControlVerticalSpacing));
     SetImage(STATE_NORMAL, icon);
   }
 
   // Overrides the main label associated with this button.  If unset,
   // label() will be used instead.  |label| should be drawn over this
   // button, but it is not necessary that it be a child view.
   void set_title(views::Label* label) { title_ = label; }
 
   // Sets a secondary label associated with this button.  |label|
   // should be drawn over this button, but it is not necessary that it
   // be a child view.
   void set_subtitle(views::Label* label) { subtitle_ = label; }
 
   ~BackgroundColorHoverButton() override {}
 
  private:
   // views::View:
+  void OnFocus() override {
+    LabelButton::OnFocus();
+    UpdateColors();
+  }
+
+  void OnBlur() override {
+    LabelButton::OnBlur();
+    UpdateColors();
+  }
+
   void OnNativeThemeChanged(const ui::NativeTheme* theme) override {
-    // The first time the theme changes, the state will not be hovered
-    // or pressed and the colors will be initialized.  It's okay to
-    // reset the colors when the theme changes and the button is NOT
-    // hovered or pressed because the labels will be in a normal state.
-    if (state() == STATE_HOVERED || state() == STATE_PRESSED)
-      return;
-
     LabelButton::OnNativeThemeChanged(theme);
-    views::Label* title = title_ ? title_ : label();
-    normal_title_color_ = title->enabled_color();
-    if (subtitle_)
-      normal_subtitle_color_ = subtitle_->disabled_color();
+    UpdateColors();
   }
 
   // views::CustomButton:
   void StateChanged(ButtonState old_state) override {
     LabelButton::StateChanged(old_state);
 
-    auto set_title_color = [&](SkColor color) {
-      if (title_)
-        title_->SetEnabledColor(color);
-      else
-        SetEnabledTextColors(color);
-    };
+    // As in a menu, focus follows the mouse. If we don't do this, the focused
+    // view and the hovered view might both have the selection highlight.
+    if ((state() == STATE_HOVERED || state() == STATE_PRESSED))

[msw, 2017/03/16 01:41:45]

nit: extra parens not needed

[Evan Stade, 2017/03/16 17:28:14]

Done.

+      RequestFocus();
+    else if (state() == STATE_NORMAL && HasFocus())

[msw, 2017/03/16 01:41:45]

q: odd, when does this happen? add a comment?

[Evan Stade, 2017/03/16 17:28:14]

this is the blur-on-unhover part of "focus follows the mouse". I've elaborated
the comment above.

[msw, 2017/03/16 17:40:17]

Acknowledged.

+      GetFocusManager()->SetFocusedView(nullptr);
 
-    bool was_prelight =
-        old_state == STATE_HOVERED || old_state == STATE_PRESSED;
-    bool is_prelight = state() == STATE_HOVERED || state() == STATE_PRESSED;
-    if (was_prelight && !is_prelight) {
-      // The pointer is no longer over this button.  Set the
-      // background and text colors back to their normal states.
-      set_background(nullptr);
-      set_title_color(normal_title_color_);
-      if (subtitle_)
-        subtitle_->SetDisabledColor(normal_subtitle_color_);
-    } else if (!was_prelight && is_prelight) {
-      // The pointer moved over this button.  Set the background and
-      // text colors back to their hovered states.
-      SkColor bg_color = profiles::kHoverColor;
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-      if (ThemeServiceFactory::GetForProfile(
-              profile_chooser_view_->browser()->profile())
-              ->UsingSystemTheme()) {
-        // When using the system (GTK) theme, use the selected menuitem colors.
-        bg_color = GetNativeTheme()->GetSystemColor(
-            ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor);
-        SkColor text_color = GetNativeTheme()->GetSystemColor(
-            ui::NativeTheme::kColorId_SelectedMenuItemForegroundColor);
-        set_title_color(text_color);
-        if (subtitle_)
-          subtitle_->SetDisabledColor(text_color);
-      }
-#endif
-      set_background(views::Background::CreateSolidBackground(bg_color));
+    UpdateColors();
+  }
+
+  void UpdateColors() {

[msw, 2017/03/16 01:41:45]

nit: should this be called from the constructor? (maybe so if we use menu
enabled/disabled colors instead of label colors). maybe add a function comment.

[Evan Stade, 2017/03/16 17:28:14]

It relies on the native theme, and the correct native theme isn't set until this
view is added to a widget, at which point OnNativeThemeChanged will be called
and thus UpdateColors.

[msw, 2017/03/16 17:40:17]

Acknowledged.

+    bool is_selected = HasFocus();
+
+    set_background(
+        is_selected
+            ? views::Background::CreateSolidBackground(
+                  GetNativeTheme()->GetSystemColor(
+                      ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor))
+            : nullptr);
+
+    SkColor text_color = GetNativeTheme()->GetSystemColor(
+        is_selected ? ui::NativeTheme::kColorId_SelectedMenuItemForegroundColor
+                    : ui::NativeTheme::kColorId_LabelEnabledColor);

[msw, 2017/03/16 01:41:45]

nit: kColorId_EnabledMenuItemForegroundColor for consistency?

[Evan Stade, 2017/03/16 17:28:15]

Indeed this is sort of weird, but the background of the avatar bubble menu
thingy is not the same as a menu background, it's a bubble background. The
button/label text should be readable on a normal background, not a menu
background. This actually matters as there are GTK themes (of the ones I have
installed, Numix is one) where a menu is white text on black bg (red bg for
highlight) but black text on a light grey bg for windows/bubbles/dialogs.

[msw, 2017/03/16 17:40:17]

Acknowledged.

+    SetEnabledTextColors(text_color);
+    if (title_)
+      title_->SetEnabledColor(text_color);
+
+    if (subtitle_) {
+      DCHECK(!subtitle_->enabled());
+      subtitle_->SetDisabledColor(GetNativeTheme()->GetSystemColor(
+          is_selected
+              ? ui::NativeTheme::kColorId_DisabledMenuItemForegroundColor
+              : ui::NativeTheme::kColorId_LabelDisabledColor));

[msw, 2017/03/16 01:41:45]

nit: kColorId_DisabledMenuItemForegroundColor for consistency?

     }
   }
 
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-  ProfileChooserView* profile_chooser_view_;
-#endif
-
   views::Label* title_;
-  SkColor normal_title_color_;
-
   views::Label* subtitle_;
-  SkColor normal_subtitle_color_;
 
   DISALLOW_COPY_AND_ASSIGN(BackgroundColorHoverButton);
 };
 
 // SizedContainer -------------------------------------------------
 
 // A simple container view that takes an explicit preferred size.
 class SizedContainer : public views::View {
  public:
   explicit SizedContainer(const gfx::Size& preferred_size)
@@ skipping 1669 matching lines @@
       IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
           IncognitoModePrefs::DISABLED;
   return incognito_available && !browser_->profile()->IsGuestSession();
 }
 
 void ProfileChooserView::PostActionPerformed(
     ProfileMetrics::ProfileDesktopMenu action_performed) {
   ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
   gaia_service_type_ = signin::GAIA_SERVICE_TYPE_NONE;
 }