Removed slide animation from location_icon_view

Removed slide animation from location_icon_view and changed it so it
uses the animation in its parent class, this fixes a bug where after
hiding the security chip it wouldn't be shown again.

Bug: 875182
Change-Id: I75a7eec4168d80684a11e335639d0a278b8391e5
Reviewed-on: https://chromium-review.googlesource.com/1187658
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587639}

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

@@ -47,16 +47,11 @@ constexpr int kIconLabelBubbleFadeOutDurationMs = 175;
 // The type of tweening for the animation.
 const gfx::Tween::Type kIconLabelBubbleTweenType = gfx::Tween::EASE_IN_OUT;
 
-// The time for the text to animate out, as well as in.
-constexpr int kIconLabelBubbleSlideTimeMs = 600;
-
 // The total time for the in and out text animation.
 constexpr int kIconLabelBubbleAnimationDurationMs = 3000;
 
-// The fraction of time taken for the text to animate out, as well as in.
-const double kIconLabelBubbleOpenTimeFraction =
-    static_cast<double>(kIconLabelBubbleSlideTimeMs) /
-    kIconLabelBubbleAnimationDurationMs;
+// The ratio of text animation duration to total animation duration.
+const double kIconLabelBubbleOpenTimeFraction = 0.2;
 }  // namespace
 
 //////////////////////////////////////////////////////////////////
@@ -413,7 +408,6 @@ void IconLabelBubbleView::OnBlur() {
 }
 
 void IconLabelBubbleView::AnimationEnded(const gfx::Animation* animation) {
-  slide_animation_.Reset();
   if (!is_animation_paused_) {
     // If there is no separator to show, then that means we want the text to
     // disappear after animating.
@@ -515,6 +509,10 @@ int IconLabelBubbleView::GetWidthBetweenIconAndSeparator() const {
              : 0;
 }
 
+int IconLabelBubbleView::GetSlideDurationTime() const {
+  return kIconLabelBubbleAnimationDurationMs;
+}
+
 int IconLabelBubbleView::GetEndPaddingWithSeparator() const {
   int end_padding = ShouldShowSeparator() ? kIconLabelBubbleSpaceBesideSeparator
                                           : GetInsets().right();
@@ -543,21 +541,33 @@ void IconLabelBubbleView::SetUpForInOutAnimation() {
   image_->EnableCanvasFlippingForRTLUI(true);
   label_->SetElideBehavior(gfx::NO_ELIDE);
   label_->SetVisible(false);
-
-  slide_animation_.SetSlideDuration(kIconLabelBubbleAnimationDurationMs);
+  slide_animation_.SetSlideDuration(GetSlideDurationTime());
   slide_animation_.SetTweenType(kIconLabelBubbleTweenType);
   open_state_fraction_ = gfx::Tween::CalculateValue(
       kIconLabelBubbleTweenType, kIconLabelBubbleOpenTimeFraction);
 }
 
-void IconLabelBubbleView::AnimateIn(int string_id) {
+void IconLabelBubbleView::AnimateIn(base::Optional<int> string_id) {
   if (!label()->visible()) {
-    SetLabel(l10n_util::GetStringUTF16(string_id));
+    if (string_id)
+      SetLabel(l10n_util::GetStringUTF16(string_id.value()));
     label()->SetVisible(true);
     ShowAnimation();
   }
 }
 
+void IconLabelBubbleView::AnimateOut() {
+  if (label()->visible()) {
+    label()->SetVisible(false);
+    HideAnimation();
+  }
+}
+
+void IconLabelBubbleView::ResetSlideAnimation(bool show) {
+  label()->SetVisible(show);
+  slide_animation_.Reset(show);
+}
+
 void IconLabelBubbleView::PauseAnimation() {
   if (slide_animation_.is_animating()) {
     // If the user clicks while we're animating, the bubble arrow will be
@@ -585,8 +595,18 @@ void IconLabelBubbleView::UnpauseAnimation() {
   }
 }
 
+double IconLabelBubbleView::GetAnimationValue() const {
+  return slide_animation_.GetCurrentValue();
+}
+
 void IconLabelBubbleView::ShowAnimation() {
   slide_animation_.Show();
   GetInkDrop()->SetShowHighlightOnHover(false);
   GetInkDrop()->SetShowHighlightOnFocus(false);
 }
+
+void IconLabelBubbleView::HideAnimation() {
+  slide_animation_.Hide();
+  GetInkDrop()->SetShowHighlightOnHover(false);
+  GetInkDrop()->SetShowHighlightOnFocus(false);
+}

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

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/size.h"
@@ -168,14 +169,26 @@ class IconLabelBubbleView : public views::InkDropObserver,
   // Set up for icons that animate their labels in and then out.
   void SetUpForInOutAnimation();
 
-  // Animates the view in and disables highlighting for hover and focus.
+  // Animates the view in and disables highlighting for hover and focus. If a
+  // |string_id| is provided it also sets/changes the label to that string.
   // TODO(bruthig): See https://crbug.com/669253. Since the ink drop highlight
   // currently cannot handle host resizes, the highlight needs to be disabled
   // when the animation is running.
-  void AnimateIn(int string_id);
+  void AnimateIn(base::Optional<int> string_id);
+
+  // Animates the view out.
+  void AnimateOut();
+
   void PauseAnimation();
   void UnpauseAnimation();
 
+  // Returns the current value of the slide animation
+  double GetAnimationValue() const;
+
+  // Sets the slide animation value without animating. |show| determines if
+  // the animation is set to fully shown or fully hidden.
+  void ResetSlideAnimation(bool show);
+
   // Returns true iff the slide animation has started, has not ended and is
   // currently paused.
   bool is_animation_paused() const { return is_animation_paused_; }
@@ -189,6 +202,10 @@ class IconLabelBubbleView : public views::InkDropObserver,
   // to the suggestion text, like in the SelectedKeywordView.
   virtual int GetExtraInternalSpacing() const;
 
+  // Subclasses that want a different duration for the slide animation can
+  // override this method.
+  virtual int GetSlideDurationTime() const;
+
   // Padding after the separator. If this separator is shown, this includes the
   // separator width.
   int GetEndPaddingWithSeparator() const;
@@ -200,8 +217,14 @@ class IconLabelBubbleView : public views::InkDropObserver,
   // views::View:
   const char* GetClassName() const override;
 
+  // Disables highlights and calls Show on the slide animation, should not be
+  // called directly, use AnimateIn() instead, which handles label visibility.
   void ShowAnimation();
 
+  // Disables highlights and calls Hide on the slide animation, should not be
+  // called directly, use AnimateIn() instead, which handles label visibility.
+  void HideAnimation();
+
   // The contents of the bubble.
   views::ImageView* image_;
   views::Label* label_;

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

@@ -4,10 +4,12 @@
 
 #include "chrome/browser/ui/views/location_bar/icon_label_bubble_view.h"
 
+#include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/test/views/chrome_views_test_base.h"
+#include "components/strings/grit/components_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/gesture_detection/gesture_configuration.h"
@@ -34,6 +36,10 @@ const int kNumberOfSteps = 300;
 
 class TestIconLabelBubbleView : public IconLabelBubbleView {
  public:
+  using IconLabelBubbleView::AnimateIn;
+  using IconLabelBubbleView::AnimateOut;
+  using IconLabelBubbleView::ResetSlideAnimation;
+
   enum State {
     GROWING,
     STEADY,
@@ -361,6 +367,37 @@ TEST_F(IconLabelBubbleViewTest, GestureInkDropState) {
 }
 #endif
 
+TEST_F(IconLabelBubbleViewTest, LabelVisibilityAfterAnimation) {
+  view()->AnimateIn(base::nullopt);
+  EXPECT_TRUE(view()->IsLabelVisible());
+  view()->AnimateOut();
+  EXPECT_FALSE(view()->IsLabelVisible());
+  // Label should reappear if animated in after being animated out.
+  view()->AnimateIn(base::nullopt);
+  EXPECT_TRUE(view()->IsLabelVisible());
+}
+
+TEST_F(IconLabelBubbleViewTest, LabelVisibilityAfterAnimationReset) {
+  view()->ResetSlideAnimation(true);
+  EXPECT_TRUE(view()->IsLabelVisible());
+  view()->ResetSlideAnimation(false);
+  EXPECT_FALSE(view()->IsLabelVisible());
+  // Label should reappear if animated in after being reset out.
+  view()->AnimateIn(base::nullopt);
+  EXPECT_TRUE(view()->IsLabelVisible());
+}
+
+TEST_F(IconLabelBubbleViewTest,
+       LabelVisibilityAfterAnimationWithDefinedString) {
+  view()->AnimateIn(IDS_AUTOFILL_CARD_SAVED);
+  EXPECT_TRUE(view()->IsLabelVisible());
+  view()->AnimateOut();
+  EXPECT_FALSE(view()->IsLabelVisible());
+  // Label should reappear if animated in after being animated out.
+  view()->AnimateIn(IDS_AUTOFILL_CARD_SAVED);
+  EXPECT_TRUE(view()->IsLabelVisible());
+}
+
 #if defined(OS_CHROMEOS)
 // Verifies IconLabelBubbleView::CalculatePreferredSize() doesn't crash when
 // there is a widget but no compositor.

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

@@ -23,14 +23,11 @@ using content::WebContents;
 
 LocationIconView::LocationIconView(const gfx::FontList& font_list,
                                    LocationBarView* location_bar)
-    : IconLabelBubbleView(font_list),
-      location_bar_(location_bar),
-      animation_(this) {
+    : IconLabelBubbleView(font_list), location_bar_(location_bar) {
   label()->SetElideBehavior(gfx::ELIDE_MIDDLE);
   set_id(VIEW_ID_LOCATION_ICON);
   Update();
-
-  animation_.SetSlideDuration(kOpenTimeMS);
+  SetUpForInOutAnimation();
 }
 
 LocationIconView::~LocationIconView() {
@@ -141,13 +138,13 @@ gfx::Size LocationIconView::GetMinimumSizeForLabelText(
 
 void LocationIconView::SetTextVisibility(bool should_show,
                                          bool should_animate) {
-  if (!should_animate) {
-    animation_.Reset(should_show);
-  } else if (should_show) {
-    animation_.Show();
-  } else {
-    animation_.Hide();
-  }
+  if (!should_animate)
+    ResetSlideAnimation(should_show);
+  else if (should_show)
+    AnimateIn(base::nullopt);
+  else
+    AnimateOut();
+
   // The label text color may have changed.
   OnNativeThemeChanged(GetNativeTheme());
 }
@@ -188,12 +185,7 @@ bool LocationIconView::IsTriggerableEvent(const ui::Event& event) {
 }
 
 double LocationIconView::WidthMultiplier() const {
-  return animation_.GetCurrentValue();
-}
-
-void LocationIconView::AnimationProgressed(const gfx::Animation*) {
-  location_bar_->Layout();
-  location_bar_->SchedulePaint();
+  return GetAnimationValue();
 }
 
 gfx::Size LocationIconView::GetMinimumSizeForPreferredSize(
@@ -203,3 +195,8 @@ gfx::Size LocationIconView::GetMinimumSizeForPreferredSize(
       GetSizeForLabelWidth(font_list().GetExpectedTextWidth(kMinCharacters)));
   return size;
 }
+
+int LocationIconView::GetSlideDurationTime() const {
+  constexpr int kSlideDurationTimeMs = 150;
+  return kSlideDurationTimeMs;
+}

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

@@ -7,8 +7,6 @@
 
 #include "base/macros.h"
 #include "chrome/browser/ui/views/location_bar/icon_label_bubble_view.h"
-#include "ui/gfx/animation/animation_delegate.h"
-#include "ui/gfx/animation/slide_animation.h"
 
 class LocationBarView;
 
@@ -53,22 +51,18 @@ class LocationIconView : public IconLabelBubbleView {
  protected:
   // IconLabelBubbleView:
   bool IsTriggerableEvent(const ui::Event& event) override;
-
- private:
-  // IconLabelBubbleView:
   double WidthMultiplier() const override;
 
-  // gfx::AnimationDelegate:
-  void AnimationProgressed(const gfx::Animation*) override;
-
+ private:
   // Returns what the minimum size would be if the preferred size were |size|.
   gfx::Size GetMinimumSizeForPreferredSize(gfx::Size size) const;
 
+  int GetSlideDurationTime() const override;
+
   // True if hovering this view should display a tooltip.
   bool show_tooltip_;
 
   LocationBarView* location_bar_;
-  gfx::SlideAnimation animation_;
 
   DISALLOW_COPY_AND_ASSIGN(LocationIconView);
 };