Ensure opaque background for subpixel rendering

This turns a previously broken DLOG into a DCHECK that triggers if a
subpixel-rendered label is painted on top of a non-opaque layer without
using an opaque background for any of its parent views.

This DCHECK is not yet turned on for ChromeOS as tests are failing.

Bug: 1139395
Change-Id: I2422a9e66220780b1f2a105ac9c9320fa9a3aae7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2488066
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820439}

ui/views/controls/label.cc

@@ -652,20 +652,33 @@ void Label::PaintText(gfx::Canvas* canvas) {
   if (display_text_)
     display_text_->Draw(canvas);
 
-#if DCHECK_IS_ON()
-  // Attempt to ensure that if we're using subpixel rendering, we're painting
-  // to an opaque background. What we don't want to find is an ancestor in the
-  // hierarchy that paints to a non-opaque layer.
+#if DCHECK_IS_ON() && !defined(OS_CHROMEOS)
+  // TODO(crbug.com/1139395): Enable this DCHECK on ChromeOS by fixing either
+  // this check (to correctly idenfify more paints-on-opaque cases), refactoring
+  // parents to use background() or by fixing subpixel-rendering issues that the
+  // DCHECK detects.
   if (!display_text_ || display_text_->subpixel_rendering_suppressed())
     return;
 
+  // Ensure that, if we're using subpixel rendering, we're painted to an opaque
+  // region. Subpixel rendering will sample from the r,g,b color channels of the
+  // canvas. These values are incorrect when sampling from transparent pixels.
+  // Note that these checks may need to be amended for other methods of painting
+  // opaquely underneath the Label or we might need to allow individual cases to
+  // skip this DCHECK.
   for (View* view = this; view; view = view->parent()) {
+    // This is our approximation of being painted on an opaque region. If any
+    // parent has an opaque background we assume that that background covers the
+    // text bounds. This is not necessarily true as the background could be
+    // inset from the parent bounds, and get_color() does not imply that all of
+    // the background is painted with the same opaque color.
     if (view->background() && IsOpaque(view->background()->get_color()))
       break;
 
-    if (view->layer() && view->layer()->fills_bounds_opaquely()) {
-      DLOG(WARNING) << "Ancestor view has a non-opaque layer: "
-                    << view->GetClassName() << " with ID " << view->GetID();
+    if (view->layer()) {
+      // If we aren't painted to an opaque background, we must paint to an
+      // opaque layer.
+      DCHECK(view->layer()->fills_bounds_opaquely());
       break;
     }
   }

ui/views/controls/label.h

@@ -314,6 +314,7 @@ class VIEWS_EXPORT Label : public View,
   FRIEND_TEST_ALL_PREFIXES(LabelTest, FocusBounds);
   FRIEND_TEST_ALL_PREFIXES(LabelTest, MultiLineSizingWithElide);
   FRIEND_TEST_ALL_PREFIXES(LabelTest, IsDisplayTextTruncated);
+  FRIEND_TEST_ALL_PREFIXES(LabelTest, ChecksSubpixelRenderingOntoOpaqueSurface);
   friend class LabelSelectionTest;
 
   // ContextMenuController overrides:

ui/views/controls/label_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/gtest_util.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/accessibility/ax_enums.mojom.h"
@@ -29,6 +30,7 @@
 #include "ui/gfx/text_constants.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/strings/grit/ui_strings.h"
+#include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/base_control_test_widget.h"
 #include "ui/views/controls/link.h"
@@ -1060,6 +1062,36 @@ TEST_F(LabelTest, TextChangedCallback) {
   EXPECT_TRUE(text_changed);
 }
 
+// TODO(crbug.com/1139395): Enable on ChromeOS along with the DCHECK in Label.
+#if !defined(OS_CHROMEOS)
+// Ensures DCHECK for subpixel rendering on transparent layer is working.
+TEST_F(LabelTest, ChecksSubpixelRenderingOntoOpaqueSurface) {
+  View view;
+  Label* label = view.AddChildView(std::make_unique<TestLabel>());
+  EXPECT_TRUE(label->GetSubpixelRenderingEnabled());
+
+  gfx::Canvas canvas;
+
+  // Painting on a view not painted to a layer should be fine.
+  label->OnPaint(&canvas);
+
+  // Painting to an opaque layer should also be fine.
+  view.SetPaintToLayer();
+  label->OnPaint(&canvas);
+
+  // Set up a transparent layer for the parent view.
+  view.layer()->SetFillsBoundsOpaquely(false);
+
+  // Painting on a transparent layer should DCHECK.
+  EXPECT_DCHECK_DEATH(label->OnPaint(&canvas));
+
+  // Painting onto a transparent layer should not DCHECK if there's an opaque
+  // background in a parent of the Label.
+  view.SetBackground(CreateSolidBackground(SK_ColorWHITE));
+  label->OnPaint(&canvas);
+}
+#endif  // !defined(OS_CHROMEOS)
+
 TEST_F(LabelSelectionTest, Selectable) {
   // By default, labels don't support text selection.
   EXPECT_FALSE(label()->GetSelectable());