MacViews: Overlay Scrollbars with Show/Hide Animations

ui/views/controls/scroll_view_unittest.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/views/controls/scroll_view.h"
 
 #include "base/macros.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/border.h"
 #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 {
 
 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:
   CustomView() {}
 
   void SetPreferredSize(const gfx::Size& size) {
     preferred_size_ = size;
     PreferredSizeChanged();
   }
 
   gfx::Size GetPreferredSize() const override { return preferred_size_; }
 
   void Layout() override {
     gfx::Size pref = GetPreferredSize();
     int width = pref.width();
     int height = pref.height();
     if (parent()) {
       width = std::max(parent()->width(), width);
       height = std::max(parent()->height(), height);
     }
     SetBounds(x(), y(), width, height);
   }
 
  private:
   gfx::Size preferred_size_;
 
   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.
 TEST(ScrollViewTest, ViewportSizedToFit) {
   ScrollView scroll_view;
   View* contents = new View;
   scroll_view.SetContents(contents);
   scroll_view.SetBoundsRect(gfx::Rect(0, 0, 100, 100));
   scroll_view.Layout();
   EXPECT_EQ("0,0 100x100", contents->parent()->bounds().ToString());
 }
 
 // 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);
   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());
   EXPECT_EQ(100, contents->parent()->height());
@@ skipping 368 matching lines @@
   scroll_view.Layout();
   EXPECT_FALSE(corner_view->parent());
 
   // Corner view should reappear when both scrollbars reappear.
   contents->SetBounds(0, 0, 200, 200);
   scroll_view.Layout();
   EXPECT_EQ(&scroll_view, corner_view->parent());
   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