Adding ripple effect for clicks on MD slider

ui/views/controls/md_slider.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/controls/md_slider.h"
 
 #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"
 #include "ui/views/controls/slider.h"
 
 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();
 }
 
 MdSlider::~MdSlider() {}
 
 void MdSlider::OnPaint(gfx::Canvas* canvas) {
   Slider::OnPaint(canvas);
 
   // Paint the slider.
   const int thumb_size = kThumbRadius * 2;
@@ skipping 13 matching lines @@
   paint.setColor(is_active_ ? kActiveColor : kDisabledColor);
   paint.setFlags(SkPaint::kAntiAlias_Flag);
 
   if (!is_active_) {
     paint.setStrokeWidth(kThumbStroke);
     paint.setStyle(SkPaint::kStroke_Style);
   }
   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 {
   return "MdSlider";
 }
 
 void MdSlider::UpdateState(bool control_on) {
   is_active_ = control_on;
   SchedulePaint();
 }
 
 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