Enhance separator logic to handle most use cases.

In https://chromium-review.googlesource.com/c/chromium/src/+/1880531, we
fixed the case where a separator would not properly fill its bounds.
However that broke functionality where a spacer was being used as e.g. a
vertical separator in a text flow with insets on either side (see
attached bug).

This solution does both:
 - when there is no inset on a particular side of the separator, the
   separator is guaranteed to draw flush with the edge of the view
 - when there is an inset, the inset is guaranteed to be preserved,
   with the old behavior of the separator being adjusted to draw only on
   whole pixels in DPI-scaled environments (minimum of 1px in either
   dimension)


Fixes: 1019503
Bug: 1019503
Change-Id: I97dd487ba2374000dcef76e43665ef44eb93acd8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1894253
Commit-Queue: Dana Fried <dfried@chromium.org>
Auto-Submit: Dana Fried <dfried@chromium.org>
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711842}

ui/views/controls/separator.cc

@@ -7,6 +7,7 @@
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/native_theme/native_theme.h"
 
 namespace views {
@@ -56,8 +57,40 @@ void Separator::OnPaint(gfx::Canvas* canvas) {
                             ? *overridden_color_
                             : GetNativeTheme()->GetSystemColor(
                                   ui::NativeTheme::kColorId_SeparatorColor);
-  canvas->DrawColor(color);
+  // Paint background and border, if any.
   View::OnPaint(canvas);
+
+  gfx::ScopedCanvas scoped_canvas(canvas);
+  const gfx::Insets insets = GetInsets();
+  const gfx::Rect contents_bounds = GetContentsBounds();
+  const float dsf = canvas->UndoDeviceScaleFactor();
+
+  // This is a hybrid of gfx::ScaleToEnclosedRect() and
+  // gfx::ScaleToEnclosingRect() based on whether there are nonzero insets on
+  // any particular side of the separator. If there is no inset, the separator
+  // will paint all the way out to the edge of the view. If there is an inset,
+  // the extent of the separator will rounded inward so that it paints only
+  // full pixels, providing a sharper appearance and preserving the inset.
+  //
+  // This allows separators that entirely fill their area to do so, and those
+  // intended as spacers in a larger flow to do so as well. See
+  // crbug.com/1016760 and crbug.com/1019503 for examples of why we need to
+  // handle both cases.
+  const int x = insets.left() == 0 ? std::floor(contents_bounds.x() * dsf)
+                                   : std::ceil(contents_bounds.x() * dsf);
+  const int y = insets.top() == 0 ? std::floor(contents_bounds.y() * dsf)
+                                  : std::ceil(contents_bounds.y() * dsf);
+  const int r = insets.right() == 0 ? std::ceil(contents_bounds.right() * dsf)
+                                    : std::floor(contents_bounds.right() * dsf);
+  const int b = insets.bottom() == 0
+                    ? std::ceil(contents_bounds.bottom() * dsf)
+                    : std::floor(contents_bounds.bottom() * dsf);
+
+  // Minimum separator size is 1 px.
+  const int w = std::max(1, r - x);
+  const int h = std::max(1, b - y);
+
+  canvas->FillRect({x, y, w, h}, color);
 }
 
 BEGIN_METADATA(Separator)

ui/views/controls/separator_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image_unittest_util.h"
+#include "ui/views/border.h"
 #include "ui/views/test/views_test_base.h"
 
 namespace views {
@@ -19,25 +20,36 @@ class SeparatorTest : public views::ViewsTestBase {
  protected:
   void ExpectDrawAtLeastOnePixel(float image_scale);
 
+  SkBitmap PaintToCanvas(float image_scale);
+
   Separator separator_;
 
+  static const SkColor kBackgroundColor;
+  static const SkColor kForegroundColor;
+  static const gfx::Size kTestImageSize;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(SeparatorTest);
 };
 
-void SeparatorTest::ExpectDrawAtLeastOnePixel(float image_scale) {
-  const gfx::Size kTestImageSize = gfx::Size(24, 24);
-  const SkColor kBackgroundColor = SK_ColorRED;
-  gfx::Canvas init(kTestImageSize, image_scale, true);
+const SkColor SeparatorTest::kBackgroundColor = SK_ColorRED;
+const SkColor SeparatorTest::kForegroundColor = SK_ColorGRAY;
+const gfx::Size SeparatorTest::kTestImageSize{24, 24};
+
+SkBitmap SeparatorTest::PaintToCanvas(float image_scale) {
   gfx::Canvas canvas(kTestImageSize, image_scale, true);
-  init.DrawColor(kBackgroundColor);
   canvas.DrawColor(kBackgroundColor);
-  ASSERT_TRUE(gfx::test::AreBitmapsEqual(canvas.GetBitmap(), init.GetBitmap()));
   separator_.OnPaint(&canvas);
+  return canvas.GetBitmap();
+}
+
+void SeparatorTest::ExpectDrawAtLeastOnePixel(float image_scale) {
+  SkBitmap painted = PaintToCanvas(image_scale);
+  gfx::Canvas unpainted(kTestImageSize, image_scale, true);
+  unpainted.DrawColor(kBackgroundColor);
 
   // At least 1 pixel should be changed.
-  EXPECT_FALSE(
-      gfx::test::AreBitmapsEqual(canvas.GetBitmap(), init.GetBitmap()));
+  EXPECT_FALSE(gfx::test::AreBitmapsEqual(painted, unpainted.GetBitmap()));
 }
 
 TEST_F(SeparatorTest, ImageScaleBelowOne) {
@@ -53,4 +65,235 @@ TEST_F(SeparatorTest, ImageScaleBelowOne_HorizontalLine) {
   ExpectDrawAtLeastOnePixel(0.4);
 }
 
+TEST_F(SeparatorTest, Paint_NoInsets_FillsCanvas_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 0));
+}
+
+TEST_F(SeparatorTest, Paint_NoInsets_FillsCanvas_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 0));
+}
+
+TEST_F(SeparatorTest, Paint_NoInsets_FillsCanvas_Scale150) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+
+  SkBitmap painted = PaintToCanvas(1.5f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 14));
+  EXPECT_EQ(kForegroundColor, painted.getColor(14, 14));
+  EXPECT_EQ(kForegroundColor, painted.getColor(14, 0));
+}
+
+TEST_F(SeparatorTest, Paint_TopInset_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(1, 0, 0, 0));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 1));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 1));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 9));
+}
+
+TEST_F(SeparatorTest, Paint_TopInset_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(1, 0, 0, 0));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 1));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(12, 1));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 2));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 2));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 12));
+}
+
+TEST_F(SeparatorTest, Paint_LeftInset_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 1, 0, 0));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(1, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(1, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 9));
+}
+
+TEST_F(SeparatorTest, Paint_LeftInset_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 1, 0, 0));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(1, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(1, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(2, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(2, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 12));
+}
+
+TEST_F(SeparatorTest, Paint_BottomInset_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 0, 1, 0));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 8));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 8));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 9));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 9));
+}
+
+TEST_F(SeparatorTest, Paint_BottomInset_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 0, 1, 0));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 10));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 10));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 11));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(12, 11));
+}
+
+TEST_F(SeparatorTest, Paint_RightInset_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 0, 0, 1));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(8, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(8, 9));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 9));
+}
+
+TEST_F(SeparatorTest, Paint_RightInset_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 0, 0, 1));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(10, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(10, 12));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(11, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(11, 12));
+}
+
+TEST_F(SeparatorTest, Paint_Vertical_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 4, 0, 5));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(3, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(3, 9));
+  EXPECT_EQ(kForegroundColor, painted.getColor(4, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(4, 9));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(5, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(5, 9));
+}
+
+TEST_F(SeparatorTest, Paint_Vertical_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(0, 4, 0, 5));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(4, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(4, 12));
+  EXPECT_EQ(kForegroundColor, painted.getColor(5, 0));
+  EXPECT_EQ(kForegroundColor, painted.getColor(5, 12));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(6, 0));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(6, 12));
+}
+
+TEST_F(SeparatorTest, Paint_Horizontal_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(4, 0, 5, 0));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 3));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 3));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 4));
+  EXPECT_EQ(kForegroundColor, painted.getColor(9, 4));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 5));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(9, 5));
+}
+
+TEST_F(SeparatorTest, Paint_Horizontal_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(4, 0, 5, 0));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 4));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(12, 4));
+  EXPECT_EQ(kForegroundColor, painted.getColor(0, 5));
+  EXPECT_EQ(kForegroundColor, painted.getColor(12, 5));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(0, 6));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(12, 6));
+}
+
+// Ensure that the separator is always at least 1px, even if insets would reduce
+// it to zero.
+TEST_F(SeparatorTest, Paint_MinimumSize_Scale100) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(5, 5, 5, 5));
+
+  SkBitmap painted = PaintToCanvas(1.0f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(5, 5));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(4, 5));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(5, 4));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(5, 6));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(6, 5));
+}
+
+// Ensure that the separator is always at least 1px, even if insets would reduce
+// it to zero (with scale factor > 1).
+TEST_F(SeparatorTest, Paint_MinimumSize_Scale125) {
+  separator_.SetSize({10, 10});
+  separator_.SetColor(kForegroundColor);
+  separator_.SetBorder(CreateEmptyBorder(5, 5, 5, 5));
+
+  SkBitmap painted = PaintToCanvas(1.25f);
+  EXPECT_EQ(kForegroundColor, painted.getColor(7, 7));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(6, 7));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(7, 6));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(7, 8));
+  EXPECT_EQ(kBackgroundColor, painted.getColor(8, 7));
+}
+
 }  // namespace views