Change Aura overlay scrollbars from solid color to painted scrollbars.

ui/native_theme/native_theme_aura.cc

Index: ui/native_theme/native_theme_aura.cc
diff --git a/ui/native_theme/native_theme_aura.cc b/ui/native_theme/native_theme_aura.cc
index b6f47d402f4dc490dfe68bef799182ec07f9ac7e..6afcbb25bcd330d34559247033c324c832650dd3 100644
--- a/ui/native_theme/native_theme_aura.cc
+++ b/ui/native_theme/native_theme_aura.cc
@@ -20,6 +20,7 @@
 #include "ui/gfx/path.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/native_theme/common_theme.h"
+#include "ui/native_theme/material_design_constants.h"
 #include "ui/native_theme/native_theme_switches.h"
 
 namespace ui {
@@ -32,11 +33,11 @@ SkAlpha ThumbAlphaForState(NativeTheme::State state) {
     case NativeTheme::kDisabled:
       return 0x00;
     case NativeTheme::kHovered:
-      return overlay ? 0xB2 : 0x4D;
+      return overlay ? kMDScrollbarAlphaHovered : 0x4D;
     case NativeTheme::kNormal:
-      return overlay ? 0x8C : 0x33;
+      return overlay ? kMDScrollbarAlphaNormal : 0x33;
     case NativeTheme::kPressed:
-      return overlay ? 0xB2 : 0x80;
+      return overlay ? kMDScrollbarAlphaPressed : 0x80;
     case NativeTheme::kNumStates:
       break;
   }
@@ -51,10 +52,11 @@ SkAlpha ThumbStrokeAlphaForState(NativeTheme::State state) {
     case NativeTheme::kDisabled:
       return 0x00;
     case NativeTheme::kHovered:
+      return kMDScrollbarAlphaHovered;
     case NativeTheme::kPressed:
-      return 0x33;
+      return kMDScrollbarAlphaPressed;
     case NativeTheme::kNormal:
-      return 0x26;
+      return kMDScrollbarAlphaNormal;
     case NativeTheme::kNumStates:
       break;
   }
@@ -84,6 +86,11 @@ NativeThemeAura::NativeThemeAura() {
   set_scrollbar_button_length(0);
 #endif
 
+  if (IsOverlayScrollbarEnabled()) {
+    scrollbar_width_ =
+        kMDScrollbarThumbWidthPressed + kMDScrollbarStrokeWidth * 2;
+  }
+
   // Images and alphas declarations assume the following order.
   static_assert(kDisabled == 0, "states unexpectedly changed");
   static_assert(kHovered == 1, "states unexpectedly changed");
@@ -197,19 +204,28 @@ void NativeThemeAura::PaintScrollbarThumbStateTransition(
     double progress,
     const gfx::Rect& rect) const {
   gfx::Rect thumb_rect(rect);
+  SkColor thumb_color;
   if (IsOverlayScrollbarEnabled()) {
     // In overlay mode, draw a stroke (border).
-    const int kStrokeWidth = 1;
+    const int kStrokeWidth = kMDScrollbarStrokeWidth;

[Peter Kasting, 2016/10/17 19:10:25]

Nit: Can be constexpr

[bokan, 2016/10/17 23:21:03]

Done.

     SkAlpha stroke_alpha = gfx::Tween::IntValueBetween(
         progress, ThumbStrokeAlphaForState(start_state),
         ThumbStrokeAlphaForState(end_state));
     SkPaint paint;
-    paint.setColor(SkColorSetA(SK_ColorWHITE, stroke_alpha));
+    paint.setColor(SkColorSetA(kMDScrollbarStrokeColor, stroke_alpha));
     paint.setStyle(SkPaint::kStroke_Style);
     paint.setStrokeWidth(kStrokeWidth);
+
+    // When stroking in skia, there's some surprising behavior so we need to
+    // inset the right and bottom edges first. See the discussion here:
+    // https://groups.google.com/forum/#!topic/skia-discuss/HGZcc3Z7w1Q

[Peter Kasting, 2016/10/17 19:10:25]

I'm concerned about this code.

Looking at the referenced discussion, I think the issue is with people trying to
draw on pixel-edge coordinates instead of stroking on pixel centers.  If, for
example, you want to stroke with width kStrokeWidth, and you want the stroke's
outer edge to be flush to the inside of |thumb_rect|, then the right thing to do
is basically:

  gfx::RectF stroke_rect(thumb_rect);
  constexpr float kStrokeHalfWidth = kStrokeWidth / 2.f;
  stroke_rect.Inset(kStrokeHalfWidth, kStrokeHalfWidth);
  canvas->drawRect(gfx::RectFToSkRect(stroke_rect)), paint;

This ensures you'll do the correct thing no matter the stroke width, whereas the
inset you've written basically only works if the stroke rect is exactly 1 (and
then probably only for scale factor 1).

[bokan, 2016/10/17 23:21:03]

You're right, I misunderstood the discussion in the link (and Skia stroking more
generally). Thanks!

+    thumb_rect.Inset(0, 0, kStrokeWidth, kStrokeWidth);
     canvas->drawIRect(gfx::RectToSkIRect(thumb_rect), paint);
 
-    thumb_rect.Inset(kStrokeWidth, kStrokeWidth, kStrokeWidth, kStrokeWidth);
+    // Inset the top and left edges for filling in the stroke below.
+    thumb_rect.Inset(kStrokeWidth, kStrokeWidth, 0, 0);
+
+    thumb_color = kMDScrollbarThumbColor;
   } else {
     // If there are no scrollbuttons then provide some padding so that the thumb
     // doesn't touch the top of the track.
@@ -220,12 +236,14 @@ void NativeThemeAura::PaintScrollbarThumbStateTransition(
       thumb_rect.Inset(kThumbPadding, extra_padding);
     else
       thumb_rect.Inset(extra_padding, kThumbPadding);
+
+    thumb_color = SK_ColorBLACK;
   }
 
   SkPaint paint;
   SkAlpha alpha = gfx::Tween::IntValueBetween(
       progress, ThumbAlphaForState(start_state), ThumbAlphaForState(end_state));
-  paint.setColor(SkColorSetA(SK_ColorBLACK, alpha));
+  paint.setColor(SkColorSetA(thumb_color, alpha));
   canvas->drawIRect(gfx::RectToSkIRect(thumb_rect), paint);
 }
 
@@ -239,4 +257,36 @@ void NativeThemeAura::PaintScrollbarCorner(SkCanvas* canvas,
   canvas->drawIRect(RectToSkIRect(rect), paint);
 }
 
+gfx::Size NativeThemeAura::GetPartSize(Part part,
+                                       State state,
+                                       const ExtraParams& extra) const {
+  if (!IsOverlayScrollbarEnabled())

[Peter Kasting, 2016/10/17 19:10:24]

Nit: This conditional effectively results in duplicated code for the "fall back
to parent" case.  It feels slightly less error-prone to just do:

  if (IsOverlayScrollbarEnabled()) {
    ... handle scrollbar constants ...
  }

  return NativeThemeBase::GetPartSize(part, state, extra);

[bokan, 2016/10/17 23:21:04]

Done.

+      return NativeThemeBase::GetPartSize(part, state, extra);

[Peter Kasting, 2016/10/17 19:10:24]

Nit: 2 spaces

[bokan, 2016/10/17 23:21:03]

Done.

+
+  // Aura overlay scrollbars need a slight tweak from the base sizes.
+  switch (part) {
+    case kScrollbarDownArrow:
+    case kScrollbarUpArrow:
+    case kScrollbarLeftArrow:
+    case kScrollbarRightArrow:
+    case kScrollbarHorizontalTrack:
+    case kScrollbarVerticalTrack:
+    case kScrollbarHorizontalGripper:
+    case kScrollbarVerticalGripper:
+      // Aura overlay scrollbars only have a thumb.
+      NOTREACHED();

[Peter Kasting, 2016/10/17 19:10:25]

Is NOTREACHED really correct here, or should we just return 0?  Basically, this
implies that calling code needs to do a runtime check for overlay scrollbars
before calling to get the size of these parts.  I can't decide whether that's a
good thing we want to enforce, or unnecessarily forces the caller to worry about
this.

[bokan, 2016/10/17 23:21:04]

I leaned towards forcing a runtime failure since I think asking for the
dimensions of one of these implies the calling code is doing something
incorrectly. This seems to be compatible with how the code works today. Also, we
already have a DCHECK for IsOverlayScrollbarEnabled() in the function above.

[bokan, 2016/10/18 21:39:48]

Ok, I found a case where this did get tripped. When we create a message box that
creates a Views::ScrollView which asks for some of these dimensions, looks to me
like Views code isn't aware of layout scrollbars yet. I've removed this
NOTREACHED and just let the base method handle this so today's behavior remains
unchanged. I've added a TODO and will address this when I look at Views
specifically.

+      break;
+    case kScrollbarHorizontalThumb:
+      return gfx::Size(kMDScrollbarMinimumLength + 2 * kMDScrollbarStrokeWidth,

[Peter Kasting, 2016/10/17 19:10:24]

Nit: You can factor this out to a constant above the switch and use it for both
these cases.

[bokan, 2016/10/17 23:21:04]

Done.

+                       scrollbar_width_);
+    case kScrollbarVerticalThumb:
+      return gfx::Size(scrollbar_width_,
+                       kMDScrollbarMinimumLength + 2 * kMDScrollbarStrokeWidth);
+    default:
+      break;
+  }
+
+  return NativeThemeBase::GetPartSize(part, state, extra);
+}
+
 }  // namespace ui