Fixed a visual artifact on ToolbarButton's border.

Fixed a visual artifact on ToolbarButton where the insets of the
border may not have updated correctly, depending on how the
animation timing ended up resolving.
This was caused by not recomputing the border when the height of
the ToolbarButton changed, resulting in incorrect border insets.

Bug: 1053917
Change-Id: Ia15ffaf7a5d08f75b4d88bc61deb7bcc3382df2d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067384
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745477}

chrome/browser/ui/views/toolbar/toolbar_button.cc

@@ -152,7 +152,8 @@ void ToolbarButton::UpdateColorsAndInsets() {
   base::Optional<SkColor> border_color =
       highlight_color_animation_.GetBorderColor();
   if (!border() || target_insets != border()->GetInsets() ||
-      last_border_color_ != border_color) {
+      last_border_color_ != border_color ||
+      last_paint_insets_ != paint_insets) {
     if (border_color) {
       int border_thickness_dp = GetText().empty()
                                     ? kBorderThicknessDpWithoutLabel
@@ -168,6 +169,7 @@ void ToolbarButton::UpdateColorsAndInsets() {
       SetBorder(views::CreateEmptyBorder(target_insets));
     }
     last_border_color_ = border_color;
+    last_paint_insets_ = paint_insets;
   }
 
   // Update spacing on the outer-side of the label to match the current

chrome/browser/ui/views/toolbar/toolbar_button.h

@@ -188,6 +188,8 @@ class ToolbarButton : public views::LabelButton,
     void AnimationProgressed(const gfx::Animation* animation) override;
 
    private:
+    friend test::ToolbarButtonTestApi;
+
     // Returns whether the animation is currently shown. Note that this returns
     // true even after calling Hide() until the fade-out animation finishes.
     bool IsShown() const;
@@ -269,7 +271,11 @@ class ToolbarButton : public views::LabelButton,
   // |this| to refresh UI).
   HighlightColorAnimation highlight_color_animation_;
 
+  // If either |last_border_color_| or |last_paint_insets_| have changed since
+  // the last update to |border_| it must be recalculated  to match current
+  // values.
   base::Optional<SkColor> last_border_color_;
+  gfx::Insets last_paint_insets_;
 
   // A factory for tasks that show the dropdown context menu for the button.
   base::WeakPtrFactory<ToolbarButton> show_menu_factory_{this};

chrome/browser/ui/views/toolbar/toolbar_button_interactive_uitest.cc

@@ -4,9 +4,11 @@
 
 #include "chrome/browser/ui/views/toolbar/toolbar_button.h"
 
+#include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/test/view_event_test_base.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/view_class_properties.h"
 
 namespace test {
 
@@ -19,6 +21,19 @@ class ToolbarButtonTestApi {
 
   views::MenuRunner* menu_runner() { return button_->menu_runner_.get(); }
   bool menu_showing() const { return button_->menu_showing_; }
+  const gfx::Insets last_paint_insets() const {
+    return button_->last_paint_insets_;
+  }
+  const gfx::Insets layout_inset_delta() const {
+    return button_->layout_inset_delta_;
+  }
+  const base::Optional<SkColor> last_border_color() const {
+    return button_->last_border_color_;
+  }
+  void SetAnimationTimingForTesting() {
+    button_->highlight_color_animation_.highlight_color_animation_
+        .SetSlideDuration(base::TimeDelta());
+  }
 
  private:
   ToolbarButton* button_;
@@ -26,9 +41,26 @@ class ToolbarButtonTestApi {
 
 }  // namespace test
 
+class TestToolbarButton : public ToolbarButton {
+ public:
+  using ToolbarButton::ToolbarButton;
+
+  void ResetBorderUpdateFlag() { did_border_update_ = false; }
+  bool did_border_update() const { return did_border_update_; }
+
+  // views::ToolbarButton
+  void SetBorder(std::unique_ptr<views::Border> b) override {
+    ToolbarButton::SetBorder(std::move(b));
+    did_border_update_ = true;
+  }
+
+ private:
+  bool did_border_update_ = false;
+};
+
 class ToolbarButtonUITest : public ViewEventTestBase {
  public:
-  ToolbarButtonUITest() {}
+  ToolbarButtonUITest() = default;
   ToolbarButtonUITest(const ToolbarButtonUITest&) = delete;
   ToolbarButtonUITest& operator=(const ToolbarButtonUITest&) = delete;
 
@@ -39,13 +71,13 @@ class ToolbarButtonUITest : public ViewEventTestBase {
     // ToolbarButton takes ownership of the |model|.
     auto model = std::make_unique<ui::SimpleMenuModel>(nullptr);
     model->AddItem(0, base::string16());
-    button_ = new ToolbarButton(nullptr, std::move(model), nullptr);
+    button_ = new TestToolbarButton(nullptr, std::move(model), nullptr);
     return button_;
   }
   void DoTestOnMessageLoop() override {}
 
  protected:
-  ToolbarButton* button_ = nullptr;
+  TestToolbarButton* button_ = nullptr;
 };
 
 // Test showing and dismissing a menu to verify menu delegate lifetime.
@@ -80,3 +112,33 @@ TEST_F(ToolbarButtonUITest, DeleteWithMenu) {
   EXPECT_TRUE(test::ToolbarButtonTestApi(button_).menu_runner());
   delete button_;
 }
+
+// Tests to make sure the button's border is updated as its height changes.
+TEST_F(ToolbarButtonUITest, TestBorderUpdateHeightChange) {
+  const gfx::Insets toolbar_padding = GetLayoutInsets(TOOLBAR_BUTTON);
+
+  button_->ResetBorderUpdateFlag();
+  for (int bounds_height : {8, 12, 20}) {
+    EXPECT_FALSE(button_->did_border_update());
+    button_->SetBoundsRect({bounds_height, bounds_height});
+    EXPECT_TRUE(button_->did_border_update());
+    EXPECT_EQ(button_->border()->GetInsets(), gfx::Insets(toolbar_padding));
+    button_->ResetBorderUpdateFlag();
+  }
+}
+
+// Tests to make sure the button's border color is updated as its animation
+// color changes.
+TEST_F(ToolbarButtonUITest, TestBorderUpdateColorChange) {
+  test::ToolbarButtonTestApi test_api(button_);
+  test_api.SetAnimationTimingForTesting();
+
+  button_->ResetBorderUpdateFlag();
+  for (SkColor border_color : {SK_ColorRED, SK_ColorGREEN, SK_ColorBLUE}) {
+    EXPECT_FALSE(button_->did_border_update());
+    button_->SetHighlight(base::string16(), border_color);
+    EXPECT_EQ(button_->border()->color(), border_color);
+    EXPECT_TRUE(button_->did_border_update());
+    button_->ResetBorderUpdateFlag();
+  }
+}