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

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

At first I just copied the way we restore highlight due to hover (i.e.
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(),
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307, 623957
Committed: https://crrev.com/efad9e3a50b9ce33582780a860519e40539c3cef
Cr-Commit-Position: refs/heads/master@{#407845}

[Evan Stade, 2016-07-07 21:20:12]

Description was changed from

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

This is similar to the way we restore hover-state ink drop highlight. It
feels a little bit sluggish, so I reduced the delay for the focus case.

BUG=626307,623957
==========

to

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

At first I just copied the way we restore highlight due to hover (i.e. I
triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), not
is_hovered_), but that looked sluggish and broken in comparison to just snapping
back to the hover state.

BUG=626307,623957
==========

[Evan Stade, 2016-07-07 21:20:12]

estade@chromium.org changed reviewers:
+ bruthig@chromium.org

[Evan Stade, 2016-07-07 21:20:28]

Description was changed from

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

At first I just copied the way we restore highlight due to hover (i.e. I
triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), not
is_hovered_), but that looked sluggish and broken in comparison to just snapping
back to the hover state.

BUG=626307,623957
==========

to

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

At first I just copied the way we restore highlight due to hover (i.e. 
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), 
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307,623957
==========

[Evan Stade, 2016-07-07 21:20:42]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 21:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 23:24:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 23:24:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[Evan Stade, 2016-07-08 23:49:22]

ping

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

pong ;)

https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:84: if (ink_drop_state ==
views::InkDropState::DEACTIVATED && is_focused_) {
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]

https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:84: if (ink_drop_state ==
views::InkDropState::DEACTIVATED && is_focused_) {
On 2016/07/09 16:25:47, bruthig wrote:
> 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?

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().

> 
> 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;
>   }
> }

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.

> 
> =====
> 
> 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?

Doing that works, but it feels too sluggish to me which is why I abandoned that
idea.

[bruthig, 2016-07-12 18:53:07]

On 2016/07/12 17:32:42, Evan Stade wrote:
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> File ui/views/animation/ink_drop_impl.cc (right):
> 
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> ui/views/animation/ink_drop_impl.cc:84: if (ink_drop_state ==
> views::InkDropState::DEACTIVATED && is_focused_) {
> On 2016/07/09 16:25:47, bruthig wrote:
> > 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?
> 
> 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().
> 
> > 
> > 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;
> >   }
> > }
> 
> 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.
> 

ACK

> > 
> > =====
> > 
> > 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?
> 
> Doing that works, but it feels too sluggish to me which is why I abandoned
that
> idea.

I'm not sure we're on the same page here.  I thought you originally tried
kicking off the timer whereas I was suggesting what you did in (b) above.


Why use strategy (b) from above for the ACTION_TRIGGERED and
ALTERNATE_ACTION_TRIGGERED use cases but not for the DEACTIVATED case?  I would
be in favour of using it for DEACTIVATED as well. i.e. why not remove the code
you added in AnimateToState()?

It's tough to say without seeing it in action but I suspect not having the
DEACTIVATED state explode, similar to your point (a), would look bad.

[bruthig, 2016-07-12 18:53:35]

https://codereview.chromium.org/2125223004/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2125223004/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:204: else if (is_focused_)
|is_focused_| and |is_hovered_| could both be true and I assume |is_focused_|
should take precedence. i.e. switch the ordering of the if/else if.

[Evan Stade, 2016-07-12 19:13:39]

On 2016/07/12 18:53:07, bruthig wrote:
> On 2016/07/12 17:32:42, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> > File ui/views/animation/ink_drop_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> > ui/views/animation/ink_drop_impl.cc:84: if (ink_drop_state ==
> > views::InkDropState::DEACTIVATED && is_focused_) {
> > On 2016/07/09 16:25:47, bruthig wrote:
> > > 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?
> > 
> > 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().
> > 
> > > 
> > > 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;
> > >   }
> > > }
> > 
> > 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.
> > 
> 
> ACK
> 
> > > 
> > > =====
> > > 
> > > 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?
> > 
> > Doing that works, but it feels too sluggish to me which is why I abandoned
> that
> > idea.
> 
> I'm not sure we're on the same page here.  I thought you originally tried
> kicking off the timer whereas I was suggesting what you did in (b) above.

Both ways look too sluggish (waiting for the explode then highlighting or
waiting for the explode, then delaying, then highlighting)

> 
> 
> Why use strategy (b) from above for the ACTION_TRIGGERED and
> ALTERNATE_ACTION_TRIGGERED use cases but not for the DEACTIVATED case?  I
would
> be in favour of using it for DEACTIVATED as well. i.e. why not remove the code
> you added in AnimateToState()?
> 
> It's tough to say without seeing it in action but I suspect not having the
> DEACTIVATED state explode, similar to your point (a), would look bad.

I recommend you do try it in person because testing out these different
strategies live was the only way I was able to decide which one felt best to me.

[bruthig, 2016-07-13 20:04:23]

On 2016/07/12 19:13:39, Evan Stade wrote:
> On 2016/07/12 18:53:07, bruthig wrote:
> > On 2016/07/12 17:32:42, Evan Stade wrote:
> > >
> >
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> > > File ui/views/animation/ink_drop_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2125223004/diff/20001/ui/views/animation/ink_...
> > > ui/views/animation/ink_drop_impl.cc:84: if (ink_drop_state ==
> > > views::InkDropState::DEACTIVATED && is_focused_) {
> > > On 2016/07/09 16:25:47, bruthig wrote:
> > > > 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?
> > > 
> > > 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().
> > > 
> > > > 
> > > > 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;
> > > >   }
> > > > }
> > > 
> > > 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.
> > > 
> > 
> > ACK
> > 
> > > > 
> > > > =====
> > > > 
> > > > 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?
> > > 
> > > Doing that works, but it feels too sluggish to me which is why I abandoned
> > that
> > > idea.
> > 
> > I'm not sure we're on the same page here.  I thought you originally tried
> > kicking off the timer whereas I was suggesting what you did in (b) above.
> 
> Both ways look too sluggish (waiting for the explode then highlighting or
> waiting for the explode, then delaying, then highlighting)
> 
> > 
> > 
> > Why use strategy (b) from above for the ACTION_TRIGGERED and
> > ALTERNATE_ACTION_TRIGGERED use cases but not for the DEACTIVATED case?  I
> would
> > be in favour of using it for DEACTIVATED as well. i.e. why not remove the
code
> > you added in AnimateToState()?
> > 
> > It's tough to say without seeing it in action but I suspect not having the
> > DEACTIVATED state explode, similar to your point (a), would look bad.
> 
> I recommend you do try it in person because testing out these different
> strategies live was the only way I was able to decide which one felt best to
me.

Ok I'm sold!  Can you add some tests?

[Evan Stade, 2016-07-13 20:27:24]

tests added

https://codereview.chromium.org/2125223004/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2125223004/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:204: else if (is_focused_)
On 2016/07/12 18:53:35, bruthig wrote:
> |is_focused_| and |is_hovered_| could both be true and I assume |is_focused_|
> should take precedence. i.e. switch the ordering of the if/else if.

Done.

[bruthig, 2016-07-13 21:11:38]

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl_unittest.cc (right):

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:267:
EXPECT_TRUE(test_api_.HasActiveAnimations());
nit: test_api_.HasActiveAnimations() could return true for the highlight
animations here and it could be a false psoitive.
Perhaps you could confirm the state is still DEACTIVATED here and possibly
updates to HIDDEN after a CompleteAnimations().  Alternatively we could maybe
add an InkDropImplTestApi::HasRippleAnimations().

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:284:
EXPECT_FALSE(test_api_.HasActiveAnimations());
nit: Might want to confirm the highlight is visible here too and that the state
is updated to HIDDEN.

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:292: ink_drop_.SetHovered(true);
I believe this should be SetFocused() and not hovered, and I suspect you will
uncover a bug when you change it ;)

[Evan Stade, 2016-07-25 22:00:42]

sorry it took me a while to get back to this.

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl_unittest.cc (right):

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:267:
EXPECT_TRUE(test_api_.HasActiveAnimations());
On 2016/07/13 21:11:38, bruthig wrote:
> nit: test_api_.HasActiveAnimations() could return true for the highlight
> animations here and it could be a false psoitive.
> Perhaps you could confirm the state is still DEACTIVATED here and possibly
> updates to HIDDEN after a CompleteAnimations().  Alternatively we could maybe
> add an InkDropImplTestApi::HasRippleAnimations().

Done, sorta

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:284:
EXPECT_FALSE(test_api_.HasActiveAnimations());
On 2016/07/13 21:11:38, bruthig wrote:
> nit: Might want to confirm the highlight is visible here too and that the
state
> is updated to HIDDEN.

Done.

https://codereview.chromium.org/2125223004/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:292: ink_drop_.SetHovered(true);
On 2016/07/13 21:11:38, bruthig wrote:
> I believe this should be SetFocused() and not hovered, and I suspect you will
> uncover a bug when you change it ;)

-_-

[bruthig, 2016-07-25 22:13:20]

lgtm

https://codereview.chromium.org/2125223004/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl_unittest.cc (right):

https://codereview.chromium.org/2125223004/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:285:
EXPECT_EQ(InkDropState::HIDDEN, test_api_.ripple()->target_ink_drop_state());
nit: Why not use InkDrop::GetTargetInkDropState()?

[Evan Stade, 2016-07-25 22:20:05]

https://codereview.chromium.org/2125223004/diff/80001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl_unittest.cc (right):

https://codereview.chromium.org/2125223004/diff/80001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl_unittest.cc:285:
EXPECT_EQ(InkDropState::HIDDEN, test_api_.ripple()->target_ink_drop_state());
On 2016/07/25 22:13:20, bruthig wrote:
> nit: Why not use InkDrop::GetTargetInkDropState()?

no reason, done

[Evan Stade, 2016-07-25 22:24:22]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 22:24:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-07-25 22:25:13]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-07-25 22:25:13]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2125223004/#ps120001
(title: "IWYU")

[commit-bot: I haz the power, 2016-07-25 22:25:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 00:14:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 00:14:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Evan Stade, 2016-07-26 17:05:30]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-07-26 17:05:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 17:47:06]

Description was changed from

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

At first I just copied the way we restore highlight due to hover (i.e. 
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), 
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307,623957
==========

to

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

At first I just copied the way we restore highlight due to hover (i.e. 
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), 
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307,623957
==========

[commit-bot: I haz the power, 2016-07-26 17:47:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-26 17:48:17]

Description was changed from

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

At first I just copied the way we restore highlight due to hover (i.e. 
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(), 
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307,623957
==========

to

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

At first I just copied the way we restore highlight due to hover (i.e.
I triggered StartHighlightAfterRippleTimer() based on ShouldHighlight(),
not is_hovered_), but that looked sluggish and broken in comparison to just
snapping back to the hover state.

BUG=626307,623957

Committed: https://crrev.com/efad9e3a50b9ce33582780a860519e40539c3cef
Cr-Commit-Position: refs/heads/master@{#407845}
==========

[commit-bot: I haz the power, 2016-07-26 17:48:18]

Patchset 7 (id:??) landed as
https://crrev.com/efad9e3a50b9ce33582780a860519e40539c3cef
Cr-Commit-Position: refs/heads/master@{#407845}