MacViews: Overlay Scrollbars with Show/Hide Animations

ui/views/controls/scroll_view_unittest.cc

Index: ui/views/controls/scroll_view_unittest.cc
diff --git a/ui/views/controls/scroll_view_unittest.cc b/ui/views/controls/scroll_view_unittest.cc
index 180560ef467d7db8d3858bdc3e8f7f53bd3a48ae..300cb4dd41b604192445b91fdbedc8a21c484949 100644
--- a/ui/views/controls/scroll_view_unittest.cc
+++ b/ui/views/controls/scroll_view_unittest.cc
@@ -10,6 +10,10 @@
 #include "ui/views/controls/scrollbar/overlay_scroll_bar.h"
 #include "ui/views/test/test_views.h"
 
+#if defined(OS_MACOSX)
+#include "ui/base/test/scoped_preferred_scroller_style_mac.h"
+#endif
+
 namespace views {
 
 namespace {
@@ -18,6 +22,8 @@ const int kWidth = 100;
 const int kMinHeight = 50;
 const int kMaxHeight = 100;
 
+enum ScrollBarOrientation { HORIZONTAL, VERTICAL };

[tapted, 2016/02/19 06:56:37]

`enum` -> `enum class`? (otherwise, I think HORIZONTAL/VERTICAL are accessible
without the ScrollBarOrientation:: prefix

[spqchan, 2016/02/19 19:35:32]

True, I removed the prefix. Is it okay if I stick with enum then? Or should I
use enum class?

+
 // View implementation that allows setting the preferred size.
 class CustomView : public View {
  public:
@@ -47,6 +53,20 @@ class CustomView : public View {
   DISALLOW_COPY_AND_ASSIGN(CustomView);
 };
 
+void CheckScrollbarVisibility(ScrollView& scroll_view,

[tapted, 2016/02/19 06:56:37]

does const-reference work? (otherwise ScrollView* -> we aren't allowed non-const
ref arguments in Chromium)

[spqchan, 2016/02/19 19:35:32]

Done.

+                              ScrollBarOrientation orientation,
+                              bool is_visible) {

[tapted, 2016/02/19 06:56:37]

maybe is_visible -> should_be_visible?

[spqchan, 2016/02/19 19:35:32]

Done.

+  const ScrollBar* scrollbar = orientation == ScrollBarOrientation::HORIZONTAL
+                                   ? scroll_view.horizontal_scroll_bar()
+                                   : scroll_view.vertical_scroll_bar();
+  if (is_visible) {
+    ASSERT_TRUE(scrollbar != NULL);

[tapted, 2016/02/19 06:56:37]

typically I just see ASSERT_TRUE(scrollbar); for non-null checks. (otherwise
NULL -> nullptr)

[spqchan, 2016/02/19 19:35:32]

Done.

+    EXPECT_TRUE(scrollbar->visible());
+  } else {
+    EXPECT_TRUE(!scrollbar || !scrollbar->visible());
+  }
+}
+
 }  // namespace
 
 // Verifies the viewport is sized to fit the available space.
@@ -60,7 +80,12 @@ TEST(ScrollViewTest, ViewportSizedToFit) {
 }
 
 // Verifies the scrollbars are added as necessary.
+// If on Mac, test the legacy scrollbars.

[tapted, 2016/02/19 06:56:37]

nit: legacy -> non-overlay style

[spqchan, 2016/02/19 19:35:32]

Done.

 TEST(ScrollViewTest, ScrollBars) {
+#if defined(OS_MACOSX)
+  ui::test::ScopedPreferredScrollerStyle scroller_style_override(false);
+#endif
+
   ScrollView scroll_view;
   View* contents = new View;
   scroll_view.SetContents(contents);
@@ -449,4 +474,46 @@ TEST(ScrollViewTest, CornerViewVisibility) {
   EXPECT_TRUE(corner_view->visible());
 }
 
+#if defined(OS_MACOSX)
+// Tests the overlay scrollbars on Mac. Ensure that they show up properly and
+// do not overlap each other.
+TEST(ScrollViewTest, CocoaOverlayScrollBars) {
+  ui::test::ScopedPreferredScrollerStyle scroller_style_override(true);
+  ScrollView scroll_view;
+  View* contents = new View;
+  scroll_view.SetContents(contents);
+  scroll_view.SetBoundsRect(gfx::Rect(0, 0, 100, 100));
+
+  // Size the contents such that vertical scrollbar is needed.
+  contents->SetBounds(0, 0, 50, 400);
+  scroll_view.Layout();
+  EXPECT_EQ(100 - scroll_view.GetScrollBarWidth(), contents->parent()->width());

[tapted, 2016/02/19 06:56:37]

should scroll_view.GetScrollBarWidth() be zero for this case? Maybe a separate
check for that, and just do

// Since it is overlaid, the ViewPort size should match the ScrollView.
EXPECT_EQ(100, contents->parent()->width());


same for the both-scrollbars cases

[spqchan, 2016/02/19 19:35:32]

Done.

+  EXPECT_EQ(100, contents->parent()->height());
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::VERTICAL, true);
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::HORIZONTAL,
+                           false);
+
+  // Size the contents such that horizontal scrollbar is needed.
+  contents->SetBounds(0, 0, 400, 50);
+  scroll_view.Layout();
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::VERTICAL, false);
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::HORIZONTAL, true);
+
+  // Both horizontal and vertical scrollbars.
+  contents->SetBounds(0, 0, 300, 400);
+  scroll_view.Layout();
+  EXPECT_EQ(100 - scroll_view.GetScrollBarWidth(), contents->parent()->width());
+  EXPECT_EQ(100 - scroll_view.GetScrollBarHeight(),
+            contents->parent()->height());
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::VERTICAL, true);
+  CheckScrollbarVisibility(scroll_view, ScrollBarOrientation::HORIZONTAL, true);
+
+  // Make sure the horizontal and vertical scrollbars don't overlap each other.
+  gfx::Rect vert_bounds = scroll_view.vertical_scroll_bar()->bounds();
+  gfx::Rect horiz_bounds = scroll_view.horizontal_scroll_bar()->bounds();
+  EXPECT_EQ(vert_bounds.x(), horiz_bounds.right());
+  EXPECT_EQ(horiz_bounds.y(), vert_bounds.bottom());
+}

[tapted, 2016/02/19 06:56:37]

I think a good test would be to (here) make scroller_style_override a scoped_ptr
and do 

scroller_style_override.reset();
scroller_style_override.reset(new ScopedPreferred..(false));

then do some spot checks to ensure the contents->parent() (the ViewPort?) has
resized to be smaller, and the ScrollBarWidth is non-zero (both without
requiring a separate call to scroll_view.Layout())

[spqchan, 2016/02/19 19:35:32]

Done.

+#endif
+
 }  // namespace views