Fix event targeting for overlay scrollbar thumbs (in native UI).

ui/views/controls/scrollbar/overlay_scroll_bar.cc

 // Copyright 2013 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/scrollbar/overlay_scroll_bar.h"
 
 #include "base/macros.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "third_party/skia/include/core/SkXfermode.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/gfx/canvas.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 
 namespace views {
 namespace {
 
 // Total thickness of the thumb (matches visuals when hovered).
 const int kThumbThickness = 11;
 // When hovered, the thumb takes up the full width. Otherwise, it's a bit
 // slimmer.
-const int kThumbUnhoveredDifference = 4;
+const int kThumbHoverOffset = 4;
 const int kThumbStroke = 1;
 const float kThumbHoverAlpha = 0.5f;
 const float kThumbDefaultAlpha = 0.3f;
 
 }  // namespace
 
 OverlayScrollBar::Thumb::Thumb(OverlayScrollBar* scroll_bar)
     : BaseScrollBarThumb(scroll_bar), scroll_bar_(scroll_bar) {
+  // |scroll_bar| isn't done being constructed; it's not safe to do anything

[Evan Stade, 2016/11/19 02:23:34]

different bug here --- don't do work in constructors/"this in initializer list",
learned the hard way: IsHorizontal() was returning 16 (haha) because scroll_bar
hadn't yet been constructed; this meant our first call to OnStateChanged() was
using the wrong translation.

[varkha, 2016/11/19 02:58:00]

Is this responsible for the first Thumb's paint being in the wrong spot?

[Evan Stade, 2016/11/21 15:26:20]

yes

+  // that might reference it yet.
+}
+
+OverlayScrollBar::Thumb::~Thumb() {}
+
+void OverlayScrollBar::Thumb::Init() {
   SetPaintToLayer(true);
+  layer()->SetFillsBoundsOpaquely(false);

[Evan Stade, 2016/11/19 02:23:34]

this is the fix for the graphical glitch Valery pointed out.

[varkha, 2016/11/19 02:58:00]

Yay! Glad you caught it.

   // Animate all changes to the layer except the first one.
   OnStateChanged();
   layer()->SetAnimator(ui::LayerAnimator::CreateImplicitAnimator());
 }
 
-OverlayScrollBar::Thumb::~Thumb() {}
-
 gfx::Size OverlayScrollBar::Thumb::GetPreferredSize() const {
-  return gfx::Size(kThumbThickness, kThumbThickness);
+  // The visual size of the thumb is kThumbThickness, but it slides back and
+  // forth by kThumbHoverOffset. To make event targetting work well, expand the
+  // width of the thumb such that it's always taking up the full width of the
+  // track regardless of the offset.
+  return gfx::Size(kThumbThickness + kThumbHoverOffset,
+                   kThumbThickness + kThumbHoverOffset);
 }
 
 void OverlayScrollBar::Thumb::OnPaint(gfx::Canvas* canvas) {
   SkPaint fill_paint;
   fill_paint.setStyle(SkPaint::kFill_Style);
   fill_paint.setColor(SK_ColorBLACK);
   gfx::RectF fill_bounds(GetLocalBounds());
+  fill_bounds.Inset(gfx::InsetsF(IsHorizontal() ? kThumbHoverOffset : 0,
+                                 IsHorizontal() ? 0 : kThumbHoverOffset, 0, 0));
   fill_bounds.Inset(gfx::InsetsF(kThumbStroke, kThumbStroke,
                                  IsHorizontal() ? 0 : kThumbStroke,
                                  IsHorizontal() ? kThumbStroke : 0));
   canvas->DrawRect(fill_bounds, fill_paint);
 
   SkPaint stroke_paint;
   stroke_paint.setStyle(SkPaint::kStroke_Style);
   stroke_paint.setColor(SK_ColorWHITE);
   stroke_paint.setStrokeWidth(kThumbStroke);
   gfx::RectF stroke_bounds(fill_bounds);
@@ skipping 17 matching lines @@
   scroll_bar_->Show();
   // Don't start the hide countdown if the thumb is still hovered or pressed.
   if (GetState() == CustomButton::STATE_NORMAL)
     scroll_bar_->StartHideCountdown();
 }
 
 void OverlayScrollBar::Thumb::OnStateChanged() {
   if (GetState() == CustomButton::STATE_NORMAL) {
     gfx::Transform translation;
     translation.Translate(
-        gfx::Vector2d(IsHorizontal() ? 0 : kThumbUnhoveredDifference,
-                      IsHorizontal() ? kThumbUnhoveredDifference : 0));
+        gfx::Vector2d(IsHorizontal() ? 0 : kThumbHoverOffset,
+                      IsHorizontal() ? kThumbHoverOffset: 0));
     layer()->SetTransform(translation);
     layer()->SetOpacity(kThumbDefaultAlpha);
 
     if (GetWidget())
       scroll_bar_->StartHideCountdown();
   } else {
     layer()->SetTransform(gfx::Transform());
     layer()->SetOpacity(kThumbHoverAlpha);
   }
 }
 
 OverlayScrollBar::OverlayScrollBar(bool horizontal)
     : BaseScrollBar(horizontal, new Thumb(this)), hide_timer_(false, false) {
+  static_cast<Thumb*>(GetThumb())->Init();

[varkha, 2016/11/19 02:58:00]

Is it possible to make a virtual BaseScrollBarThumb::Init, call it in
BaseScrollBar ctor and override Init in OverlayScrollBar::Thumb? Not sure if
this would be strictly simpler but should eliminate the need for cast.

[Evan Stade, 2016/11/21 15:26:20]

"Avoid virtual method calls in constructors"

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

[varkha, 2016/11/21 17:16:46]

Fair enough, although I thought this was about calling Class::VirtualMethod()
from Class::Class(), not from SomeOtherClass::SomeOtherClass(). In this case the
Thumb would be fully constructed so there should be no harm.

   set_notify_enter_exit_on_child(true);
   SetPaintToLayer(true);
   layer()->SetMasksToBounds(true);
   layer()->SetFillsBoundsOpaquely(false);
 }
 
-OverlayScrollBar::~OverlayScrollBar() {
-}
+OverlayScrollBar::~OverlayScrollBar() {}
 
 gfx::Rect OverlayScrollBar::GetTrackBounds() const {
-  return GetLocalBounds();
+  gfx::Rect local = GetLocalBounds();
+  // The track has to be wide enough for the thumb.
+  local.Inset(gfx::Insets(IsHorizontal() ? -kThumbHoverOffset : 0,
+                          IsHorizontal() ? 0 : -kThumbHoverOffset, 0, 0));
+  return local;
 }
 
 int OverlayScrollBar::GetLayoutSize() const {
   return 0;
 }
 
 int OverlayScrollBar::GetContentOverlapSize() const {
   return kThumbThickness;
 }
 
@@ skipping 31 matching lines @@
     thumb_bounds.set_x(thumb->x());
     thumb_bounds.set_width(thumb->width());
   } else {
     thumb_bounds.set_y(thumb->y());
     thumb_bounds.set_height(thumb->height());
   }
   thumb->SetBoundsRect(thumb_bounds);
 }
 
 }  // namespace views