Fix holding space scrollview clipping.

The pinned files section of holding space needs to paint to a layer to
avoid clipping the focus rings for holding space item views. That said,
it still needs to impose a clipping rect to avoid scrollview contents
being displayed outside of the scrollview viewport.

Bug: 1152948
Change-Id: Ia14ff0df7dc36650d573d1f9d79f73360c08178b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589043Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#836931}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -29,6 +29,7 @@ constexpr int kHoldingSpaceContextMenuMargin = 8;
 constexpr int kHoldingSpaceCornerRadius = 8;
 constexpr int kHoldingSpaceDownloadsChevronIconSize = 20;
 constexpr int kHoldingSpaceDownloadsHeaderSpacing = 16;
+constexpr int kHoldingSpaceFocusInsets = -2;
 constexpr int kHoldingSpaceIconSize = 20;
 constexpr int kHoldingSpaceRowSpacing = 8;
 constexpr gfx::Insets kHoldingSpaceScreenCapturePadding(8);

ash/system/holding_space/holding_space_item_view.cc

@@ -211,8 +211,8 @@ void HoldingSpaceItemView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
   InvalidateLayer(selected_layer_owner_->layer());
 
   // The focus ring is painted just outside the bounds for this view.
-  const float kFocusInsets =
-      -2.f - (views::PlatformStyle::kFocusHaloThickness / 2.f);
+  const float kFocusInsets = kHoldingSpaceFocusInsets -
+                             (views::PlatformStyle::kFocusHaloThickness / 2.f);
 
   bounds.Inset(gfx::Insets(kFocusInsets));
   focused_layer_owner_->layer()->SetBounds(bounds);

ash/system/holding_space/holding_space_item_views_section.cc

@@ -10,6 +10,7 @@
 #include "ui/compositor/callback_layer_animation_observer.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/style/platform_style.h"
 
 namespace ash {
 
@@ -24,6 +25,14 @@ constexpr bool kDeleteObserver = true;
 class HoldingSpaceScrollView : public views::ScrollView,
                                public views::ViewObserver {
  public:
+  HoldingSpaceScrollView() {
+    // `HoldingSpaceItemView`s draw a focus ring outside of their view bounds.
+    // `HoldingSpaceScrollView` needs to paint to a layer to avoid clipping
+    // these focus rings.
+    SetPaintToLayer();
+    layer()->SetFillsBoundsOpaquely(false);
+  }
+
   views::View* SetContents(std::unique_ptr<views::View> view) {
     views::View* contents = views::ScrollView::SetContents(std::move(view));
     view_observer_.Observe(contents);
@@ -31,6 +40,21 @@ class HoldingSpaceScrollView : public views::ScrollView,
   }
 
  private:
+  // views::ScrollView:
+  void OnBoundsChanged(const gfx::Rect& previous_bounds) override {
+    // The focus ring for `HoldingSpaceItemView`s is painted just outside of
+    // their view bounds. The clip rect for this view should be expanded to
+    // avoid clipping of these focus rings. Note that a clip rect *does* need to
+    // be applied to prevent this view from painting its contents outside of its
+    // viewport.
+    const float kFocusInsets =
+        kHoldingSpaceFocusInsets -
+        (views::PlatformStyle::kFocusHaloThickness / 2.f);
+    gfx::Rect bounds = GetLocalBounds();
+    bounds.Inset(gfx::Insets(kFocusInsets));
+    layer()->SetClipRect(bounds);
+  }
+
   // views::ViewObserver:
   void OnViewPreferredSizeChanged(View* observed_view) override {
     PreferredSizeChanged();
@@ -91,8 +115,6 @@ void HoldingSpaceItemViewsSection::Init() {
     scroll->SetBackgroundColor(base::nullopt);
     scroll->ClipHeightTo(0, INT_MAX);
     scroll->SetDrawOverflowIndicator(false);
-    scroll->SetPaintToLayer();
-    scroll->layer()->SetFillsBoundsOpaquely(false);
     container_ = scroll->SetContents(CreateContainer());
     container_->SetVisible(false);
   }