MacViews: Style BUTTON_STYLE buttons using the "modern" UI

ui/views/controls/button/label_button.cc

 // Copyright (c) 2012 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 "ui/views/controls/button/label_button.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 12 matching lines @@
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/painter.h"
 #include "ui/views/style/platform_style.h"
 #include "ui/views/window/dialog_delegate.h"
 
 namespace {
 
 // The default spacing between the icon and text.
 const int kSpacing = 5;
 
-#if !(defined(OS_LINUX) && !defined(OS_CHROMEOS))
-// Default text and shadow colors for STYLE_BUTTON.
-const SkColor kStyleButtonTextColor = SK_ColorBLACK;
-const SkColor kStyleButtonShadowColor = SK_ColorWHITE;
-#endif
-
 const gfx::FontList& GetDefaultNormalFontList() {
   static base::LazyInstance<gfx::FontList>::Leaky font_list =
       LAZY_INSTANCE_INITIALIZER;
   return font_list.Get();
 }
 
 const gfx::FontList& GetDefaultBoldFontList() {
   static base::LazyInstance<gfx::FontList>::Leaky font_list =
       LAZY_INSTANCE_INITIALIZER;
   if ((font_list.Get().GetFontStyle() & gfx::Font::BOLD) == 0) {
@@ skipping 109 matching lines @@
 }
 
 const gfx::FontList& LabelButton::GetFontList() const {
   return label_->font_list();
 }
 
 void LabelButton::SetFontList(const gfx::FontList& font_list) {
   cached_normal_font_list_ = font_list;
   cached_bold_font_list_ = font_list.DeriveWithStyle(
       font_list.GetFontStyle() | gfx::Font::BOLD);
-  label_->SetFontList(is_default_ ?
-      cached_bold_font_list_ : cached_normal_font_list_);
+  bool bold = PlatformStyle::kDefaultLabelButtonHasBoldFont && is_default_;

[sky, 2016/04/07 17:16:31]

Seems like you can skip cached_bold_font_list_ entirely if
kDefaultLabelButtonHasBoldFont is false. Maybe make cached_bold_font_list_  a
scoped_ptr?

[tapted, 2016/04/08 08:35:33]

gfx::FontList is itself just a scoped_refptr, and default-constructing a
gfx::FontList is cheap now that it just references `g_default_impl` in
font_list.cc. So a scoped_ptr might not add much. However, you're right that I'm
missing out on cases where it's fine to leave cached_bold_font_list_ untouched,
so I've fixed that.


(I guess an orthogonal question is whether the caching really adds much on any
platform, since it should be rare for a button to switch between
SetIsDefault(true/false)...)

[sky, 2016/04/08 16:24:22]

I think you're right and am fine to nuke the caching. I think that will simplify
the code too.

+  label_->SetFontList(bold ? cached_bold_font_list_ : cached_normal_font_list_);
 }
 
 void LabelButton::SetElideBehavior(gfx::ElideBehavior elide_behavior) {
   label_->SetElideBehavior(elide_behavior);
 }
 
 void LabelButton::SetHorizontalAlignment(gfx::HorizontalAlignment alignment) {
   DCHECK_NE(gfx::ALIGN_TO_HEAD, alignment);
   horizontal_alignment_ = alignment;
   InvalidateLayout();
@@ skipping 11 matching lines @@
 
 void LabelButton::SetIsDefault(bool is_default) {
   DCHECK_EQ(STYLE_BUTTON, style_);
   if (is_default == is_default_)
     return;
 
   is_default_ = is_default;
   ui::Accelerator accel(ui::VKEY_RETURN, ui::EF_NONE);
   is_default_ ? AddAccelerator(accel) : RemoveAccelerator(accel);
 
-  label_->SetFontList(
-      is_default ? cached_bold_font_list_ : cached_normal_font_list_);
+  bool bold = PlatformStyle::kDefaultLabelButtonHasBoldFont && is_default_;
+  label_->SetFontList(bold ? cached_bold_font_list_ : cached_normal_font_list_);
   InvalidateLayout();
+  HighlightStateChanged();
 }
 
 void LabelButton::SetStyle(ButtonStyle style) {
   // All callers currently pass STYLE_BUTTON, and should only call this once, to
   // change from the default style.
   DCHECK_EQ(style, STYLE_BUTTON);
   DCHECK_EQ(style_, STYLE_TEXTBUTTON);
   DCHECK(!GetWidget()) << "Can't change button style after adding to a Widget.";
 
   style_ = style;
 
   SetFocusPainter(nullptr);
   SetHorizontalAlignment(gfx::ALIGN_CENTER);
   SetFocusable(true);
-  SetMinSize(gfx::Size(70, 33));
+  SetMinSize(gfx::Size(PlatformStyle::kMinLabelButtonWidth,
+                       PlatformStyle::kMinLabelButtonHeight));
 
   // Themed borders will be set once the button is added to a Widget, since that
   // provides the value of GetNativeTheme().
 }
 
 void LabelButton::SetImageLabelSpacing(int spacing) {
   if (spacing == image_label_spacing_)
     return;
   image_label_spacing_ = spacing;
   ResetCachedPreferredSize();
   InvalidateLayout();
 }
 
 void LabelButton::SetFocusPainter(scoped_ptr<Painter> focus_painter) {
   focus_painter_ = std::move(focus_painter);
 }
 
 gfx::Size LabelButton::GetPreferredSize() const {
   if (cached_preferred_size_valid_)
     return cached_preferred_size_;
 
   // Use a temporary label copy for sizing to avoid calculation side-effects.
   Label label(GetText(), cached_normal_font_list_);
   label.SetShadows(label_->shadows());
 
-  if (style() == STYLE_BUTTON) {
+  if (style_ == STYLE_BUTTON && PlatformStyle::kDefaultLabelButtonHasBoldFont) {
     // Some text appears wider when rendered normally than when rendered bold.
     // Accommodate the widest, as buttons may show bold and shouldn't resize.
     const int current_width = label.GetPreferredSize().width();
     label.SetFontList(cached_bold_font_list_);
     if (label.GetPreferredSize().width() < current_width)
       label.SetFontList(cached_normal_font_list_);
   }
 
   // Calculate the required size.
   const gfx::Size image_size(image_->GetPreferredSize());
@@ skipping 134 matching lines @@
 
 void LabelButton::OnBlur() {
   CustomButton::OnBlur();
   // Typically the border renders differently when focused.
   SchedulePaint();
 }
 
 void LabelButton::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   ResetColorsFromNativeTheme();
   UpdateThemedBorder();
+  HighlightStateChanged();
   // Invalidate the layout to pickup the new insets from the border.
   InvalidateLayout();
 }
 
 void LabelButton::AddInkDropLayer(ui::Layer* ink_drop_layer) {
   image()->SetPaintToLayer(true);
   image()->layer()->SetFillsBoundsOpaquely(false);
   ink_drop_container_->SetVisible(true);
   ink_drop_container_->layer()->Add(ink_drop_layer);
 }
@@ skipping 24 matching lines @@
 gfx::Point LabelButton::GetInkDropCenter() const {
   // TODO(bruthig): Make the flood fill ink drops centered on the LocatedEvent
   // that triggered them.
   return GetText().empty() ? image()->GetMirroredBounds().CenterPoint()
                            : CustomButton::GetInkDropCenter();
 }
 
 void LabelButton::StateChanged() {
   const gfx::Size previous_image_size(image_->GetPreferredSize());
   UpdateImage();
-  const SkColor color = button_state_colors_[state()];
-  if (state() != STATE_DISABLED && label_->enabled_color() != color)
-    label_->SetEnabledColor(color);
+  HighlightStateChanged();
   label_->SetEnabled(state() != STATE_DISABLED);
   if (image_->GetPreferredSize() != previous_image_size)
     Layout();
 }
 
 void LabelButton::GetExtraParams(ui::NativeTheme::ExtraParams* params) const {
   params->button.checked = false;
   params->button.indeterminate = false;
   params->button.is_default = is_default_;
   params->button.is_focused = HasFocus() && IsAccessibilityFocusable();
   params->button.has_border = false;
   params->button.classic_state = 0;
   params->button.background_color = label_->background_color();
 }
 
 void LabelButton::ResetColorsFromNativeTheme() {
   const ui::NativeTheme* theme = GetNativeTheme();
   SkColor colors[STATE_COUNT] = {
     theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonEnabledColor),
     theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonHoverColor),
     theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonHoverColor),
     theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonDisabledColor),
   };
 
-  // Certain styles do not change text color when hovered or pressed.
-  bool constant_text_color = false;
   // Use hardcoded colors for inverted color scheme support and STYLE_BUTTON.
   if (color_utils::IsInvertedColorScheme()) {
-    constant_text_color = true;
     colors[STATE_NORMAL] = SK_ColorWHITE;
+    colors[STATE_HOVERED] = colors[STATE_PRESSED] = colors[STATE_NORMAL];

[sky, 2016/04/07 17:16:31]

nit: a single assignment setting all to WHITE is more readable.

[tapted, 2016/04/08 08:35:33]

Done.

     label_->SetBackgroundColor(SK_ColorBLACK);
     label_->set_background(Background::CreateSolidBackground(SK_ColorBLACK));
     label_->SetAutoColorReadabilityEnabled(true);
     label_->SetShadows(gfx::ShadowValues());
   } else if (style() == STYLE_BUTTON) {
-    // TODO(erg): This is disabled on desktop linux because of the binary asset
-    // confusion. These details should either be pushed into ui::NativeThemeWin
-    // or should be obsoleted by rendering buttons with paint calls instead of
-    // with static assets. http://crbug.com/350498
-#if !(defined(OS_LINUX) && !defined(OS_CHROMEOS))
-    constant_text_color = true;
-    colors[STATE_NORMAL] = kStyleButtonTextColor;
-    label_->SetBackgroundColor(theme->GetSystemColor(
-        ui::NativeTheme::kColorId_ButtonBackgroundColor));
-    label_->SetAutoColorReadabilityEnabled(false);
-    label_->SetShadows(gfx::ShadowValues(
-        1, gfx::ShadowValue(gfx::Vector2d(0, 1), 0, kStyleButtonShadowColor)));
-#endif
-    label_->set_background(NULL);
+    PlatformStyle::ApplyLabelButtonTextStyle(label_, &colors);
+    label_->set_background(nullptr);
   } else {
-    label_->set_background(NULL);
+    label_->set_background(nullptr);
   }
 
-  if (constant_text_color)
-    colors[STATE_HOVERED] = colors[STATE_PRESSED] = colors[STATE_NORMAL];
-
   for (size_t state = STATE_NORMAL; state < STATE_COUNT; ++state) {
     if (!explicitly_set_colors_[state]) {
       SetTextColor(static_cast<ButtonState>(state), colors[state]);
       explicitly_set_colors_[state] = false;
     }
   }
 }
 
 void LabelButton::UpdateImage() {
   image_->SetImage(GetImage(state()));
@@ skipping 54 matching lines @@
     ui::NativeTheme::ExtraParams* params) const {
   GetExtraParams(params);
   return ui::NativeTheme::kHovered;
 }
 
 void LabelButton::ResetCachedPreferredSize() {
   cached_preferred_size_valid_ = false;
   cached_preferred_size_ = gfx::Size();
 }
 
+void LabelButton::HighlightStateChanged() {

[sky, 2016/04/07 17:16:31]

This function is confusingly named as there is no 'highlight' state, and it's
invoked at random ties. I think a more specific name would be better, perhaps
ResetLabelEnabledColor().

[tapted, 2016/04/08 08:35:33]

Done.

+  const SkColor color =

[sky, 2016/04/07 17:16:31]

If you're going to use const (which is good), be consistent. You don't use it
for 'bool bold' in this file when you could as well.

[tapted, 2016/04/08 08:35:33]

Done.

+      explicitly_set_colors_[state()]
+          ? button_state_colors_[state()]
+          : PlatformStyle::TextColorForButton(button_state_colors_, *this);
+  if (state() != STATE_DISABLED && label_->enabled_color() != color)
+    label_->SetEnabledColor(color);
+}
+
 }  // namespace views