views: update MdTextButton background on visual state change

If this is not done, when the button's visual state changes, its text
color changes but its background does not. For prominent buttons the
result is very hard to read.

Bug: 1109798
Change-Id: Id00c1cc7d9ec7c5a71ac5e4f72949883497ab025
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321056
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792800}

ui/views/BUILD.gn

@@ -926,6 +926,8 @@ jumbo_source_set("test_support") {
     "test/test_widget_observer.h",
     "test/view_metadata_test_utils.cc",
     "test/view_metadata_test_utils.h",
+    "test/views_drawing_test_utils.cc",
+    "test/views_drawing_test_utils.h",
     "test/views_test_base.cc",
     "test/views_test_base.h",
     "test/views_test_helper.cc",

ui/views/controls/button/label_button.cc

@@ -554,6 +554,7 @@ Button::ButtonState LabelButton::GetVisualState() const {
 
 void LabelButton::VisualStateChanged() {
   UpdateImage();
+  UpdateBackgroundColor();
   label_->SetEnabled(GetVisualState() != STATE_DISABLED);
 }
 

ui/views/controls/button/label_button.h

@@ -148,6 +148,14 @@ class VIEWS_EXPORT LabelButton : public Button, public NativeThemeDelegate {
   // Updates the image view to contain the appropriate button state image.
   void UpdateImage();
 
+  // Updates the background color, if the background color is state-sensitive.
+  virtual void UpdateBackgroundColor() {}
+
+  // Returns the current visual appearance of the button. This takes into
+  // account both the button's underlying state and the state of the containing
+  // widget.
+  ButtonState GetVisualState() const;
+
   // Fills |params| with information about the button.
   virtual void GetExtraParams(ui::NativeTheme::ExtraParams* params) const;
 
@@ -178,11 +186,6 @@ class VIEWS_EXPORT LabelButton : public Button, public NativeThemeDelegate {
   // height as total height, and clamp to min/max sizes as appropriate.
   gfx::Size GetUnclampedSizeWithoutLabel() const;
 
-  // Returns the current visual appearance of the button. This takes into
-  // account both the button's underlying state and the state of the containing
-  // widget.
-  ButtonState GetVisualState() const;
-
   // Updates the portions of the object that might change in response to a
   // change in the value returned by GetVisualState().
   void VisualStateChanged();

ui/views/controls/button/md_text_button.cc

@@ -251,7 +251,7 @@ void MdTextButton::UpdateTextColor() {
 }
 
 void MdTextButton::UpdateBackgroundColor() {
-  bool is_disabled = GetState() == STATE_DISABLED;
+  bool is_disabled = GetVisualState() == STATE_DISABLED;
   ui::NativeTheme* theme = GetNativeTheme();
   SkColor bg_color =
       theme->GetSystemColor(ui::NativeTheme::kColorId_ButtonColor);

ui/views/controls/button/md_text_button.h

@@ -64,7 +64,7 @@ class VIEWS_EXPORT MdTextButton : public LabelButton {
   gfx::Insets CalculateDefaultPadding() const;
 
   void UpdateTextColor();
-  virtual void UpdateBackgroundColor();
+  void UpdateBackgroundColor() override;
   void UpdateColors();
 
   // True if this button uses prominent styling (blue fill, etc.).

ui/views/controls/button/md_text_button_unittest.cc

@@ -4,7 +4,11 @@
 
 #include "ui/views/controls/button/md_text_button.h"
 
+#include "ui/views/background.h"
+#include "ui/views/style/platform_style.h"
+#include "ui/views/test/views_drawing_test_utils.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/test/widget_test.h"
 
 namespace views {
 
@@ -22,4 +26,55 @@ TEST_F(MdTextButtonTest, CustomPadding) {
   EXPECT_EQ(button->GetInsets(), custom_padding);
 }
 
+TEST_F(MdTextButtonTest, BackgroundColorChangesWithWidgetActivation) {
+  // Test whether the button's background color changes when its containing
+  // widget's activation changes.
+  if (!PlatformStyle::kInactiveWidgetControlsAppearDisabled)
+    GTEST_SKIP() << "Button colors do not change with widget activation here.";
+
+  std::unique_ptr<Widget> widget = CreateTestWidget();
+  auto* button = widget->SetContentsView(
+      MdTextButton::Create(nullptr, base::ASCIIToUTF16("button")));
+  button->SetProminent(true);
+  button->SetBounds(0, 0, 70, 20);
+  widget->LayoutRootViewIfNecessary();
+
+  const ui::NativeTheme* native_theme = button->GetNativeTheme();
+
+  test::WidgetTest::SimulateNativeActivate(widget.get());
+  EXPECT_TRUE(widget->IsActive());
+  SkBitmap active_bitmap = views::test::PaintViewToBitmap(button);
+
+  auto background_color = [button](const SkBitmap& bitmap) {
+    // The very edge of the bitmap contains the button's border, which we aren't
+    // interested in here. Instead, grab a pixel that is inset by the button's
+    // corner radius from the top-left point to avoid the border.
+    //
+    // It would make a bit more sense to inset by the border thickness or
+    // something, but MdTextButton doesn't expose (or even know) that value
+    // without some major abstraction violation.
+    int corner_radius = button->GetCornerRadius();
+    return bitmap.getColor(corner_radius, corner_radius);
+  };
+
+  EXPECT_EQ(background_color(active_bitmap),
+            native_theme->GetSystemColor(
+                ui::NativeTheme::kColorId_ProminentButtonColor));
+
+  // It would be neat to also check the text color here, but the label's text
+  // ends up drawn on top of the background with antialiasing, which means there
+  // aren't any pixels that are actually *exactly*
+  // kColorId_TextOnProminentButtonColor. Bummer.
+
+  // Activate another widget to cause the original widget to deactivate.
+  std::unique_ptr<Widget> other_widget = CreateTestWidget();
+  test::WidgetTest::SimulateNativeActivate(other_widget.get());
+  EXPECT_FALSE(widget->IsActive());
+  SkBitmap inactive_bitmap = views::test::PaintViewToBitmap(button);
+
+  EXPECT_EQ(background_color(inactive_bitmap),
+            native_theme->GetSystemColor(
+                ui::NativeTheme::kColorId_ProminentButtonDisabledColor));
+}
+
 }  // namespace views

ui/views/test/views_drawing_test_utils.cc

+// Copyright 2020 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/test/views_drawing_test_utils.h"
+
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/compositor/canvas_painter.h"
+#include "ui/gfx/color_palette.h"
+#include "ui/views/paint_info.h"
+#include "ui/views/view.h"
+
+namespace views {
+namespace test {
+
+SkBitmap PaintViewToBitmap(View* view) {
+  SkBitmap bitmap;
+  {
+    gfx::Size size = view->size();
+    ui::CanvasPainter canvas_painter(&bitmap, size, 1.f, gfx::kPlaceholderColor,
+                                     false);
+    view->Paint(PaintInfo::CreateRootPaintInfo(canvas_painter.context(), size));
+  }
+  return bitmap;
+}
+
+}  // namespace test
+}  // namespace views

ui/views/test/views_drawing_test_utils.h

+// Copyright 2020 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.
+
+#ifndef UI_VIEWS_TEST_VIEWS_DRAWING_TEST_UTILS_H_
+#define UI_VIEWS_TEST_VIEWS_DRAWING_TEST_UTILS_H_
+
+class SkBitmap;
+
+namespace views {
+class View;
+
+namespace test {
+
+// Paints the provided View and all its children to an SkBitmap, exactly like
+// the view would be painted in actual use. This includes antialiasing of text,
+// graphical effects, hover state, focus rings, and so on.
+SkBitmap PaintViewToBitmap(View* view);
+
+}  // namespace test
+}  // namespace views
+
+#endif  // UI_VIEWS_TEST_VIEWS_DRAWING_TEST_UTILS_H_