Fix clipping issue on rounded corners for bubbles

Currently bubbles rely on having the NonClientFrameView apply a clip
mask to the ClientView so that the ClientView's contents are
correctly constrained to the bounds of the frame.

However, if the ClientView's content view hierarchy employs layer
backed views, the ClientView's clip mask is ignored if the ClientView
does not itself paint to a layer. This results in clipping issues
where views can clip outside the bounds of the bubble when corner
radius is increased.

This patch addresses this issue by having the BubbleDialogDelegate
supply a ClientView that paints to a textured layer. To address the
existing cases where bubbles rely on the frame's background color
(i.e. they assume that the ClientView is transparent), the
BubbleFrameView will update the background color to ensure existing
bubble behavior is preserved.

Currently these features are gated behind the
kEnableMDRoundedCornersOnDialogs feature flag.

Bug: 932970, 822075
Change-Id: I68542ffe84b533fd8b751acc01e5852a8b9bf6b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257452Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782101}

ash/shelf/contextual_nudge.cc

@@ -42,6 +42,9 @@ ContextualNudge::ContextualNudge(views::View* anchor,
                                       GetArrowForPosition(position),
                                       views::BubbleBorder::NO_ASSETS),
       tap_callback_(tap_callback) {
+  // Bubbles that use transparent colors should not paint their ClientViews to a
+  // layer as doing so could result in visual artifacts.
+  SetPaintClientToLayer(false);
   set_color(SK_ColorTRANSPARENT);
   set_close_on_deactivate(false);
   set_margins(gfx::Insets());

ash/shelf/shelf_bubble.cc

@@ -36,6 +36,9 @@ ShelfBubble::ShelfBubble(views::View* anchor,
           /* Don't pass the Shelf so the translucent color is always used. */
           nullptr,
           Shell::Get()->wallpaper_controller()) {
+  // Bubbles that use transparent colors should not paint their ClientViews to a
+  // layer as doing so could result in visual artifacts.
+  SetPaintClientToLayer(false);
   SetButtons(ui::DIALOG_BUTTON_NONE);
   background_animator_.Init(ShelfBackgroundType::kDefaultBg);
   background_animator_.AddObserver(this);

ash/system/tray/tray_bubble_view.cc

@@ -215,6 +215,9 @@ TrayBubbleView::TrayBubbleView(const InitParams& init_params)
       owned_bubble_border_(bubble_border_),
       is_gesture_dragging_(false),
       mouse_actively_entered_(false) {
+  // Bubbles that use transparent colors should not paint their ClientViews to a
+  // layer as doing so could result in visual artifacts.
+  SetPaintClientToLayer(false);
   SetButtons(ui::DIALOG_BUTTON_NONE);
   DCHECK(delegate_);
   DCHECK(params_.parent_window);

chrome/browser/ui/views/accessibility/caption_bubble.cc

@@ -164,6 +164,9 @@ CaptionBubble::CaptionBubble(views::View* anchor,
       ratio_in_parent_x_(kDefaultRatioInParentX),
       ratio_in_parent_y_(kDefaultRatioInParentY),
       browser_view_(browser_view) {
+  // Bubbles that use transparent colors should not paint their ClientViews to a
+  // layer as doing so could result in visual artifacts.
+  SetPaintClientToLayer(false);
   SetButtons(ui::DIALOG_BUTTON_NONE);
   set_draggable(true);
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));

ui/views/bubble/bubble_dialog_delegate_view.cc

@@ -41,6 +41,16 @@ bool BubbleDialogDelegate::devtools_dismiss_override_ = false;
 
 namespace {
 
+// A BubbleFrameView will apply a masking path to its ClientView to ensure
+// contents are appropriately clipped to the frame's rounded corners. If the
+// bubble uses layers in its views hierarchy, these will not be clipped to
+// the client mask unless the ClientView is backed by a textured ui::Layer.
+// This flag tracks whether or not to to create a layer backed ClientView.
+//
+// TODO(tluk): Fix all cases where bubble transparency is used and have bubble
+// ClientViews always paint to a layer.
+DEFINE_UI_CLASS_PROPERTY_KEY(bool, kPaintClientToLayer, true)
+
 // Override base functionality of Widget to give bubble dialogs access to the
 // theme provider of the window they're anchored to.
 class BubbleWidget : public Widget {
@@ -342,6 +352,20 @@ NonClientFrameView* BubbleDialogDelegate::CreateNonClientFrameView(
   return frame;
 }
 
+ClientView* BubbleDialogDelegate::CreateClientView(Widget* widget) {
+  client_view_ = DialogDelegate::CreateClientView(widget);
+  // In order for the |client_view|'s content view hierarchy to respect its clip
+  // mask we must paint to a layer. This is necessary because layers do not
+  // respect the clip of a non-layer backed parent.
+  if (base::FeatureList::IsEnabled(
+          features::kEnableMDRoundedCornersOnDialogs) &&
+      GetProperty(kPaintClientToLayer)) {
+    client_view_->SetPaintToLayer();
+  }
+
+  return client_view_;
+}
+
 bool BubbleDialogDelegateView::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   if (accelerator.key_code() == ui::VKEY_DOWN ||
@@ -487,6 +511,11 @@ ui::LayerType BubbleDialogDelegate::GetLayerType() const {
   return ui::LAYER_TEXTURED;
 }
 
+void BubbleDialogDelegate::SetPaintClientToLayer(bool paint_client_to_layer) {
+  DCHECK(!client_view_);
+  SetProperty(kPaintClientToLayer, paint_client_to_layer);
+}
+
 void BubbleDialogDelegate::UseCompactMargins() {
   set_margins(gfx::Insets(6));
 }

ui/views/bubble/bubble_dialog_delegate_view.h

@@ -13,6 +13,7 @@
 #include "build/build_config.h"
 #include "ui/accessibility/ax_enums.mojom-forward.h"
 #include "ui/base/accelerators/accelerator.h"
+#include "ui/base/class_property.h"
 #include "ui/views/bubble/bubble_border.h"
 #include "ui/views/view_tracker.h"
 #include "ui/views/widget/widget.h"
@@ -39,7 +40,8 @@ namespace views {
 
 class Button;
 
-class VIEWS_EXPORT BubbleDialogDelegate : public DialogDelegate {
+class VIEWS_EXPORT BubbleDialogDelegate : public DialogDelegate,
+                                          public ui::PropertyHandler {
  public:
   enum class CloseReason {
     DEACTIVATION,
@@ -58,6 +60,7 @@ class VIEWS_EXPORT BubbleDialogDelegate : public DialogDelegate {
   // DialogDelegate:
   BubbleDialogDelegate* AsBubbleDialogDelegate() override;
   NonClientFrameView* CreateNonClientFrameView(Widget* widget) override;
+  ClientView* CreateClientView(Widget* widget) override;
   ax::mojom::Role GetAccessibleWindowRole() override;
 
   //////////////////////////////////////////////////////////////////////////////
@@ -218,6 +221,9 @@ class VIEWS_EXPORT BubbleDialogDelegate : public DialogDelegate {
     title_margins_ = title_margins;
   }
 
+  // Sets whether or not CreateClientView() returns a layer backed ClientView.
+  void SetPaintClientToLayer(bool paint_client_to_layer);
+
   // Sets the content margins to a default picked for smaller bubbles.
   void UseCompactMargins();
 
@@ -341,6 +347,9 @@ class VIEWS_EXPORT BubbleDialogDelegate : public DialogDelegate {
   bool accept_events_ = true;
   gfx::NativeView parent_window_ = nullptr;
 
+  // Pointer to this bubble's ClientView.
+  ClientView* client_view_ = nullptr;
+
 #if defined(OS_MACOSX)
   // Special handler for close_on_deactivate() on Mac. Window (de)activation is
   // suppressed by the WindowServer when clicking rapidly, so the bubble must

ui/views/bubble/bubble_dialog_delegate_view_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "ui/base/hit_test.h"
 #include "ui/events/event_utils.h"
@@ -27,6 +28,7 @@
 #include "ui/views/test/test_widget_observer.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/test/widget_test.h"
+#include "ui/views/views_features.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -601,6 +603,50 @@ TEST_F(BubbleDialogDelegateViewTest, GetThemeProvider_FromAnchorWidget) {
             anchor_widget->GetThemeProvider());
 }
 
+// Tests whether the BubbleDialogDelegateView will create a layer backed client
+// view when prompted to do so.
+class BubbleDialogDelegateClientLayerTest : public test::WidgetTest {
+ public:
+  BubbleDialogDelegateClientLayerTest() = default;
+  ~BubbleDialogDelegateClientLayerTest() override = default;
+
+  void SetUp() override {
+    WidgetTest::SetUp();
+    scoped_feature_list_.InitWithFeatures(
+        {features::kEnableMDRoundedCornersOnDialogs}, {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(BubbleDialogDelegateClientLayerTest, WithClientLayerTest) {
+  std::unique_ptr<Widget> anchor_widget =
+      CreateTestWidget(Widget::InitParams::TYPE_WINDOW);
+  auto bubble_delegate = std::make_unique<BubbleDialogDelegateView>(
+      nullptr, BubbleBorder::TOP_LEFT);
+  bubble_delegate->set_parent_window(anchor_widget->GetNativeView());
+
+  WidgetAutoclosePtr bubble_widget(
+      BubbleDialogDelegateView::CreateBubble(bubble_delegate.release()));
+
+  EXPECT_NE(nullptr, bubble_widget->client_view()->layer());
+}
+
+TEST_F(BubbleDialogDelegateClientLayerTest, WithoutClientLayerTest) {
+  std::unique_ptr<Widget> anchor_widget =
+      CreateTestWidget(Widget::InitParams::TYPE_WINDOW);
+  auto bubble_delegate = std::make_unique<BubbleDialogDelegateView>(
+      nullptr, BubbleBorder::TOP_LEFT);
+  bubble_delegate->SetPaintClientToLayer(false);
+  bubble_delegate->set_parent_window(anchor_widget->GetNativeView());
+
+  WidgetAutoclosePtr bubble_widget(
+      BubbleDialogDelegateView::CreateBubble(bubble_delegate.release()));
+
+  EXPECT_EQ(nullptr, bubble_widget->client_view()->layer());
+}
+
 // Anchoring Tests -------------------------------------------------------------
 
 namespace {

ui/views/bubble/bubble_frame_view.cc

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/feature_list.h"
 #include "build/build_config.h"
 #include "components/vector_icons/vector_icons.h"
 #include "third_party/skia/include/core/SkPath.h"
@@ -34,6 +35,7 @@
 #include "ui/views/paint_info.h"
 #include "ui/views/resources/grit/views_resources.h"
 #include "ui/views/view_class_properties.h"
+#include "ui/views/views_features.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 #include "ui/views/window/client_view.h"
@@ -450,6 +452,7 @@ void BubbleFrameView::OnThemeChanged() {
   if (bubble_border_ && bubble_border_->use_theme_background_color()) {
     bubble_border_->set_background_color(GetNativeTheme()->GetSystemColor(
         ui::NativeTheme::kColorId_DialogBackground));
+    UpdateClientViewBackground();
     SchedulePaint();
   }
 }
@@ -554,6 +557,7 @@ void BubbleFrameView::SetArrow(BubbleBorder::Arrow arrow) {
 
 void BubbleFrameView::SetBackgroundColor(SkColor color) {
   bubble_border_->set_background_color(color);
+  UpdateClientViewBackground();
   SchedulePaint();
 }
 
@@ -561,6 +565,24 @@ SkColor BubbleFrameView::GetBackgroundColor() const {
   return bubble_border_->background_color();
 }
 
+void BubbleFrameView::UpdateClientViewBackground() {
+  if (!base::FeatureList::IsEnabled(features::kEnableMDRoundedCornersOnDialogs))
+    return;
+  DCHECK(GetWidget());
+  DCHECK(GetWidget()->client_view());
+
+  // If dealing with a layer backed ClientView we need to update it's color to
+  // match that of the frame view.
+  View* client_view = GetWidget()->client_view();
+  if (client_view->layer()) {
+    // If the ClientView's background is transparent this could result in visual
+    // artifacts. Make sure this isn't the case.
+    DCHECK_EQ(SK_AlphaOPAQUE, SkColorGetA(GetBackgroundColor()));
+    client_view->SetBackground(CreateSolidBackground(GetBackgroundColor()));
+    client_view->SchedulePaint();
+  }
+}
+
 gfx::Rect BubbleFrameView::GetUpdatedWindowBounds(
     const gfx::Rect& anchor_rect,
     const BubbleBorder::Arrow delegate_arrow,

ui/views/bubble/bubble_frame_view.h

@@ -140,6 +140,12 @@ class VIEWS_EXPORT BubbleFrameView : public NonClientFrameView,
   void SetBackgroundColor(SkColor color);
   SkColor GetBackgroundColor() const;
 
+  // For masking reasons, the ClientView may be painted to a textured layer. To
+  // ensure bubbles that rely on the frame background color continue to work as
+  // expected, we must set the background of the ClientView to match that of the
+  // BubbleFrameView.
+  void UpdateClientViewBackground();
+
   // Given the size of the contents and the rect to point at, returns the bounds
   // of the bubble window. The bubble's arrow location may change if the bubble
   // does not fit on the monitor or anchor window (if one exists) and