MD - add ripples to DL shelf items

chrome/browser/ui/views/download/download_item_view_md.cc

Index: chrome/browser/ui/views/download/download_item_view_md.cc
diff --git a/chrome/browser/ui/views/download/download_item_view_md.cc b/chrome/browser/ui/views/download/download_item_view_md.cc
index fa2da545bbf5740539589c2e260dcbed43a7352e..82eb8c2bb0e15f841ffcf62289a3bb71f85e3f52 100644
--- a/chrome/browser/ui/views/download/download_item_view_md.cc
+++ b/chrome/browser/ui/views/download/download_item_view_md.cc
@@ -57,7 +57,9 @@
 #include "ui/gfx/text_elider.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/gfx/vector_icons_public.h"
+#include "ui/views/animation/flood_fill_ink_drop_animation.h"
 #include "ui/views/animation/ink_drop_delegate.h"
+#include "ui/views/animation/ink_drop_hover.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/label_button.h"
@@ -158,6 +160,7 @@ DownloadItemViewMd::DownloadItemViewMd(DownloadItem* download_item,
       dragging_(false),
       starting_drag_(false),
       model_(download_item),
+      ink_drop_delegate_(this, this),
       save_button_(nullptr),
       discard_button_(nullptr),
       dropdown_button_(new BarControlButton(this)),
@@ -185,9 +188,6 @@ DownloadItemViewMd::DownloadItemViewMd(DownloadItem* download_item,
   status_font_list_ =
       rb.GetFontList(ui::ResourceBundle::BaseFont).DeriveWithSizeDelta(-2);
 
-  body_hover_animation_.reset(new gfx::SlideAnimation(this));

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

Is this not needed for non-md?

[Evan Stade, 2016/03/31 21:07:38]

no, this file is md-only.

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

Acknowledged.

-  drop_hover_animation_.reset(new gfx::SlideAnimation(this));
-
   SetAccessibilityFocusable(true);
 
   OnDownloadUpdated(download());
@@ -376,10 +376,12 @@ gfx::Size DownloadItemViewMd::GetPreferredSize() const {
                                    2 * kMinimumVerticalPadding + child_height));
 }
 
-// Handle a mouse click and open the context menu if the mouse is
-// over the drop-down region.
 bool DownloadItemViewMd::OnMousePressed(const ui::MouseEvent& event) {
   HandlePressEvent(event, event.IsOnlyLeftMouseButton());
+  if (!IsShowingWarningDialog()) {
+    last_ink_drop_location_ = event.location();
+    ink_drop_delegate_.OnAction(views::InkDropState::ACTION_PENDING);
+  }
   return true;
 }
 
@@ -401,6 +403,7 @@ bool DownloadItemViewMd::OnMouseDragged(const ui::MouseEvent& event) {
       views::Widget* widget = GetWidget();
       DragDownloadItem(download(), icon,
                        widget ? widget->GetNativeView() : NULL);
+      ink_drop_delegate_.OnAction(views::InkDropState::ACTIVATED);

[bruthig, 2016/03/31 19:39:02]

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.

[Evan Stade, 2016/03/31 21:07:38]

ok, done

     }
   } else if (ExceededDragThreshold(event.location() - drag_start_point_)) {
     dragging_ = true;
@@ -410,11 +413,16 @@ bool DownloadItemViewMd::OnMouseDragged(const ui::MouseEvent& event) {
 
 void DownloadItemViewMd::OnMouseReleased(const ui::MouseEvent& event) {
   HandleClickEvent(event, event.IsOnlyLeftMouseButton());
+
+  if (!IsShowingWarningDialog()) {
+    ink_drop_delegate_.OnAction(views::InkDropState::HIDDEN);

[bruthig, 2016/03/31 19:39:02]

Does it make sense to move this to the HandleClickEvent() method?

[Evan Stade, 2016/03/31 21:07:38]

Done.

+    ink_drop_delegate_.SetHovered(IsMouseHovered());

[bruthig, 2016/03/31 19:39:02]

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?

[Evan Stade, 2016/03/31 21:07:38]

doesn't seem to really do anything. Removed.

+  }
 }
 
 void DownloadItemViewMd::OnMouseCaptureLost() {
   // Mouse should not activate us in dangerous mode.
-  if (mode_ == DANGEROUS_MODE)
+  if (mode_ != NORMAL_MODE)
     return;
 
   if (dragging_) {
@@ -424,6 +432,15 @@ void DownloadItemViewMd::OnMouseCaptureLost() {
   }
 }
 
+void DownloadItemViewMd::OnMouseEntered(const ui::MouseEvent& event) {
+  if (!IsShowingWarningDialog())
+    ink_drop_delegate_.SetHovered(true);

[bruthig, 2016/03/31 19:39:02]

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.

[Evan Stade, 2016/03/31 21:07:38]

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

+}
+
+void DownloadItemViewMd::OnMouseExited(const ui::MouseEvent& event) {
+  ink_drop_delegate_.SetHovered(false);
+}
+
 bool DownloadItemViewMd::OnKeyPressed(const ui::KeyEvent& event) {
   // Key press should not activate us in dangerous mode.
   if (IsShowingWarningDialog())
@@ -450,6 +467,10 @@ bool DownloadItemViewMd::GetTooltipText(const gfx::Point& p,
   return true;
 }
 
+void DownloadItemViewMd::OnDragDone() {
+  ink_drop_delegate_.OnAction(views::InkDropState::DEACTIVATED);

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

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.

[Evan Stade, 2016/03/31 21:07:38]

removed

+}
+
 void DownloadItemViewMd::GetAccessibleState(ui::AXViewState* state) {
   state->name = accessible_name_;
   state->role = ui::AX_ROLE_BUTTON;
@@ -463,10 +484,31 @@ void DownloadItemViewMd::OnThemeChanged() {
   UpdateColorsFromTheme();
 }
 
+scoped_ptr<views::InkDropAnimation> DownloadItemViewMd::CreateInkDropAnimation()
+    const {
+  return make_scoped_ptr(new views::FloodFillInkDropAnimation(
+      size(), last_ink_drop_location_,
+      color_utils::DeriveDefaultIconColor(GetTextColor())));
+}
+
+scoped_ptr<views::InkDropHover> DownloadItemViewMd::CreateInkDropHover() const {
+  // TODO(estade): when the item is animating open and the user hovers, the

[bruthig, 2016/03/31 19:39:02]

Is the size still stuck if you mouse off and mouse on again?

[Evan Stade, 2016/03/31 21:07:38]

nope

+  // effect gets stuck at that size and does not grow with the item. Using
+  // GetPreferredSize() instead of size() here does not fix it.
+  return make_scoped_ptr(new views::InkDropHover(
+      size(), kInkDropSmallCornerRadius,
+      GetLocalBounds().CenterPoint(),
+      color_utils::DeriveDefaultIconColor(GetTextColor())));
+}
+
 void DownloadItemViewMd::OnGestureEvent(ui::GestureEvent* event) {
   if (event->type() == ui::ET_GESTURE_TAP_DOWN) {
     HandlePressEvent(*event, true);
     event->SetHandled();
+    if (mode_ == NORMAL_MODE) {
+      last_ink_drop_location_ = event->location();
+      ink_drop_delegate_.OnAction(views::InkDropState::QUICK_ACTION);

[bruthig, 2016/03/31 19:39:02]

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.

[Evan Stade, 2016/03/31 21:07:38]

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.

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

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.
You can use F6 to cycle between the different child Views of the BrowserView ;)
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.

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

Why is that odd? We're calling OnAction twice in the click case.
see https://chromium.googlesource.com/chromium/src/+/master/base/logging.h

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

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

+    }
     return;
   }
 
@@ -1073,10 +1115,10 @@ void DownloadItemViewMd::ProgressTimerFired() {
     SchedulePaint();
 }
 
-SkColor DownloadItemViewMd::GetTextColor() {
+SkColor DownloadItemViewMd::GetTextColor() const {
   return GetTextColorForThemeProvider(GetThemeProvider());
 }
 
-SkColor DownloadItemViewMd::GetDimmedTextColor() {
+SkColor DownloadItemViewMd::GetDimmedTextColor() const {
   return SkColorSetA(GetTextColor(), 0xC7);
 }