Adding ripple effect for clicks on MD slider

ui/views/controls/md_slider.cc

Index: ui/views/controls/md_slider.cc
diff --git a/ui/views/controls/md_slider.cc b/ui/views/controls/md_slider.cc
index 1a58eaec55cd9ec966c6a5ea67eb260224616532..da7270bc093ef0d73f55f581770497e5b874cce7 100644
--- a/ui/views/controls/md_slider.cc
+++ b/ui/views/controls/md_slider.cc
@@ -6,6 +6,7 @@
 
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkPaint.h"
+#include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
@@ -16,18 +17,25 @@ namespace views {
 // Color of slider at the active and the disabled state, respectively.
 const SkColor kActiveColor = SkColorSetARGB(0xFF, 0x42, 0x85, 0xF4);
 const SkColor kDisabledColor = SkColorSetARGB(0x42, 0x00, 0x00, 0x00);
+const SkColor kHighlightColor = SkColorSetARGB(0x4D, 0x42, 0x85, 0xF4);
 
 // The thickness of the slider.
 const int kLineThickness = 2;
 
 // The radius of the thumb of the slider.
-const int kThumbRadius = 6;
+const float kThumbRadius = 6.0;
 
 // The stroke of the thumb when the slider is disabled.
 const int kThumbStroke = 2;
 
+// The radius and stroke of the thumb's ripple.
+const float kThumbHighlightStroke = 4.0;
+
+// Duration of the animation.

[bruthig, 2016/09/21 00:27:45]

Be more specific here.  i.e. What 'animation'?

[yiyix, 2016/09/21 15:20:52]

Sorry, I was playing with slide animation and linear animation, i was keep
changing and keep rolling back changes. Sorry to give you something incomplete
to review.

+const int kSlideValueChangeDurationMS = 150;
+
 MdSlider::MdSlider(SliderListener* listener)
-    : Slider(listener), is_active_(true) {
+    : Slider(listener), is_active_(true), animating_thumb_value_(0.f) {
   SchedulePaint();
 }
 
@@ -61,6 +69,22 @@ void MdSlider::OnPaint(gfx::Canvas* canvas) {
   canvas->DrawCircle(
       gfx::Point(content.x() + full + kThumbRadius, content.height() / 2),
       is_active_ ? kThumbRadius : (kThumbRadius - kThumbStroke / 2), paint);
+
+  if (is_active_ && value_ > 0) {

[bruthig, 2016/09/21 00:27:46]

Is it an intentional decision to paint the highlight on top of the thumb?  With
the disclaimer that I haven't seen it, I feel like it would look better if the
highlight was painted behind the thumb. i.e. move this block above the thumb
painting on line 69

[yiyix, 2016/09/21 15:20:52]

The highlight and the thumb are not over each other, they are exclusive. I did
some math on the size of stroke and the size of the thumb, so that they are
exclusive.

+    SkPaint highlight;
+    highlight.setColor(kHighlightColor);
+    highlight.setStyle(SkPaint::kStroke_Style);
+    highlight.setFlags(SkPaint::kAntiAlias_Flag);
+    float val =

[bruthig, 2016/09/21 00:27:45]

Using |highlight_radius| variable name would be more appropriate than |val|.

[yiyix, 2016/09/21 15:20:52]

Done.

+        highlight_animation_.get() && highlight_animation_->is_animating()

[bruthig, 2016/09/21 00:27:45]

It would be less confusing if the painting of the highlight wasn't dependent on
these three different variables: |value_|, |highlight_animation_|, and
|animating_thumb_value_|.  You should be able to collapse these 3 conditions
into a single |highlight_radius_| value.  It would then be the
AnimationProgressed() callback's responsibility to set this |highlight_radius_|
value.  It might make sense to discuss this one offline.

[yiyix, 2016/09/21 15:20:52]

I don't really get it, the highlight is dependent on |kThumbRadius| (fixed val)
and |highlight_radius| (variable val) only; and |highlight_radius| is determined
by animation.

As the stroke value changes, I need to change the circle radius, so the
highlight effect stays separately from the thumb drawing. we can talk about this
computation offline.

+            ? animating_thumb_value_
+            : kThumbHighlightStroke;
+
+    highlight.setStrokeWidth(val);
+    canvas->DrawCircle(
+        gfx::Point(content.x() + full + kThumbRadius, content.height() / 2),

[bruthig, 2016/09/21 00:27:45]

Consider computing the thumb center point once and reusing that value on line 70
as well as here.

[yiyix, 2016/09/21 15:20:52]

good point.

+        (kThumbRadius + val / 2), highlight);
+  }
 }
 
 const char* MdSlider::GetClassName() const {
@@ -75,4 +99,24 @@ void MdSlider::UpdateState(bool control_on) {
 int MdSlider::GetThumbWidth() {
   return kThumbRadius * 2;
 }
+
+void MdSlider::SetFocus(bool focus) {
+  if (focus) {
+    value_ = kThumbHighlightStroke;
+    animating_thumb_value_ = 0;
+    highlight_animation_.reset(new gfx::SlideAnimation(this));
+    highlight_animation_->SetSlideDuration(kSlideValueChangeDurationMS);
+    highlight_animation_->Show();
+  } else {
+    value_ = 0;

[bruthig, 2016/09/21 00:27:45]

I think it would be more visually pleasing if the highlight animated away when
disappearing.  You should be able to use the SlideAnimation::Hide() to do this.

[yiyix, 2016/09/21 15:20:52]

The animation was not really specified in the design doc, I was thinkking let
sebastien play with it and give some comments. I have added the hiding
animation.

+  }
+  SchedulePaint();
+}
+
+void MdSlider::AnimationProgressed(const gfx::Animation* animation) {
+  Slider::AnimationProgressed(animation);
+  animating_thumb_value_ =

[bruthig, 2016/09/21 00:27:45]

This should only update |animating_thumb_value_| if |animation| ==
|highlight_animation_|. And it should call SchedulePaint() when
|animating_thumb_value_| is actually updated.

[yiyix, 2016/09/21 15:20:52]

Done.

+      animation->CurrentValueBetween(animating_thumb_value_, value_);
+}
+
 }  // namespace views