Fix highlight state getting stuck on after menus close

BUG=919057,916410,934213
R=mohsen

Change-Id: If60bc5958d07fec526b04493f80cb47570b9d00a
Reviewed-on: https://chromium-review.googlesource.com/c/1432880
Auto-Submit: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636713}

ui/views/animation/ink_drop_impl.cc

@@ -351,10 +351,14 @@ void InkDropImpl::HideHighlightOnRippleHiddenState::AnimationStarted(
     // |ink_drop_ripple_|.
     // TODO(bruthig): Investigate if the animation framework can address this
     // issue instead. See https://crbug.com/663335.
-    if (GetInkDrop()->ink_drop_ripple_)
-      GetInkDrop()->ink_drop_ripple_->SnapToHidden();
-    GetInkDrop()->SetHighlightState(
-        state_factory()->CreateVisibleState(base::TimeDelta(), false));
+    InkDropImpl* ink_drop = GetInkDrop();
+    HighlightStateFactory* highlight_state_factory = state_factory();
+    if (ink_drop->ink_drop_ripple_)
+      ink_drop->ink_drop_ripple_->SnapToHidden();
+    // |this| may be destroyed after SnapToHidden(), so be sure not to access
+    // |any members.
+    ink_drop->SetHighlightState(
+        highlight_state_factory->CreateVisibleState(base::TimeDelta(), false));
   }
 }
 
@@ -629,7 +633,9 @@ void InkDropImpl::HostSizeChanged(const gfx::Size& new_size) {
   root_layer_->SetBounds(gfx::Rect(new_size));
 
   const bool create_ink_drop_ripple = !!ink_drop_ripple_;
-  const InkDropState state = GetTargetInkDropState();
+  InkDropState state = GetTargetInkDropState();
+  if (ShouldAnimateToHidden(state))
+    state = views::InkDropState::HIDDEN;
   DestroyInkDropRipple();
 
   if (highlight_) {

ui/views/animation/ink_drop_ripple.cc

@@ -39,16 +39,7 @@ void InkDropRipple::AnimateToState(InkDropState ink_drop_state) {
   // and these invalid transitions will be logged as warnings in
   // AnimateStateChange().
 
-  // |animation_observer| will be deleted when AnimationEndedCallback() returns
-  // true.
-  // TODO(bruthig): Implement a safer ownership model for the
-  // |animation_observer|.
-  ui::CallbackLayerAnimationObserver* animation_observer =
-      new ui::CallbackLayerAnimationObserver(
-          base::Bind(&InkDropRipple::AnimationStartedCallback,
-                     base::Unretained(this), ink_drop_state),
-          base::Bind(&InkDropRipple::AnimationEndedCallback,
-                     base::Unretained(this), ink_drop_state));
+  animation_observer_ = CreateAnimationObserver(ink_drop_state);
 
   InkDropState old_ink_drop_state = target_ink_drop_state_;
   // Assign to |target_ink_drop_state_| before calling AnimateStateChange() so
@@ -62,41 +53,27 @@ void InkDropRipple::AnimateToState(InkDropState ink_drop_state) {
   }
 
   AnimateStateChange(old_ink_drop_state, target_ink_drop_state_,
-                     animation_observer);
-  animation_observer->SetActive();
-  // |this| may be deleted! |animation_observer| might synchronously call
+                     animation_observer_.get());
+  animation_observer_->SetActive();
+  // |this| may be deleted! |animation_observer_| might synchronously call
   // AnimationEndedCallback which can delete |this|.
 }
 
 void InkDropRipple::SnapToState(InkDropState ink_drop_state) {
-  switch (ink_drop_state) {
-    case InkDropState::HIDDEN:
-      SnapToHidden();
-      break;
-    case InkDropState::ACTIVATED:
-      SnapToActivated();
-      break;
-    default:
-      AbortAllAnimations();
-      target_ink_drop_state_ = ink_drop_state;
-  }
+  AbortAllAnimations();
+  if (ink_drop_state == InkDropState::ACTIVATED)
+    GetRootLayer()->SetVisible(true);
+  else if (ink_drop_state == InkDropState::HIDDEN)
+    SetStateToHidden();
+  target_ink_drop_state_ = ink_drop_state;
+  animation_observer_ = CreateAnimationObserver(ink_drop_state);
+  animation_observer_->SetActive();
+  // |this| may be deleted! |animation_observer_| might synchronously call
+  // AnimationEndedCallback which can delete |this|.
 }
 
 void InkDropRipple::SnapToActivated() {
-  AbortAllAnimations();
-  // |animation_observer| will be deleted when AnimationEndedCallback() returns
-  // true.
-  // TODO(bruthig): Implement a safer ownership model for the
-  // |animation_observer|.
-  ui::CallbackLayerAnimationObserver* animation_observer =
-      new ui::CallbackLayerAnimationObserver(
-          base::Bind(&InkDropRipple::AnimationStartedCallback,
-                     base::Unretained(this), InkDropState::ACTIVATED),
-          base::Bind(&InkDropRipple::AnimationEndedCallback,
-                     base::Unretained(this), InkDropState::ACTIVATED));
-  GetRootLayer()->SetVisible(true);
-  target_ink_drop_state_ = InkDropState::ACTIVATED;
-  animation_observer->SetActive();
+  SnapToState(InkDropState::ACTIVATED);
 }
 
 bool InkDropRipple::IsVisible() {
@@ -104,9 +81,7 @@ bool InkDropRipple::IsVisible() {
 }
 
 void InkDropRipple::SnapToHidden() {
-  AbortAllAnimations();
-  SetStateToHidden();
-  target_ink_drop_state_ = InkDropState::HIDDEN;
+  SnapToState(InkDropState::HIDDEN);
 }
 
 test::InkDropRippleTestApi* InkDropRipple::GetTestApi() {
@@ -131,7 +106,16 @@ bool InkDropRipple::AnimationEndedCallback(
                                   ? InkDropAnimationEndedReason::PRE_EMPTED
                                   : InkDropAnimationEndedReason::SUCCESS);
   // |this| may be deleted!
-  return true;
+  return false;
+}
+
+std::unique_ptr<ui::CallbackLayerAnimationObserver>
+InkDropRipple::CreateAnimationObserver(InkDropState ink_drop_state) {
+  return std::make_unique<ui::CallbackLayerAnimationObserver>(
+      base::BindRepeating(&InkDropRipple::AnimationStartedCallback,
+                          base::Unretained(this), ink_drop_state),
+      base::BindRepeating(&InkDropRipple::AnimationEndedCallback,
+                          base::Unretained(this), ink_drop_state));
 }
 
 }  // namespace views

ui/views/animation/ink_drop_ripple.h

@@ -5,6 +5,8 @@
 #ifndef UI_VIEWS_ANIMATION_INK_DROP_RIPPLE_H_
 #define UI_VIEWS_ANIMATION_INK_DROP_RIPPLE_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/size.h"
@@ -117,11 +119,18 @@ class VIEWS_EXPORT InkDropRipple {
       InkDropState ink_drop_state,
       const ui::CallbackLayerAnimationObserver& observer);
 
+  // Creates a new animation observer bound to AnimationStartedCallback() and
+  // AnimationEndedCallback().
+  std::unique_ptr<ui::CallbackLayerAnimationObserver> CreateAnimationObserver(
+      InkDropState ink_drop_state);
+
   // The target InkDropState.
   InkDropState target_ink_drop_state_;
 
   InkDropRippleObserver* observer_;
 
+  std::unique_ptr<ui::CallbackLayerAnimationObserver> animation_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(InkDropRipple);
 };
 

ui/views/animation/ink_drop_ripple_unittest.cc

@@ -264,8 +264,8 @@ TEST_P(InkDropRippleTest, SnapToHiddenWithoutActiveAnimations) {
   EXPECT_FALSE(test_api_->HasActiveAnimations());
   EXPECT_EQ(views::InkDropState::HIDDEN,
             ink_drop_ripple_->target_ink_drop_state());
-  EXPECT_EQ(1, observer_.last_animation_started_ordinal());
-  EXPECT_EQ(2, observer_.last_animation_ended_ordinal());
+  EXPECT_EQ(3, observer_.last_animation_started_ordinal());
+  EXPECT_EQ(4, observer_.last_animation_ended_ordinal());
 
   EXPECT_EQ(InkDropRipple::kHiddenOpacity, test_api_->GetCurrentOpacity());
   EXPECT_FALSE(ink_drop_ripple_->IsVisible());