Allow InkDropRipple to co-exist with highlight

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"
@@ skipping 60 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_) {
-    ink_drop_ripple_->HideImmediately();
-    SetHighlight(true, base::TimeDelta(), false);
-    return;
-  }
+  if (ink_drop_ripple_->OverridesHighlight()) {
+    // 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_) {
+      ink_drop_ripple_->HideImmediately();
+      SetHighlight(true, base::TimeDelta(), false);
+      return;
+    }
 
-  if (ink_drop_state != views::InkDropState::HIDDEN) {
-    SetHighlight(false, base::TimeDelta::FromMilliseconds(
-                            kHighlightFadeOutBeforeRippleDurationMs),
-                 true);
+    if (ink_drop_state != views::InkDropState::HIDDEN) {
+      SetHighlight(false, base::TimeDelta::FromMilliseconds(
+                              kHighlightFadeOutBeforeRippleDurationMs),
+                   true);
+    }
   }
 
   ink_drop_ripple_->AnimateToState(ink_drop_state);
 }
 
 void InkDropImpl::SnapToActivated() {
   DestroyHiddenTargetedAnimations();
   if (!ink_drop_ripple_)
     CreateInkDropRipple();
 
-  SetHighlight(false, base::TimeDelta(), false);
+  if (ink_drop_ripple_->OverridesHighlight())
+    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(
                          kHighlightFadeInFromUserInputDurationMs)
@@ skipping 11 matching lines @@
   if (ink_drop_ripple_ &&
       (ink_drop_ripple_->target_ink_drop_state() == InkDropState::HIDDEN ||
        ShouldAnimateToHidden(ink_drop_ripple_->target_ink_drop_state()))) {
     DestroyInkDropRipple();
   }
 }
 
 void InkDropImpl::CreateInkDropRipple() {
   DestroyInkDropRipple();
   ink_drop_ripple_ = ink_drop_host_->CreateInkDropRipple();
-  ink_drop_ripple_->set_observer(this);
-  root_layer_->Add(ink_drop_ripple_->GetRootLayer());
+  if (ink_drop_ripple_->GetRootLayer()) {
+    ink_drop_ripple_->set_observer(this);
+    root_layer_->Add(ink_drop_ripple_->GetRootLayer());
+  }
   AddRootLayerToHostIfNeeded();
 }
 
 void InkDropImpl::DestroyInkDropRipple() {
   if (!ink_drop_ripple_)
     return;
-  root_layer_->Remove(ink_drop_ripple_->GetRootLayer());
+  if (ink_drop_ripple_->GetRootLayer())
+    root_layer_->Remove(ink_drop_ripple_->GetRootLayer());
   ink_drop_ripple_.reset();
   RemoveRootLayerFromHostIfNeeded();
 }
 
 void InkDropImpl::CreateInkDropHighlight() {
   DestroyInkDropHighlight();
 
   highlight_ = ink_drop_host_->CreateInkDropHighlight();
   if (!highlight_)
     return;
   highlight_->set_observer(this);
   root_layer_->Add(highlight_->layer());
   AddRootLayerToHostIfNeeded();
 }
 
 void InkDropImpl::DestroyInkDropHighlight() {
   if (!highlight_)
     return;
   root_layer_->Remove(highlight_->layer());
   highlight_->set_observer(nullptr);
   highlight_.reset();
   RemoveRootLayerFromHostIfNeeded();
 }
 
 void InkDropImpl::AddRootLayerToHostIfNeeded() {
   DCHECK(highlight_ || ink_drop_ripple_);
   if (!root_layer_added_to_host_) {
     root_layer_added_to_host_ = true;
     ink_drop_host_->AddInkDropLayer(root_layer_.get());

[bruthig, 2016/08/25 17:45:29]

Perhaps this should only add the |root_layer_| to the |ink_drop_host_| if
|root_layer_| actually has child layers?

[Evan Stade, 2016/08/26 23:36:15]

But we'll need to do that for highlight anyway.

[bruthig, 2016/08/31 22:22:29]

I don't think this method should make assumptions on when it is called and
whether the specific highlight type will require this.  This type of assumption
makes extension much more difficult.  Why not just implement it for the general
case now?

[Evan Stade, 2016/09/01 01:19:15]

If both highlight and ripple are layerless, then there's no point in having an
InkDropImpl, is there? I don't want to support a use case that doesn't exist.
That's why.

I've made some changes though, do you like them?

[bruthig, 2016/09/01 15:53:57]

Yeah

   }
 }
 
 void InkDropImpl::RemoveRootLayerFromHostIfNeeded() {
   if (root_layer_added_to_host_ && !highlight_ && !ink_drop_ripple_) {

[bruthig, 2016/08/25 17:45:29]

This should check if the ink drop layer exists instead of whether
|ink_drop_ripple_| is non-null.

[Evan Stade, 2016/08/26 23:36:15]

That would break the case where we destroy the highlight but the ripple is still
around.

[bruthig, 2016/08/31 22:22:29]

Sorry I wasn't clear on that.  The following re-write would allow |highlight_|
and |ink_drop_ripple_| instances to exist that don't actually have Layers.

if (root_layer_added_to_host_ && (!highlight_ || !highlight->GetRootLayer()| &&
(!ink_drop_ripple_ || !ink_drop_ripple_->GetRootLayer())) { ... }

[Evan Stade, 2016/09/01 01:19:15]

I don't want to support an empty ripple and highlight until there's a good
reason to. But if the ripple has no layer then root_layer_added_to_host will be
false.

[bruthig, 2016/09/01 15:53:57]

Consider the following case:

1. SetHover(true)
- Root layer gets added

2. CreateInkDropRipple()

3. SetHover(false) -> DestroyInkDropHighlight() ->
RemoveRootLayerFromHostIfNeeded() 
- Root layer is NOT removed because |ink_drop_ripple_| is not null
- And as it currently stands DestroyInkDropRipple() will not be called because
EmptyInkDropRipple doesn't notify the AnimationEnded observer.  Thus the
InkDropHostView will have a 'zombie' ink drop layer.

This might be worthy of a test...

[Evan Stade, 2016/09/07 23:46:19]

Done.

     root_layer_added_to_host_ = false;
     ink_drop_host_->RemoveInkDropLayer(root_layer_.get());
   }
 }
 
 bool InkDropImpl::IsHighlightFadingInOrVisible() const {
   return highlight_ && highlight_->IsFadingInOrVisible();
 }
 
 // -----------------------------------------------------------------------------
 // views::InkDropRippleObserver:
 
 void InkDropImpl::AnimationStarted(InkDropState ink_drop_state) {}
 
 void InkDropImpl::AnimationEnded(InkDropState ink_drop_state,
                                  InkDropAnimationEndedReason reason) {
   if (reason != InkDropAnimationEndedReason::SUCCESS)
     return;
   if (ShouldAnimateToHidden(ink_drop_state)) {
     ink_drop_ripple_->AnimateToState(views::InkDropState::HIDDEN);
   } else if (ink_drop_state == views::InkDropState::HIDDEN) {
     // Re-highlight, as necessary. For hover, there's a delay; for focus, jump
     // straight into the animation.
-    if (is_focused_)
-      HighlightAfterRippleTimerFired();
-    else if (is_hovered_)
-      StartHighlightAfterRippleTimer();
+    if (ink_drop_ripple_->OverridesHighlight()) {

[bruthig, 2016/08/25 17:45:29]

I'm wondering if this should check |ink_drop_highlight_|->IsFadingInOrVisible()
instead.  That would be more future proof.

[Evan Stade, 2016/08/26 23:36:15]

Done.

+      if (is_focused_)
+        HighlightAfterRippleTimerFired();
+      else if (is_hovered_)
+        StartHighlightAfterRippleTimer();
+    }
 
     // TODO(bruthig): Investigate whether creating and destroying
     // InkDropRipples is expensive and consider creating an
     // InkDropRipplePool. See www.crbug.com/522175.
     DestroyInkDropRipple();
   }
 }
 
 // -----------------------------------------------------------------------------
 // views::InkDropHighlightObserver:
@@ skipping 12 matching lines @@
 void InkDropImpl::SetHighlight(bool should_highlight,
                                base::TimeDelta animation_duration,
                                bool explode) {
   highlight_after_ripple_timer_.reset();
 
   if (IsHighlightFadingInOrVisible() == should_highlight)
     return;
 
   if (should_highlight) {
     CreateInkDropHighlight();
     if (highlight_ && !(ink_drop_ripple_ && ink_drop_ripple_->IsVisible()))

[bruthig, 2016/08/25 17:45:29]

This clause is currently preventing the highlight from fading in if the ripple
is visible.  This needs to be updated to adhere to the new OverridesHighlight()
semantics.

[Evan Stade, 2016/08/26 23:36:15]

Done.

       highlight_->FadeIn(animation_duration);
   } else {
     highlight_->FadeOut(animation_duration, explode);
   }
 }
 
 bool InkDropImpl::ShouldHighlight() const {
   return is_focused_ || is_hovered_;
 }
 
 void InkDropImpl::StartHighlightAfterRippleTimer() {
   highlight_after_ripple_timer_.reset(new base::OneShotTimer);
   highlight_after_ripple_timer_->Start(
       FROM_HERE,
       base::TimeDelta::FromMilliseconds(kHoverFadeInAfterRippleDelayMs),
       base::Bind(&InkDropImpl::HighlightAfterRippleTimerFired,
                  base::Unretained(this)));
 }
 
 void InkDropImpl::HighlightAfterRippleTimerFired() {
   SetHighlight(true, base::TimeDelta::FromMilliseconds(
                          kHighlightFadeInAfterRippleDurationMs),
                true);
   highlight_after_ripple_timer_.reset();
 }
 
 }  // namespace views