MD - add ripples to DL shelf items

MD - add ripples to DL shelf items

BUG=591582
Committed: https://crrev.com/15bda587d910b5d9f59b7184ecfeda37022eb6f4
Cr-Commit-Position: refs/heads/master@{#385052}

[Evan Stade, 2016-03-31 03:31:59]

Description was changed from

==========
WIP - ripples on DL shelf items

BUGS:
- hover while animating is left in wrong size
- text subpixel AA sometimes messed up
- sizing, color of ripples

BUG=591582
==========

to

==========
MD - add ripples to DL shelf items

See bug for list of unresolved TODOs.

BUG=591582
==========

[Evan Stade, 2016-03-31 03:32:00]

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

[Evan Stade, 2016-03-31 03:41:27]

issues mostly resolved, PTAL (your tip to remove the bounds masking was very
helpful)

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:105: float
CalculateLargestDistanceToCorners(const gfx::Size& size,
This calculation is a bit overkill for any centerpoint-centered ripple as all
corners are equidistant from the centerpoint. It also didn't make sense for
click-centered ripples, because it would mean the max size of the ripple (hence
the speed with which the ripple expanded) would change based on the click
location. I think the speed should always be the same, hence the simplification
(the calculation is now inlined in the one place it's called). This would
theoretically affect bookmark buttons slightly, but it actually looks pretty
similar to me at full speed (perhaps due to tweening).

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:189:
AnimateToTransform(transform, GetAnimationDuration(HIDDEN_TRANSFORM),
I got rid of this because it looked weird on the shelf items and didn't match
polymer buttons[1], which fade while continuing to expand.

[1]
https://elements.polymer-project.org/elements/paper-button?view=demo:demo/ind...

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:21: const float kMinRadius =
1.f;
this was the source of the problematic sizing (it was starting at 1x and
animating to Nx instead of starting at 0x and animating to Nx). It worked fine
if I changed this to 0.1f, but realistically there's no difference between
displaying a circle of size 1px vs 0.1px for a tiny fraction of a second. I
wanted to bump it to a higher number to avoid running into this problem again.

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

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:116:
CreateInkDropAnimation();
this change probably warrants a rename of the fn. What do you think of the
change itself? It is needed because otherwise a second click before the first
ink drop is totally faded (and destroyed on L171) will reuse the old ink drop,
which has the original center point. We need to create a new ink drop for every
hidden->not hidden animation.

[Evan Stade, 2016-03-31 03:42:23]

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

[commit-bot: I haz the power, 2016-03-31 03:42:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832963002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832963002/60001

[commit-bot: I haz the power, 2016-03-31 04:09:44]

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

[commit-bot: I haz the power, 2016-03-31 04:09:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bruthig, 2016-03-31 19:39:03]

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:188:
body_hover_animation_.reset(new gfx::SlideAnimation(this));
Is this not needed for non-md?

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:406:
ink_drop_delegate_.OnAction(views::InkDropState::ACTIVATED);
How come you are leaving a ripple active during a drag operation?  For the
BookmarksButtons we decided to hide the hover/ripple during a drag.  We should
probably be consistent.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:418:
ink_drop_delegate_.OnAction(views::InkDropState::HIDDEN);
Does it make sense to move this to the HandleClickEvent() method?

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:419:
ink_drop_delegate_.SetHovered(IsMouseHovered());
Why do you need to set hovered here?  When the HIDDEN animation ends the
InkDropAnimationControllerImpl should fade the hover back in after a delay.  Is
it the delay that you are trying to skip?

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:437:
ink_drop_delegate_.SetHovered(true);
How come the ButtonInkDropAnimation::OnMouseEntered() handler doesn't work here?
 I'm not sure if this is helpful but FYI if an InkDropAnimation is visible then
the InkDropAnimationControllerImpl won't actually make the hover visible even
when SetHovered(true) is called.  Think of it as logical hover versus visibly
hovered.  The InkDropDelegate::SetHovered() controls the logical state and the
InkDropAnimationControllerImpl is responsible for deciding when to actually show
it so that it meshes nicely with the ripple.

Similar applies to OnMouseExited() below.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:471:
ink_drop_delegate_.OnAction(views::InkDropState::DEACTIVATED);
FYI On Chrome-Linux I am able to get the ripple to persist when dragging an item
and dropping it in the content area.  This may not be getting called.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:495: // TODO(estade):
when the item is animating open and the user hovers, the
Is the size still stuck if you mouse off and mouse on again?

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:510:
ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);
I would expect the QUICK_ACTION trigger to be tied closely to the OpenDownload()
action.  Would it make sense to move this into the OpenDownload() call?  The
added benefit there is that a keyboard action will also trigger the ripple,
which is what we want.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:105: float
CalculateLargestDistanceToCorners(const gfx::Size& size,
On 2016/03/31 03:41:27, Evan Stade wrote:
> This calculation is a bit overkill for any centerpoint-centered ripple as all
> corners are equidistant from the centerpoint. It also didn't make sense for

Yes this is definitely overkill for ripples that are always View centered, it
was added purely for the event centered ripples.

> click-centered ripples, because it would mean the max size of the ripple
(hence
> the speed with which the ripple expanded) would change based on the click
> location. I think the speed should always be the same, hence the
simplification
> (the calculation is now inlined in the one place it's called). This would
> theoretically affect bookmark buttons slightly, but it actually looks pretty
> similar to me at full speed (perhaps due to tweening).

You are correct and varkha@ and I debated about this same point.  Should it be
constant speed or constant time to fill the View surface.  We chose to go with
constant time for two main reasons.  

1. On a QUICK_ACTION we wanted the ripple to be fading out by the time it was
getting close to the boundaries.  If you fix the radius then you will see the
ripple fading out when you click near a corner, but if you click near the center
then the ripple hits the edges hard at full opacity before fading out. 

2. In regards to the BookmarkButtons we had to balance different sizes of
buttons.  We felt like using a constant speed caused the smallest of the
BookmarkButtons to fill too quickly with little sense of shape to the expanding
ripple.

We may want to consider using more dynamic ripple durations and have them be a
function of the View surface.  The download shelf buttons are much much larger
in comparison to the other ripple surfaces and I think the ripple might benefit
from using different timings.

Perhaps we should show both versions to the designers to see if they have any
strong opinions.

WDYT?

I will add varkha@ as he may also have input.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:189:
AnimateToTransform(transform, GetAnimationDuration(HIDDEN_TRANSFORM),
On 2016/03/31 03:41:27, Evan Stade wrote:
> I got rid of this because it looked weird on the shelf items and didn't match
> polymer buttons[1], which fade while continuing to expand.
> 
> [1]
>
https://elements.polymer-project.org/elements/paper-button?view=demo:demo/ind...

This was a very intentional choice and I feel like it is a rough edge in the
Polymer spec.  It's main purpose is to make the ripple visually distinct for
when a user aborts a pending action compared to when a QUICK_ACTION is
triggered.  Without it these two actions are indistinguishable.
(Oddly enough I'm having a tough time triggering it, I may have to investigate)

What looked weird about it? And was the button actually being triggered or
aborted?

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:21: const float kMinRadius =
1.f;
On 2016/03/31 03:41:27, Evan Stade wrote:
> this was the source of the problematic sizing (it was starting at 1x and
> animating to Nx instead of starting at 0x and animating to Nx). It worked fine
> if I changed this to 0.1f, but realistically there's no difference between
> displaying a circle of size 1px vs 0.1px for a tiny fraction of a second. I
> wanted to bump it to a higher number to avoid running into this problem again.

Acknowledged.

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

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:116:
CreateInkDropAnimation();
On 2016/03/31 03:41:27, Evan Stade wrote:
> this change probably warrants a rename of the fn. What do you think of the
> change itself? It is needed because otherwise a second click before the first
> ink drop is totally faded (and destroyed on L171) will reuse the old ink drop,
> which has the original center point. We need to create a new ink drop for
every
> hidden->not hidden animation.

I was really, really hoping not to tie the lifetime of an ink drop to a change
in the center point.  In this particular case we will be incurring an extra cost
of Layer destruction/creation when ideally the existing ripple could just be
re-used.

Also I was really, really hoping that not every ripple surface would have to
track the |last_ink_drop_location_| for a couple of reasons.

1. I think it will be difficult to maintain and track the ripple center
correctly when taking in to account ripples should be activated on keyboard
input and ripples are triggered up and down the Button inheritance hierarchy as
well as from the (Button)InkDropDelegate.

2. The plan is to suppress ripples for touch events on Windows.  It would be
great if this decision was captured in one spot, e.g. the
InkDropAnimationControllerImpl.  I haven't flushed out all the details but was
thinking the InkDropDelegate::OnAction() function could also accept a ui::Event
parameter and then it could also use this to set the ripples center point.

Long story short, I'm hesitant to moving forward with this pattern for centering
ripples without at least discussing alternatives.

[Evan Stade, 2016-03-31 21:07:39]

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:188:
body_hover_animation_.reset(new gfx::SlideAnimation(this));
On 2016/03/31 19:39:03, bruthig wrote:
> Is this not needed for non-md?

no, this file is md-only.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:406:
ink_drop_delegate_.OnAction(views::InkDropState::ACTIVATED);
On 2016/03/31 19:39:02, bruthig wrote:
> How come you are leaving a ripple active during a drag operation?  For the
> BookmarksButtons we decided to hide the hover/ripple during a drag.  We should
> probably be consistent.

ok, done

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:418:
ink_drop_delegate_.OnAction(views::InkDropState::HIDDEN);
On 2016/03/31 19:39:02, bruthig wrote:
> Does it make sense to move this to the HandleClickEvent() method?

Done.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:419:
ink_drop_delegate_.SetHovered(IsMouseHovered());
On 2016/03/31 19:39:02, bruthig wrote:
> Why do you need to set hovered here?  When the HIDDEN animation ends the
> InkDropAnimationControllerImpl should fade the hover back in after a delay. 
Is
> it the delay that you are trying to skip?

doesn't seem to really do anything. Removed.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:437:
ink_drop_delegate_.SetHovered(true);
On 2016/03/31 19:39:02, bruthig wrote:
> How come the ButtonInkDropAnimation::OnMouseEntered() handler doesn't work
here?
>  I'm not sure if this is helpful but FYI if an InkDropAnimation is visible
then
> the InkDropAnimationControllerImpl won't actually make the hover visible even
> when SetHovered(true) is called.  Think of it as logical hover versus visibly
> hovered.  The InkDropDelegate::SetHovered() controls the logical state and the
> InkDropAnimationControllerImpl is responsible for deciding when to actually
show
> it so that it meshes nicely with the ripple.
> 
> Similar applies to OnMouseExited() below.

I dunno why I added this. Removed. Added a check for item state in
CreateInkDropHover.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:471:
ink_drop_delegate_.OnAction(views::InkDropState::DEACTIVATED);
On 2016/03/31 19:39:03, bruthig wrote:
> FYI On Chrome-Linux I am able to get the ripple to persist when dragging an
item
> and dropping it in the content area.  This may not be getting called.

removed

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:495: // TODO(estade):
when the item is animating open and the user hovers, the
On 2016/03/31 19:39:02, bruthig wrote:
> Is the size still stuck if you mouse off and mouse on again?

nope

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:510:
ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);
On 2016/03/31 19:39:02, bruthig wrote:
> I would expect the QUICK_ACTION trigger to be tied closely to the
OpenDownload()
> action.  Would it make sense to move this into the OpenDownload() call?  The
> added benefit there is that a keyboard action will also trigger the ripple,
> which is what we want.

I only did this because of this comment above the enum value:

  // The quick action for the view has been triggered. e.g. a tap gesture to
  // click a button.

I can add it to OnKeyEvent as well, although I'm not sure how to focus the
download items in the first place.

If I move this to OpenDownload, then clicking with a mouse creates a double
ripple effect.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:105: float
CalculateLargestDistanceToCorners(const gfx::Size& size,
On 2016/03/31 19:39:03, bruthig wrote:
> On 2016/03/31 03:41:27, Evan Stade wrote:
> > This calculation is a bit overkill for any centerpoint-centered ripple as
all
> > corners are equidistant from the centerpoint. It also didn't make sense for
> 
> Yes this is definitely overkill for ripples that are always View centered, it
> was added purely for the event centered ripples.
> 
> > click-centered ripples, because it would mean the max size of the ripple
> (hence
> > the speed with which the ripple expanded) would change based on the click
> > location. I think the speed should always be the same, hence the
> simplification
> > (the calculation is now inlined in the one place it's called). This would
> > theoretically affect bookmark buttons slightly, but it actually looks pretty
> > similar to me at full speed (perhaps due to tweening).
> 
> You are correct and varkha@ and I debated about this same point.  Should it be
> constant speed or constant time to fill the View surface.  We chose to go with
> constant time for two main reasons.  
> 
> 1. On a QUICK_ACTION we wanted the ripple to be fading out by the time it was
> getting close to the boundaries.  If you fix the radius then you will see the
> ripple fading out when you click near a corner, but if you click near the
center
> then the ripple hits the edges hard at full opacity before fading out. 
> 
> 2. In regards to the BookmarkButtons we had to balance different sizes of
> buttons.  We felt like using a constant speed caused the smallest of the
> BookmarkButtons to fill too quickly with little sense of shape to the
expanding
> ripple.

With this change, the speed is not constant WRT the size of the view, it is
constant WRT the location of the click. It is still slower for smaller views and
faster for larger views.

I feel pretty confident it looks bad for the ripple speed to change as you mouse
around the view and click repeatedly in different places.

> 
> We may want to consider using more dynamic ripple durations and have them be a
> function of the View surface.  The download shelf buttons are much much larger
> in comparison to the other ripple surfaces and I think the ripple might
benefit
> from using different timings.
> 
> Perhaps we should show both versions to the designers to see if they have any
> strong opinions.
> 
> WDYT?
> 
> I will add varkha@ as he may also have input.
>

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:189:
AnimateToTransform(transform, GetAnimationDuration(HIDDEN_TRANSFORM),
On 2016/03/31 19:39:03, bruthig wrote:
> On 2016/03/31 03:41:27, Evan Stade wrote:
> > I got rid of this because it looked weird on the shelf items and didn't
match
> > polymer buttons[1], which fade while continuing to expand.
> > 
> > [1]
> >
>
https://elements.polymer-project.org/elements/paper-button?view=demo:demo/ind...
> 
> This was a very intentional choice and I feel like it is a rough edge in the
> Polymer spec.  It's main purpose is to make the ripple visually distinct for
> when a user aborts a pending action compared to when a QUICK_ACTION is
> triggered.  Without it these two actions are indistinguishable.
> (Oddly enough I'm having a tough time triggering it, I may have to
investigate)

IIUC, you're saying that there's no visual distinction between click and click,
drag, release, which is a common way to cancel the action when the view is a
button. Yet traditional button press effects don't distinguish these two cases
either. They just switch between pressed and not-pressed.

> 
> What looked weird about it?

To my knowledge, the analogy to real life is a drop of water falling into a
larger, placid body of water. The ripple expands outwards then fades away. It
never contracts or rewinds. The polymer buttons match this and look natural to
me (your opinion may differ). I also think it would be nicer if we supported
multiple simultaneous ripples.

> And was the button actually being triggered or
> aborted?

triggered.

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

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:116:
CreateInkDropAnimation();
On 2016/03/31 19:39:03, bruthig wrote:
> On 2016/03/31 03:41:27, Evan Stade wrote:
> > this change probably warrants a rename of the fn. What do you think of the
> > change itself? It is needed because otherwise a second click before the
first
> > ink drop is totally faded (and destroyed on L171) will reuse the old ink
drop,
> > which has the original center point. We need to create a new ink drop for
> every
> > hidden->not hidden animation.
> 
> I was really, really hoping not to tie the lifetime of an ink drop to a change
> in the center point.  In this particular case we will be incurring an extra
cost
> of Layer destruction/creation when ideally the existing ripple could just be
> re-used.

I don't think that's a perf optimization worth worrying about. Perf is important
when many things are happening at once or happening automatically. Since this
happens at most once per user gesture, I doubt this could ever be a problem.
Plus, we currently create a new inkdrop every time the old inkdrop is finished
(this change only affects what we do when the existing effect hasn't completed
yet).

In any case, the way I would go about this if I were worried about it is to make
InkDropHost::CreateInkDropAnimation pass in the old animation, and let it either
update that ink drop or return a new one, as needed.

Also note that the center point can change regardless of whether the ripple is
centered on the click event. A bookmark button can grow or shrink when you
change its title. We have to refresh or replace the ripple even in the
centerpoin-centered case.

> 
> Also I was really, really hoping that not every ripple surface would have to
> track the |last_ink_drop_location_| for a couple of reasons.

Clearly not every ripple surface does need to. So far, only one out of many.

> 
> 1. I think it will be difficult to maintain and track the ripple center
> correctly when taking in to account ripples should be activated on keyboard
> input and ripples are triggered up and down the Button inheritance hierarchy
as
> well as from the (Button)InkDropDelegate.

I don't see the difficulty. Keyboard input can reset the location to the center.

> 
> 2. The plan is to suppress ripples for touch events on Windows.  It would be
> great if this decision was captured in one spot, e.g. the
> InkDropAnimationControllerImpl.  I haven't flushed out all the details but was
> thinking the InkDropDelegate::OnAction() function could also accept a
ui::Event
> parameter and then it could also use this to set the ripples center point.
> 
> Long story short, I'm hesitant to moving forward with this pattern for
centering
> ripples without at least discussing alternatives.

[bruthig, 2016-03-31 22:46:19]

I have to head out for the day but thought I would move some of the threads
along.  One or two others I will need to digest and think about a little more.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:188:
body_hover_animation_.reset(new gfx::SlideAnimation(this));
On 2016/03/31 21:07:38, Evan Stade wrote:
> On 2016/03/31 19:39:03, bruthig wrote:
> > Is this not needed for non-md?
> 
> no, this file is md-only.

Acknowledged.

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:510:
ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);
On 2016/03/31 21:07:38, Evan Stade wrote:
> On 2016/03/31 19:39:02, bruthig wrote:
> > I would expect the QUICK_ACTION trigger to be tied closely to the
> OpenDownload()
> > action.  Would it make sense to move this into the OpenDownload() call?  The
> > added benefit there is that a keyboard action will also trigger the ripple,
> > which is what we want.
> 
> I only did this because of this comment above the enum value:
> 
>   // The quick action for the view has been triggered. e.g. a tap gesture to
>   // click a button.

If both event handling methods delegate to a common function like OpenDownload()
then it should be moved to that common spot.  Feel free to update that enum
comment to something less confusing.

> 
> I can add it to OnKeyEvent as well, although I'm not sure how to focus the
> download items in the first place.

You can use F6 to cycle between the different child Views of the BrowserView ;)

> 
> If I move this to OpenDownload, then clicking with a mouse creates a double
> ripple effect.

That's odd, I would add some logging or set a breakpoint in
FloodFillInkDropAnimation::AnimateToState() to see what state changes are being
triggered and from where.

It would be nice to have this logging in place with a switch to turn it on/off. 
Is there already some logging framework to do this?  Like 'trace' logs or
something?

As an aside, I was also seeing a double ripple on the drop down button as well.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:105: float
CalculateLargestDistanceToCorners(const gfx::Size& size,
> With this change, the speed is not constant WRT the size of the view, it is
> constant WRT the location of the click. It is still slower for smaller views
and
> faster for larger views.

I need to digest this a bit more.

> 
> I feel pretty confident it looks bad for the ripple speed to change as you
mouse
> around the view and click repeatedly in different places.

Fair point, I will have to play with it a bit more.

Ultimately though, if we made the durations dynamically calculated then we would
be able to A. keep a constant speed and B. have the ripple fading out as it hits
the bounds of the View.  These are really my only two goals.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:189:
AnimateToTransform(transform, GetAnimationDuration(HIDDEN_TRANSFORM),
On 2016/03/31 21:07:38, Evan Stade wrote:
> On 2016/03/31 19:39:03, bruthig wrote:
> > On 2016/03/31 03:41:27, Evan Stade wrote:
> > > I got rid of this because it looked weird on the shelf items and didn't
> match
> > > polymer buttons[1], which fade while continuing to expand.
> > > 
> > > [1]
> > >
> >
>
https://elements.polymer-project.org/elements/paper-button?view=demo:demo/ind...
> > 
> > This was a very intentional choice and I feel like it is a rough edge in the
> > Polymer spec.  It's main purpose is to make the ripple visually distinct for
> > when a user aborts a pending action compared to when a QUICK_ACTION is
> > triggered.  Without it these two actions are indistinguishable.
> > (Oddly enough I'm having a tough time triggering it, I may have to
> investigate)
> 
> IIUC, you're saying that there's no visual distinction between click and
click,
> drag, release, which is a common way to cancel the action when the view is a
> button. Yet traditional button press effects don't distinguish these two cases
> either. They just switch between pressed and not-pressed.

Correct.  You can try this out on the "Home" button as well.  I was trying to
mimic that implosion effect for the FloodFill style here.

> 
> > 
> > What looked weird about it?
> 
> To my knowledge, the analogy to real life is a drop of water falling into a
> larger, placid body of water. The ripple expands outwards then fades away. It
> never contracts or rewinds. The polymer buttons match this and look natural to
> me (your opinion may differ). I also think it would be nicer if we supported
> multiple simultaneous ripples.
> 

I still have the proof of concept for multiple ripples and definitely think it's
something we should entertain but unfortunately there have been more pressing
issues.

> > And was the button actually being triggered or
> > aborted?
> 
> triggered.

If the button action is triggered then the QUICK_ACTION animation should be
finishing off at a hidden opacity and there should be no visual effect from this
code.  It should essentially be a no-op.  The only time I expect there to be an
effect is in the 'cancel' situations.  This could be caused by the extra HIDDEN
trigger in DownloadItemViewMd::HandleClickEvent().

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

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:116:
CreateInkDropAnimation();
On 2016/03/31 21:07:38, Evan Stade wrote:
> On 2016/03/31 19:39:03, bruthig wrote:
> > On 2016/03/31 03:41:27, Evan Stade wrote:
> > > this change probably warrants a rename of the fn. What do you think of the
> > > change itself? It is needed because otherwise a second click before the
> first
> > > ink drop is totally faded (and destroyed on L171) will reuse the old ink
> drop,
> > > which has the original center point. We need to create a new ink drop for
> > every
> > > hidden->not hidden animation.
> > 
> > I was really, really hoping not to tie the lifetime of an ink drop to a
change
> > in the center point.  In this particular case we will be incurring an extra
> cost
> > of Layer destruction/creation when ideally the existing ripple could just be
> > re-used.
> 
> I don't think that's a perf optimization worth worrying about. Perf is
important
> when many things are happening at once or happening automatically. Since this
> happens at most once per user gesture, I doubt this could ever be a problem.
> Plus, we currently create a new inkdrop every time the old inkdrop is finished
> (this change only affects what we do when the existing effect hasn't completed
> yet).
> 
> In any case, the way I would go about this if I were worried about it is to
make
> InkDropHost::CreateInkDropAnimation pass in the old animation, and let it
either
> update that ink drop or return a new one, as needed.
> 
> Also note that the center point can change regardless of whether the ripple is
> centered on the click event. A bookmark button can grow or shrink when you
> change its title. We have to refresh or replace the ripple even in the
> centerpoin-centered case.

Acknowledged.  At one point I was flirting with the idea of having the
InkDropHost be able to call a Layout like function on the InkDrop for situations
like this, but I haven't pursued this much further than that.

> 
> > 
> > Also I was really, really hoping that not every ripple surface would have to
> > track the |last_ink_drop_location_| for a couple of reasons.
> 
> Clearly not every ripple surface does need to. So far, only one out of many.

Well we know of at least two others in the Bookmarks bar...

> 
> > 
> > 1. I think it will be difficult to maintain and track the ripple center
> > correctly when taking in to account ripples should be activated on keyboard
> > input and ripples are triggered up and down the Button inheritance hierarchy
> as
> > well as from the (Button)InkDropDelegate.
> 
> I don't see the difficulty. Keyboard input can reset the location to the
center.

How do you foresee the ButtonInkDropDelegate::OnGestureEvent() communicating the
last touch point?

> 
> > 
> > 2. The plan is to suppress ripples for touch events on Windows.  It would be
> > great if this decision was captured in one spot, e.g. the
> > InkDropAnimationControllerImpl.  I haven't flushed out all the details but
was
> > thinking the InkDropDelegate::OnAction() function could also accept a
> ui::Event
> > parameter and then it could also use this to set the ripples center point.
> > 
> > Long story short, I'm hesitant to moving forward with this pattern for
> centering
> > ripples without at least discussing alternatives.
>

https://codereview.chromium.org/1832963002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:842:
ink_drop_delegate_.OnAction(views::InkDropState::HIDDEN);
Is this necessary?  The QUICK_ACTION animations should auto transition to HIDDEN
as per InkDropAnimationControllerImpl::InkDropAnimationEnded().  If this is
necessary what is the previous InkDropState that this will be transitioning
from?

[Evan Stade, 2016-04-01 19:03:13]

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:510:
ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);
On 2016/03/31 22:46:19, bruthig wrote:
> On 2016/03/31 21:07:38, Evan Stade wrote:
> > On 2016/03/31 19:39:02, bruthig wrote:
> > > I would expect the QUICK_ACTION trigger to be tied closely to the
> > OpenDownload()
> > > action.  Would it make sense to move this into the OpenDownload() call? 
The
> > > added benefit there is that a keyboard action will also trigger the
ripple,
> > > which is what we want.
> > 
> > I only did this because of this comment above the enum value:
> > 
> >   // The quick action for the view has been triggered. e.g. a tap gesture to
> >   // click a button.
> 
> If both event handling methods delegate to a common function like
OpenDownload()
> then it should be moved to that common spot.  Feel free to update that enum
> comment to something less confusing.
> 
> > 
> > I can add it to OnKeyEvent as well, although I'm not sure how to focus the
> > download items in the first place.
> 
> You can use F6 to cycle between the different child Views of the BrowserView
;)
> 
> > 
> > If I move this to OpenDownload, then clicking with a mouse creates a double
> > ripple effect.
> 
> That's odd, I would add some logging or set a breakpoint in
> FloodFillInkDropAnimation::AnimateToState() to see what state changes are
being
> triggered and from where.

Why is that odd? We're calling OnAction twice in the click case.

> 
> It would be nice to have this logging in place with a switch to turn it
on/off. 
> Is there already some logging framework to do this?  Like 'trace' logs or
> something?

see https://chromium.googlesource.com/chromium/src/+/master/base/logging.h

> 
> As an aside, I was also seeing a double ripple on the drop down button as
well.

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (left):

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/floo...
ui/views/animation/flood_fill_ink_drop_animation.cc:189:
AnimateToTransform(transform, GetAnimationDuration(HIDDEN_TRANSFORM),
On 2016/03/31 22:46:19, bruthig wrote:
> On 2016/03/31 21:07:38, Evan Stade wrote:
> > On 2016/03/31 19:39:03, bruthig wrote:
> > > On 2016/03/31 03:41:27, Evan Stade wrote:
> > > > I got rid of this because it looked weird on the shelf items and didn't
> > match
> > > > polymer buttons[1], which fade while continuing to expand.
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://elements.polymer-project.org/elements/paper-button?view=demo:demo/ind...
> > > 
> > > This was a very intentional choice and I feel like it is a rough edge in
the
> > > Polymer spec.  It's main purpose is to make the ripple visually distinct
for
> > > when a user aborts a pending action compared to when a QUICK_ACTION is
> > > triggered.  Without it these two actions are indistinguishable.
> > > (Oddly enough I'm having a tough time triggering it, I may have to
> > investigate)
> > 
> > IIUC, you're saying that there's no visual distinction between click and
> click,
> > drag, release, which is a common way to cancel the action when the view is a
> > button. Yet traditional button press effects don't distinguish these two
cases
> > either. They just switch between pressed and not-pressed.
> 
> Correct.  You can try this out on the "Home" button as well.  I was trying to
> mimic that implosion effect for the FloodFill style here.
> 
> > 
> > > 
> > > What looked weird about it?
> > 
> > To my knowledge, the analogy to real life is a drop of water falling into a
> > larger, placid body of water. The ripple expands outwards then fades away.
It
> > never contracts or rewinds. The polymer buttons match this and look natural
to
> > me (your opinion may differ). I also think it would be nicer if we supported
> > multiple simultaneous ripples.
> > 
> 
> I still have the proof of concept for multiple ripples and definitely think
it's
> something we should entertain but unfortunately there have been more pressing
> issues.
> 
> > > And was the button actually being triggered or
> > > aborted?
> > 
> > triggered.
> 
> If the button action is triggered then the QUICK_ACTION animation should be
> finishing off at a hidden opacity and there should be no visual effect from
this
> code.  It should essentially be a no-op.  The only time I expect there to be
an
> effect is in the 'cancel' situations.  This could be caused by the extra
HIDDEN
> trigger in DownloadItemViewMd::HandleClickEvent().

ah ok. So if I change DownloadItemViewMd's sequence for click events from:
ACTION_PENDING, HIDDEN
to
ACTION_PENDING, QUICK_ACTION

the effect looks good

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

https://codereview.chromium.org/1832963002/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:116:
CreateInkDropAnimation();
On 2016/03/31 22:46:19, bruthig wrote:
> On 2016/03/31 21:07:38, Evan Stade wrote:
> > On 2016/03/31 19:39:03, bruthig wrote:
> > > On 2016/03/31 03:41:27, Evan Stade wrote:
> > > > this change probably warrants a rename of the fn. What do you think of
the
> > > > change itself? It is needed because otherwise a second click before the
> > first
> > > > ink drop is totally faded (and destroyed on L171) will reuse the old ink
> > drop,
> > > > which has the original center point. We need to create a new ink drop
for
> > > every
> > > > hidden->not hidden animation.
> > > 
> > > I was really, really hoping not to tie the lifetime of an ink drop to a
> change
> > > in the center point.  In this particular case we will be incurring an
extra
> > cost
> > > of Layer destruction/creation when ideally the existing ripple could just
be
> > > re-used.
> > 
> > I don't think that's a perf optimization worth worrying about. Perf is
> important
> > when many things are happening at once or happening automatically. Since
this
> > happens at most once per user gesture, I doubt this could ever be a problem.
> > Plus, we currently create a new inkdrop every time the old inkdrop is
finished
> > (this change only affects what we do when the existing effect hasn't
completed
> > yet).
> > 
> > In any case, the way I would go about this if I were worried about it is to
> make
> > InkDropHost::CreateInkDropAnimation pass in the old animation, and let it
> either
> > update that ink drop or return a new one, as needed.
> > 
> > Also note that the center point can change regardless of whether the ripple
is
> > centered on the click event. A bookmark button can grow or shrink when you
> > change its title. We have to refresh or replace the ripple even in the
> > centerpoin-centered case.
> 
> Acknowledged.  At one point I was flirting with the idea of having the
> InkDropHost be able to call a Layout like function on the InkDrop for
situations
> like this, but I haven't pursued this much further than that.
> 
> > 
> > > 
> > > Also I was really, really hoping that not every ripple surface would have
to
> > > track the |last_ink_drop_location_| for a couple of reasons.
> > 
> > Clearly not every ripple surface does need to. So far, only one out of many.
> 
> Well we know of at least two others in the Bookmarks bar...
> 
> > 
> > > 
> > > 1. I think it will be difficult to maintain and track the ripple center
> > > correctly when taking in to account ripples should be activated on
keyboard
> > > input and ripples are triggered up and down the Button inheritance
hierarchy
> > as
> > > well as from the (Button)InkDropDelegate.
> > 
> > I don't see the difficulty. Keyboard input can reset the location to the
> center.
> 
> How do you foresee the ButtonInkDropDelegate::OnGestureEvent() communicating
the
> last touch point?

I guess ink drop delegate can store the last touch point and expose a getter.

> 
> > 
> > > 
> > > 2. The plan is to suppress ripples for touch events on Windows.  It would
be
> > > great if this decision was captured in one spot, e.g. the
> > > InkDropAnimationControllerImpl.  I haven't flushed out all the details but
> was
> > > thinking the InkDropDelegate::OnAction() function could also accept a
> > ui::Event
> > > parameter and then it could also use this to set the ripples center point.
> > > 
> > > Long story short, I'm hesitant to moving forward with this pattern for
> > centering
> > > ripples without at least discussing alternatives.
> > 
>

https://codereview.chromium.org/1832963002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:842:
ink_drop_delegate_.OnAction(views::InkDropState::HIDDEN);
On 2016/03/31 22:46:19, bruthig wrote:
> Is this necessary?  The QUICK_ACTION animations should auto transition to
HIDDEN
> as per InkDropAnimationControllerImpl::InkDropAnimationEnded().  If this is
> necessary what is the previous InkDropState that this will be transitioning
> from?

ok, no it's not.

[Evan Stade, 2016-04-01 19:04:08]

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/download/download_item_view_md.cc (right):

https://codereview.chromium.org/1832963002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/download/download_item_view_md.cc:510:
ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);
On 2016/04/01 19:03:13, Evan Stade wrote:
> Why is that odd? We're calling OnAction twice in the click case.

you can ignore this comment -- I understand better now.

[Evan Stade, 2016-04-01 19:14:10]

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

[commit-bot: I haz the power, 2016-04-01 19:14:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832963002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832963002/100001

[Evan Stade, 2016-04-01 20:45:38]

updated

[Evan Stade, 2016-04-01 20:45:46]

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

[commit-bot: I haz the power, 2016-04-01 20:46:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832963002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832963002/120001

[commit-bot: I haz the power, 2016-04-01 22:18:56]

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

[commit-bot: I haz the power, 2016-04-01 22:18:57]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-04-04 17:20:17]

lgtm

https://codereview.chromium.org/1832963002/diff/120001/ui/views/animation/flo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (right):

https://codereview.chromium.org/1832963002/diff/120001/ui/views/animation/flo...
ui/views/animation/flood_fill_ink_drop_animation.cc:317: gfx::Transform
FloodFillInkDropAnimation::GetActivatedTargetTransform() const {
nit: It probably makes sense to rename this now that it is being used for
InkDropStates other than ACTIVATED.  Maybe something like
GetMaxSizeTargetTransform/GetTargetTransformForFloodFill/etc.

[Evan Stade, 2016-04-04 18:09:08]

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

[Evan Stade, 2016-04-04 18:09:09]

+sky for OWNERS on everything that doesn't have "ink" in the name (most touched
files are just enum value renames) and in particular, review of
download_item_view_md.*

https://codereview.chromium.org/1832963002/diff/120001/ui/views/animation/flo...
File ui/views/animation/flood_fill_ink_drop_animation.cc (right):

https://codereview.chromium.org/1832963002/diff/120001/ui/views/animation/flo...
ui/views/animation/flood_fill_ink_drop_animation.cc:317: gfx::Transform
FloodFillInkDropAnimation::GetActivatedTargetTransform() const {
On 2016/04/04 17:20:17, bruthig wrote:
> nit: It probably makes sense to rename this now that it is being used for
> InkDropStates other than ACTIVATED.  Maybe something like
> GetMaxSizeTargetTransform/GetTargetTransformForFloodFill/etc.

Done.

[Evan Stade, 2016-04-04 20:17:32]

Description was changed from

==========
MD - add ripples to DL shelf items

See bug for list of unresolved TODOs.

BUG=591582
==========

to

==========
MD - add ripples to DL shelf items

BUG=591582
==========

[sky, 2016-04-04 20:50:26]

LGTM

[Evan Stade, 2016-04-04 22:11:05]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-04-04 22:11:06]

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/1832963002/#ps180001
(title: "fix another TODO")

[commit-bot: I haz the power, 2016-04-04 22:11:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832963002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832963002/180001

[commit-bot: I haz the power, 2016-04-04 23:28:58]

Description was changed from

==========
MD - add ripples to DL shelf items

BUG=591582
==========

to

==========
MD - add ripples to DL shelf items

BUG=591582
==========

[commit-bot: I haz the power, 2016-04-04 23:29:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-04 23:30:07]

Description was changed from

==========
MD - add ripples to DL shelf items

BUG=591582
==========

to

==========
MD - add ripples to DL shelf items

BUG=591582

Committed: https://crrev.com/15bda587d910b5d9f59b7184ecfeda37022eb6f4
Cr-Commit-Position: refs/heads/master@{#385052}
==========

[commit-bot: I haz the power, 2016-04-04 23:30:08]

Patchset 10 (id:??) landed as
https://crrev.com/15bda587d910b5d9f59b7184ecfeda37022eb6f4
Cr-Commit-Position: refs/heads/master@{#385052}