Change Aura overlay scrollbars from solid color to painted scrollbars.

ui/native_theme/native_theme_aura.cc

 // Copyright (c) 2012 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/native_theme/native_theme_aura.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/logging.h"
 #include "build/build_config.h"
 #include "ui/base/layout.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/gfx/animation/tween.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
 #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 {
 
 namespace {
 
 SkAlpha ThumbAlphaForState(NativeTheme::State state) {
   bool overlay = IsOverlayScrollbarEnabled();
   switch (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;
   }
 
   NOTREACHED();
   return 0xFF;
 }
 
 SkAlpha ThumbStrokeAlphaForState(NativeTheme::State state) {

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

Do we even need this function anymore?

It looks like the implementation could just be:

  DCHECK(IsOverlayScrollbarEnabled());
  return ThumbAlphaForState(state);

But in that case, why not have the caller just call ThumbAlphaForState()
directly?

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

This function is now gone.

   DCHECK(IsOverlayScrollbarEnabled());
   switch (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;
   }
 
   NOTREACHED();
   return 0xFF;
 }
 
 const SkColor kTrackColor = SkColorSetRGB(0xF1, 0xF1, 0xF1);
 
 }  // namespace
 
 // static
 NativeTheme* NativeTheme::GetInstanceForWeb() {
   return NativeThemeAura::instance();
 }
 
 // static
 NativeThemeAura* NativeThemeAura::instance() {
   CR_DEFINE_STATIC_LOCAL(NativeThemeAura, s_native_theme, ());
   return &s_native_theme;
 }
 
 NativeThemeAura::NativeThemeAura() {
   // We don't draw scrollbar buttons.
 #if defined(OS_CHROMEOS)
   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");
   static_assert(kNormal == 2, "states unexpectedly changed");
   static_assert(kPressed == 3, "states unexpectedly changed");
 }
 
 NativeThemeAura::~NativeThemeAura() {
 }
 
@@ skipping 93 matching lines @@
 }
 
 void NativeThemeAura::PaintScrollbarThumbStateTransition(
     SkCanvas* canvas,
     Part part,
     State start_state,
     State end_state,
     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.
     const int kThumbPadding = 2;
     const int extra_padding =
         (scrollbar_button_length() == 0) ? kThumbPadding : 0;
     if (part == NativeTheme::kScrollbarVerticalThumb)
       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);
 }
 
 void NativeThemeAura::PaintScrollbarCorner(SkCanvas* canvas,
                                            State state,
                                            const gfx::Rect& rect) const {
   // Overlay Scrollbar should never paint a scrollbar corner.
   DCHECK(!IsOverlayScrollbarEnabled());
   SkPaint paint;
   paint.setColor(SkColorSetRGB(0xDC, 0xDC, 0xDC));
   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