Restore focus-state ink drop highlight on buttons after ripple animation

ui/views/animation/ink_drop_impl.cc

Index: ui/views/animation/ink_drop_impl.cc
diff --git a/ui/views/animation/ink_drop_impl.cc b/ui/views/animation/ink_drop_impl.cc
index cde9818ce5df9ae2bf4e111720d3579e832f413d..a5d7736f939f6417f9fcdb3f56dc4621d887ad17 100644
--- a/ui/views/animation/ink_drop_impl.cc
+++ b/ui/views/animation/ink_drop_impl.cc
@@ -17,23 +17,23 @@ namespace {
 
 // The duration, in milliseconds, of the highlight state fade in animation when
 // it is triggered by user input.
-const int kHighlightFadeInFromUserInputDurationInMs = 250;
+const int kHighlightFadeInFromUserInputDurationMs = 250;
 
 // The duration, in milliseconds, of the highlight state fade out animation when
 // it is triggered by user input.
-const int kHighlightFadeOutFromUserInputDurationInMs = 250;
+const int kHighlightFadeOutFromUserInputDurationMs = 250;
 
 // The duration, in milliseconds, of the highlight state fade in animation when
 // it is triggered by an ink drop ripple animation ending.
-const int kHighlightFadeInAfterRippleDurationInMs = 250;
+const int kHighlightFadeInAfterRippleDurationMs = 250;
 
 // The duration, in milliseconds, of the highlight state fade out animation when
 // it is triggered by an ink drop ripple animation starting.
-const int kHighlightFadeOutBeforeRippleDurationInMs = 120;
+const int kHighlightFadeOutBeforeRippleDurationMs = 120;
 
 // The amount of time in milliseconds that |highlight_| should delay after a
-// ripple animation before fading in.
-const int kHighlightFadeInAfterRippleDelayInMs = 1000;
+// ripple animation before fading in, for highlight due to mouse hover.
+const int kHoverFadeInAfterRippleDelayMs = 1000;
 
 // Returns true if an ink drop with the given |ink_drop_state| should
 // automatically transition to the InkDropState::HIDDEN state.
@@ -78,9 +78,18 @@ void InkDropImpl::AnimateToState(InkDropState ink_drop_state) {
   if (!ink_drop_ripple_)
     CreateInkDropRipple();
 
+  // When deactivating and the host is focused, snap back to the highlight
+  // state. (In the case of highlighting due to hover, we'll animate the
+  // highlight back in after a delay.)
+  if (ink_drop_state == views::InkDropState::DEACTIVATED && is_focused_) {

[bruthig, 2016/07/09 16:25:47]

Should the focus highlight snap back in after an ACTION_TRIGGERED and an
ALTERNATE_ACTION_TRIGGERED as well?  Or do we want to let those animations
actually happen as the ACTION_TRIGGERED shape is not the same as the highlight?

If we want to snap back to focused for all states that auto-transition to HIDDEN
then I think it might be better to handle it in the
AnimationStarted(InkDropState) method like so:

void AnimationStarted(InkDropState state) {
  if (ShouldAnimateToHidden(state) && is_focused_) {
    ink_drop_ripple_->HideImmediately();
    SetHighlight(true, base::TimeDelta(), false);
    return;
  }
}

=====

Alternatively would it be enough to snap the highlight back in after the HIDDEN
animation completes instead of starting the timer as you tried originally?

[Evan Stade, 2016/07/12 17:32:42]

good point, I guess you can see this if you focus the back button and press
enter. The focus highlight should come back. (assuming back is still active)

I tried out several options for handling ACTION_TRIGGERED including
a) snapping directly to highlight - this looks bad because there's no activation
explosion
b) wait till after the ripple animation, then animate to highlight (like for
hover but with no delay)
c) bring the hover back in while animating to HIDDEN. This didn't work because
SetHighlight does nothing if the ripple is still visible. I'd be worried about
touching that logic.

So I picked (b), which is represented by the additional code in
AnimationEnded().
This exact code doesn't work (causes a crash). Anyway it seems better to not
animate in the first place than to animate and immediately hide.
Doing that works, but it feels too sluggish to me which is why I abandoned that
idea.

+    ink_drop_ripple_->HideImmediately();
+    SetHighlight(true, base::TimeDelta(), false);
+    return;
+  }
+
   if (ink_drop_state != views::InkDropState::HIDDEN) {
     SetHighlight(false, base::TimeDelta::FromMilliseconds(
-                            kHighlightFadeOutBeforeRippleDurationInMs),
+                            kHighlightFadeOutBeforeRippleDurationMs),
                  true);
   }
 
@@ -102,9 +111,9 @@ void InkDropImpl::SetHovered(bool is_hovered) {
   SetHighlight(ShouldHighlight(),
                ShouldHighlight()
                    ? base::TimeDelta::FromMilliseconds(
-                         kHighlightFadeInFromUserInputDurationInMs)
+                         kHighlightFadeInFromUserInputDurationMs)
                    : base::TimeDelta::FromMilliseconds(
-                         kHighlightFadeOutFromUserInputDurationInMs),
+                         kHighlightFadeOutFromUserInputDurationMs),
                false);
 }
 
@@ -214,7 +223,7 @@ void InkDropImpl::AnimationEnded(InkDropHighlight::AnimationType animation_type,
 void InkDropImpl::SetHighlight(bool should_highlight,
                                base::TimeDelta animation_duration,
                                bool explode) {
-  StopHighlightAfterRippleTimer();
+  highlight_after_ripple_timer_.reset();
 
   if (IsHighlightFadingInOrVisible() == should_highlight)
     return;
@@ -233,26 +242,17 @@ bool InkDropImpl::ShouldHighlight() const {
 }
 
 void InkDropImpl::StartHighlightAfterRippleTimer() {
-  StopHighlightAfterRippleTimer();
-
-  if (!highlight_after_ripple_timer_)
-    highlight_after_ripple_timer_.reset(new base::OneShotTimer);
-
+  highlight_after_ripple_timer_.reset(new base::OneShotTimer);
   highlight_after_ripple_timer_->Start(
       FROM_HERE,
-      base::TimeDelta::FromMilliseconds(kHighlightFadeInAfterRippleDelayInMs),
+      base::TimeDelta::FromMilliseconds(kHoverFadeInAfterRippleDelayMs),
       base::Bind(&InkDropImpl::HighlightAfterRippleTimerFired,
                  base::Unretained(this)));
 }
 
-void InkDropImpl::StopHighlightAfterRippleTimer() {
-  if (highlight_after_ripple_timer_)
-    highlight_after_ripple_timer_.reset();
-}
-
 void InkDropImpl::HighlightAfterRippleTimerFired() {
   SetHighlight(true, base::TimeDelta::FromMilliseconds(
-                         kHighlightFadeInAfterRippleDurationInMs),
+                         kHighlightFadeInAfterRippleDurationMs),
                true);
   highlight_after_ripple_timer_.reset();
 }