[Views] Update ink drop for omnibox icons

chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

Index: chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
diff --git a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
index 38e36e746a84a6d9b330689fb76cc94cde13d89a..ec607846b56e92d22e500214fe04ecdc5765b6bc 100644
--- a/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
+++ b/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
@@ -8,11 +8,16 @@
 #include "chrome/browser/ui/views/location_bar/background_with_1_px_border.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "ui/accessibility/ax_node_data.h"
+#include "ui/compositor/layer_animator.h"
+#include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/scoped_canvas.h"
 #include "ui/native_theme/native_theme.h"
+#include "ui/views/animation/flood_fill_ink_drop_ripple.h"
 #include "ui/views/animation/ink_drop_highlight.h"
+#include "ui/views/animation/ink_drop_impl.h"
+#include "ui/views/animation/ink_drop_ripple.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/widget/widget.h"
@@ -22,12 +27,80 @@ namespace {
 // Amount of space on either side of the separator that appears after the label.
 constexpr int kSpaceBesideSeparator = 8;
 
+// The length of the separator's fade animation.
+constexpr int kFadeInDurationMs = 250;
+constexpr int kFadeOutDurationMs = 175;
+
 }  // namespace
 
+//////////////////////////////////////////////////////////////////
+// SeparatorView class
+
+IconLabelBubbleView::SeparatorView::SeparatorView(IconLabelBubbleView* owner) {
+  DCHECK(owner);
+  owner_ = owner;
+  SetPaintToLayer();

[bruthig, 2017/03/14 03:49:39]

Can this use a LAYER_SOLID_COLOR LayerType instead of the default LAYER_TEXTURED
type?

[spqchan, 2017/03/21 18:25:22]

I can do that, but I'll have to set the width of the separator to exactly one
pixel. Will that have a different effect from calling Draw1pxLine?

[bruthig, 2017/03/22 17:20:16]

I spoke with sadrul@ offline and he wasn't sure how to force a 1px wide line
using Layers.  So ignore this request.

+  layer()->SetFillsBoundsOpaquely(false);
+}
+
+void IconLabelBubbleView::SeparatorView::OnPaint(gfx::Canvas* canvas) {
+  if (!owner_->ShouldShowLabel())
+    return;
+
+  const SkColor plain_text_color = owner_->GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_TextfieldDefaultColor);
+  const SkColor separator_color = SkColorSetA(
+      plain_text_color, color_utils::IsDark(plain_text_color) ? 0x59 : 0xCC);
+
+  gfx::Rect bounds(owner_->label()->bounds());
+  const int kSeparatorHeight = 16;
+  bounds.Inset(0, (bounds.height() - kSeparatorHeight) / 2);
+
+  // Draw the 1 px separator.
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const float scale = canvas->UndoDeviceScaleFactor();
+  // Keep the separator aligned on a pixel center.
+  const gfx::RectF pixel_aligned_bounds =
+      gfx::ScaleRect(gfx::RectF(bounds), scale) - gfx::Vector2dF(0.5f, 0);
+  canvas->DrawLine(pixel_aligned_bounds.top_right(),
+                   pixel_aligned_bounds.bottom_right(), separator_color);
+}
+
+bool IconLabelBubbleView::SeparatorView::CanProcessEventsWithinSubtree() const {
+  return false;
+}
+
+void IconLabelBubbleView::SeparatorView::UpdateOpacity() {
+  views::InkDrop* ink_drop = owner_->GetInkDrop();
+  DCHECK(ink_drop);
+
+  // If an inkdrop highlight or ripple is animating in or visible, the
+  // separator should

[bruthig, 2017/03/14 03:49:41]

Incomplete sentence?

[spqchan, 2017/03/21 18:25:23]

Done.

+  views::InkDropState state = ink_drop->GetTargetInkDropState();
+  float opacity = 0.0f;
+  float duration = kFadeOutDurationMs;
+  if (!ink_drop->IsHighlightFadingInOrVisible() &&
+      (state == views::InkDropState::HIDDEN ||
+       state == views::InkDropState::DEACTIVATED)) {
+    opacity = 1.0f;
+    duration = kFadeInDurationMs;
+  }
+
+  ui::ScopedLayerAnimationSettings animation(layer()->GetAnimator());
+  animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(duration));
+  animation.SetTweenType(gfx::Tween::Type::EASE_IN);
+  layer()->SetOpacity(opacity);
+}
+
+//////////////////////////////////////////////////////////////////
+// IconLabelBubbleView class
+
 IconLabelBubbleView::IconLabelBubbleView(const gfx::FontList& font_list,
                                          bool elide_in_middle)
     : image_(new views::ImageView()),
-      label_(new views::Label(base::string16(), font_list)) {
+      label_(new views::Label(base::string16(), font_list)),
+      ink_drop_container_(new views::InkDropContainerView()),
+      suppress_mouse_released_action_(false) {
   // Disable separate hit testing for |image_|.  This prevents views treating
   // |image_| as a separate mouse hover region from |this|.
   image_->set_interactive(false);
@@ -41,6 +114,12 @@ IconLabelBubbleView::IconLabelBubbleView(const gfx::FontList& font_list,
     label_->SetElideBehavior(gfx::ELIDE_MIDDLE);
   AddChildView(label_);
 
+  separator_view_.reset(new SeparatorView(this));
+  AddChildView(separator_view_.get());
+
+  AddChildView(ink_drop_container_);
+  ink_drop_container_->SetVisible(false);

[bruthig, 2017/03/14 03:49:40]

Is SetVisible(true) ever called on the |ink_drop_container_|, I don't see it
anywhere?  Assuming it's not, it surprises me that the highlight and ripple are
ever visible.

[spqchan, 2017/03/21 18:25:23]

Yes:
https://cs.chromium.org/chromium/src/ui/views/animation/ink_drop.cc?type=cs&q...

+
   // Bubbles are given the full internal height of the location bar so that all
   // child views in the location bar have the same height. The visible height of
   // the bubble should be smaller, so use an empty border to shrink down the
@@ -55,6 +134,10 @@ IconLabelBubbleView::IconLabelBubbleView(const gfx::FontList& font_list,
 IconLabelBubbleView::~IconLabelBubbleView() {
 }
 
+void IconLabelBubbleView::AnimationStarted() {
+  separator_view_->UpdateOpacity();
+}
+
 void IconLabelBubbleView::SetLabel(const base::string16& label) {
   label_->SetText(label);
 }
@@ -79,6 +162,14 @@ bool IconLabelBubbleView::OnActivate(const ui::Event& event) {
   return false;
 }
 
+bool IconLabelBubbleView::IsBubbleShown() const {
+  return false;
+}
+
+bool IconLabelBubbleView::CanClick() const {
+  return false;
+}
+
 gfx::Size IconLabelBubbleView::GetPreferredSize() const {
   // Height will be ignored by the LocationBarView.
   return GetSizeForLabelWidth(label_->GetPreferredSize().width());
@@ -120,6 +211,30 @@ void IconLabelBubbleView::Layout() {
   const int label_width =
       std::max(0, width() - label_x - bubble_trailing_padding);
   label_->SetBounds(label_x, 0, label_width, height());
+  separator_view_->SetBoundsRect(GetLocalBounds());
+  ink_drop_container_->SetBoundsRect(GetLocalBounds());
+}
+
+bool IconLabelBubbleView::OnMousePressed(const ui::MouseEvent& event) {
+  suppress_mouse_released_action_ = IsBubbleShown() || !CanClick();
+  if (!suppress_mouse_released_action_)
+    AnimateInkDrop(views::InkDropState::ACTION_PENDING, &event);
+
+  return true;
+}
+
+void IconLabelBubbleView::OnMouseReleased(const ui::MouseEvent& event) {

[bruthig, 2017/03/14 03:49:43]

Does the InkDropHostView::InkDropGestureHandler work correctly for touch
interactions with these views?

[spqchan, 2017/03/21 18:25:22]

That's a good point. I made changes so that CustomButton now handles the gesture
events.

+  if (suppress_mouse_released_action_) {
+    suppress_mouse_released_action_ = false;
+    return;
+  }
+
+  if (HitTestPoint(event.location())) {
+    AnimateInkDrop(views::InkDropState::ACTIVATED, &event);

[bruthig, 2017/03/14 03:49:42]

Should it really animate to ACTIVATED if OnActivate() returns false?

[spqchan, 2017/03/21 18:25:22]

This is now dealt by CustomButton

+    OnActivate(event);
+  } else {
+    AnimateInkDrop(views::InkDropState::DEACTIVATED, &event);

[bruthig, 2017/03/14 03:49:38]

I think you want the HIDDEN state here, this will cause a PENDING ripple to
shrink, which is used on other buttons to let the user know the 'click' was
canceled.

[spqchan, 2017/03/21 18:25:23]

Done.

+  }
 }
 
 void IconLabelBubbleView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
@@ -134,25 +249,73 @@ void IconLabelBubbleView::OnNativeThemeChanged(
 }
 
 void IconLabelBubbleView::AddInkDropLayer(ui::Layer* ink_drop_layer) {
-  image()->SetPaintToLayer();
-  image()->layer()->SetFillsBoundsOpaquely(false);
-  InkDropHostView::AddInkDropLayer(ink_drop_layer);
+  ink_drop_container_->AddInkDropLayer(ink_drop_layer);
 }
 
 void IconLabelBubbleView::RemoveInkDropLayer(ui::Layer* ink_drop_layer) {
-  InkDropHostView::RemoveInkDropLayer(ink_drop_layer);
-  image()->DestroyLayer();
+  ink_drop_container_->RemoveInkDropLayer(ink_drop_layer);
+}
+
+std::unique_ptr<views::InkDrop> IconLabelBubbleView::CreateInkDrop() {
+  std::unique_ptr<views::InkDropImpl> ink_drop =
+      CreateDefaultFloodFillInkDropImpl();
+  ink_drop->SetShowHighlightOnFocus(true);
+  ink_drop->SetObserver(this);
+  return std::move(ink_drop);
 }
 
 std::unique_ptr<views::InkDropHighlight>
 IconLabelBubbleView::CreateInkDropHighlight() const {
-  // Only show a highlight effect when the label is empty/invisible.
-  return label()->visible() ? nullptr
-                            : InkDropHostView::CreateInkDropHighlight();
+  if (!CanClick())

[bruthig, 2017/03/14 03:49:43]

I believe the button hierarchy prevents a highlight from being shown by calling
View::SetEnabled(false).  Would that work here as well?  I'm not sure if
Tooltips are involved or whether they are a factor to consider.

A few other questions:
- Alternatively, could you use InkDropImpl::SetShowHighlightOnHover(false) when
the view isn't clickable?
- Will CanClick() ever dynamically update it's return value while an instance
has the mouse over it?  Presumably the View should then be showing the highlight
but with this implementation I don't think it will.
- This begs the question of how the View is supposed to behave when it gets
keyboard focus.  Should the highlight be shown?

[spqchan, 2017/03/21 18:25:22]

Sounds good.

Question:
- I can try that although your first solution seems good enough
- No, that shouldn't happen
- It should be if it's focused

+    return nullptr;
+
+  gfx::PointF ink_drop_center = gfx::RectF(GetLocalBounds()).CenterPoint();
+  gfx::Size ink_drop_size = size();

[bruthig, 2017/03/14 03:49:38]

When is it valid for size() to be empty?

[spqchan, 2017/03/21 18:25:22]

It happens during tests. Now that you mentioned it, it's makes more sense to
change the test instead.

+  if (ink_drop_size.IsEmpty())
+    ink_drop_size = gfx::Size(kDefaultInkDropSize, kDefaultInkDropSize);
+
+  if (ShouldShowLabel()) {
+    int padding_offset = -GetPostSeparatorPadding();
+    ink_drop_center.Offset(padding_offset / 2, 0);
+    ink_drop_size.Enlarge(padding_offset, 0);
+  }
+
+  return InkDropHostView::CreateDefaultInkDropHighlight(ink_drop_center,
+                                                        ink_drop_size);
+}
+
+std::unique_ptr<views::InkDropRipple> IconLabelBubbleView::CreateInkDropRipple()
+    const {
+  gfx::Point ink_drop_center = GetLocalBounds().CenterPoint();
+  gfx::Size ink_drop_size = size();
+  if (ink_drop_size.IsEmpty())
+    return CreateDefaultInkDropRipple(ink_drop_center);
+
+  if (ShouldShowLabel()) {
+    int padding_offset = -GetPostSeparatorPadding();
+    ink_drop_center.Offset(padding_offset / 2, 0);
+    ink_drop_size.Enlarge(padding_offset, 0);
+  }
+
+  return base::MakeUnique<views::FloodFillInkDropRipple>(
+      ink_drop_size, ink_drop_center, GetInkDropBaseColor(),
+      ink_drop_visible_opacity());
 }
 
 SkColor IconLabelBubbleView::GetInkDropBaseColor() const {
-  return color_utils::DeriveDefaultIconColor(GetTextColor());
+  return color_utils::DeriveDefaultIconColor(GetNativeTheme()->GetSystemColor(
+      ui::NativeTheme::kColorId_TextfieldDefaultColor));
+}
+
+void IconLabelBubbleView::OnWidgetDestroying(views::Widget* widget) {
+  widget->RemoveObserver(this);
+}
+
+void IconLabelBubbleView::OnWidgetVisibilityChanged(views::Widget* widget,
+                                                    bool visible) {
+  // |widget| is a bubble that has just got shown / hidden.
+  if (!visible)
+    AnimateInkDrop(views::InkDropState::DEACTIVATED, nullptr /* event */);
 }
 
 SkColor IconLabelBubbleView::GetParentBackgroundColor() const {
@@ -216,26 +379,3 @@ float IconLabelBubbleView::GetScaleFactor() const {
 const char* IconLabelBubbleView::GetClassName() const {
   return "IconLabelBubbleView";
 }
-
-void IconLabelBubbleView::OnPaint(gfx::Canvas* canvas) {
-  if (!ShouldShowLabel())
-    return;
-
-  const SkColor plain_text_color = GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_TextfieldDefaultColor);
-  const SkColor separator_color = SkColorSetA(
-      plain_text_color, color_utils::IsDark(plain_text_color) ? 0x59 : 0xCC);
-
-  gfx::Rect bounds(label_->bounds());
-  const int kSeparatorHeight = 16;
-  bounds.Inset(0, (bounds.height() - kSeparatorHeight) / 2);
-
-  // Draw the 1 px separator.
-  gfx::ScopedCanvas scoped_canvas(canvas);
-  const float scale = canvas->UndoDeviceScaleFactor();
-  // Keep the separator aligned on a pixel center.
-  const gfx::RectF pixel_aligned_bounds =
-      gfx::ScaleRect(gfx::RectF(bounds), scale) - gfx::Vector2dF(0.5f, 0);
-  canvas->DrawLine(pixel_aligned_bounds.top_right(),
-                   pixel_aligned_bounds.bottom_right(), separator_color);
-}