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

ui/views/animation/ink_drop_impl.cc

 // Copyright 2015 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/animation/ink_drop_impl.h"
 
 #include "base/auto_reset.h"
 #include "base/timer/timer.h"
 #include "ui/compositor/layer.h"
 #include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/animation/ink_drop_host.h"
 #include "ui/views/animation/square_ink_drop_ripple.h"
 
 namespace views {
 
 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.
 bool ShouldAnimateToHidden(InkDropState ink_drop_state) {
   switch (ink_drop_state) {
     case views::InkDropState::ACTION_TRIGGERED:
     case views::InkDropState::ALTERNATE_ACTION_TRIGGERED:
     case views::InkDropState::DEACTIVATED:
       return true;
     default:
@@ skipping 24 matching lines @@
   if (!ink_drop_ripple_)
     return InkDropState::HIDDEN;
   return ink_drop_ripple_->target_ink_drop_state();
 }
 
 void InkDropImpl::AnimateToState(InkDropState ink_drop_state) {
   DestroyHiddenTargetedAnimations();
   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);
   }
 
   ink_drop_ripple_->AnimateToState(ink_drop_state);
 }
 
 void InkDropImpl::SnapToActivated() {
   DestroyHiddenTargetedAnimations();
   if (!ink_drop_ripple_)
     CreateInkDropRipple();
 
   SetHighlight(false, base::TimeDelta(), false);
 
   ink_drop_ripple_->SnapToActivated();
 }
 
 void InkDropImpl::SetHovered(bool is_hovered) {
   is_hovered_ = is_hovered;
   SetHighlight(ShouldHighlight(),
                ShouldHighlight()
                    ? base::TimeDelta::FromMilliseconds(
-                         kHighlightFadeInFromUserInputDurationInMs)
+                         kHighlightFadeInFromUserInputDurationMs)
                    : base::TimeDelta::FromMilliseconds(
-                         kHighlightFadeOutFromUserInputDurationInMs),
+                         kHighlightFadeOutFromUserInputDurationMs),
                false);
 }
 
 void InkDropImpl::SetFocused(bool is_focused) {
   is_focused_ = is_focused;
   SetHighlight(ShouldHighlight(), base::TimeDelta(), false);
 }
 
 void InkDropImpl::DestroyHiddenTargetedAnimations() {
   if (ink_drop_ripple_ &&
@@ skipping 89 matching lines @@
                                  InkDropAnimationEndedReason reason) {
   if (animation_type == InkDropHighlight::FADE_OUT &&
       reason == InkDropAnimationEndedReason::SUCCESS) {
     DestroyInkDropHighlight();
   }
 }
 
 void InkDropImpl::SetHighlight(bool should_highlight,
                                base::TimeDelta animation_duration,
                                bool explode) {
-  StopHighlightAfterRippleTimer();
+  highlight_after_ripple_timer_.reset();
 
   if (IsHighlightFadingInOrVisible() == should_highlight)
     return;
 
   if (should_highlight) {
     CreateInkDropHighlight();
     if (highlight_ && !(ink_drop_ripple_ && ink_drop_ripple_->IsVisible()))
       highlight_->FadeIn(animation_duration);
   } else {
     highlight_->FadeOut(animation_duration, explode);
   }
 }
 
 bool InkDropImpl::ShouldHighlight() const {
   return is_focused_ || is_hovered_;
 }
 
 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();
 }
 
 }  // namespace views