Do not invert the colors of SVG shapes in dark mode.

Inline SVG images do not have an Image object associated
with them, so they do not go through the regular dark mode
image classification that non-inline SVG images would.

With the current design, the shapes belonging to the same
image might receive different classifications in dark mode.

So until there is a way to properly classify an inline
SVG image by considering all its individual shapes, do
not invert any part of that image.

Bug: 963998
Change-Id: Ib946c3c1ff7cdce08d2f5af9cca8542ccb9c91fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893111Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Prashant Nevase <prashant.n@samsung.com>
Commit-Queue: Varun Chowdhary Paturi <v.paturi@samsung.com>
Cr-Commit-Position: refs/heads/master@{#715229}

third_party/blink/renderer/core/paint/svg_shape_painter.cc

@@ -164,15 +164,18 @@ void SVGShapePainter::FillShape(GraphicsContext& context,
                                 SkPath::FillType fill_type) {
   switch (layout_svg_shape_.GeometryCodePath()) {
     case kRectGeometryFastPath:
-      context.DrawRect(layout_svg_shape_.ObjectBoundingBox(), flags);
+      context.DrawRect(layout_svg_shape_.ObjectBoundingBox(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
       break;
     case kEllipseGeometryFastPath:
-      context.DrawOval(layout_svg_shape_.ObjectBoundingBox(), flags);
+      context.DrawOval(layout_svg_shape_.ObjectBoundingBox(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
       break;
     default: {
       PathWithTemporaryWindingRule path_with_winding(
           layout_svg_shape_.GetPath(), fill_type);
-      context.DrawPath(path_with_winding.GetSkPath(), flags);
+      context.DrawPath(path_with_winding.GetSkPath(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
     }
   }
 }
@@ -184,17 +187,20 @@ void SVGShapePainter::StrokeShape(GraphicsContext& context,
 
   switch (layout_svg_shape_.GeometryCodePath()) {
     case kRectGeometryFastPath:
-      context.DrawRect(layout_svg_shape_.ObjectBoundingBox(), flags);
+      context.DrawRect(layout_svg_shape_.ObjectBoundingBox(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
       break;
     case kEllipseGeometryFastPath:
-      context.DrawOval(layout_svg_shape_.ObjectBoundingBox(), flags);
+      context.DrawOval(layout_svg_shape_.ObjectBoundingBox(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
       break;
     default:
       DCHECK(layout_svg_shape_.HasPath());
       const Path* use_path = &layout_svg_shape_.GetPath();
       if (layout_svg_shape_.HasNonScalingStroke())
         use_path = &layout_svg_shape_.NonScalingStrokePath();
-      context.DrawPath(use_path->GetSkPath(), flags);
+      context.DrawPath(use_path->GetSkPath(), flags,
+                       DarkModeFilter::ElementRole::kSVG);
   }
 }
 

third_party/blink/renderer/platform/graphics/dark_mode_filter.cc

@@ -172,22 +172,27 @@ bool DarkModeFilter::IsDarkModeActive() const {
 // perform some other logic in between confirming dark mode is active and
 // checking the color classifiers.
 bool DarkModeFilter::ShouldApplyToColor(const Color& color, ElementRole role) {
-  if (role == ElementRole::kBackground) {
-    // Calling get() is necessary below because operator<< in std::unique_ptr is
-    // a C++20 feature.
-    // TODO(https://crbug.com/980914): Drop .get() once we move to C++20.
-    DCHECK_NE(background_classifier_.get(), nullptr);
-    return background_classifier_->ShouldInvertColor(color) ==
-           DarkModeClassification::kApplyFilter;
+  switch (role) {
+    case ElementRole::kText:
+      DCHECK(text_classifier_);
+      return text_classifier_->ShouldInvertColor(color) ==
+             DarkModeClassification::kApplyFilter;
+    case ElementRole::kBackground:
+      DCHECK(background_classifier_);
+      return background_classifier_->ShouldInvertColor(color) ==
+             DarkModeClassification::kApplyFilter;
+    case ElementRole::kSVG:
+      // 1) Inline SVG images are considered as individual shapes and do not
+      // have an Image object associated with them. So they do not go through
+      // the regular image classification pipeline. Do not apply any filter to
+      // the SVG shapes until there is a way to get the classification for the
+      // entire image to which these shapes belong.
+
+      // 2) Non-inline SVG images are already classified at this point and have
+      // a filter applied if necessary.
+      return false;
   }
-
-  DCHECK_EQ(role, ElementRole::kText);
-  // Calling get() is necessary below because operator<< in std::unique_ptr is
-  // a C++20 feature.
-  // TODO(https://crbug.com/980914): Drop .get() once we move to C++20.
-  DCHECK_NE(text_classifier_.get(), nullptr);
-  return text_classifier_->ShouldInvertColor(color) ==
-         DarkModeClassification::kApplyFilter;
+  NOTREACHED();
 }
 
 }  // namespace blink

third_party/blink/renderer/platform/graphics/dark_mode_filter.h

@@ -37,7 +37,7 @@ class PLATFORM_EXPORT DarkModeFilter {
   // TODO(gilmanmh): Add a role for shadows. In general, we don't want to
   // invert shadows, but we may need to do some other kind of processing for
   // them.
-  enum class ElementRole { kText, kBackground };
+  enum class ElementRole { kText, kBackground, kSVG };
   Color InvertColorIfNeeded(const Color& color, ElementRole element_role);
   base::Optional<cc::PaintFlags> ApplyToFlagsIfNeeded(
       const cc::PaintFlags& flags,

third_party/blink/renderer/platform/graphics/dark_mode_filter_test.cc

@@ -49,6 +49,13 @@ TEST(DarkModeFilterTest, ApplyDarkModeToColorsAndFlags) {
             filter.InvertColorIfNeeded(
                 Color::kBlack, DarkModeFilter::ElementRole::kBackground));
 
+  EXPECT_EQ(Color::kWhite,
+            filter.InvertColorIfNeeded(Color::kWhite,
+                                       DarkModeFilter::ElementRole::kSVG));
+  EXPECT_EQ(Color::kBlack,
+            filter.InvertColorIfNeeded(Color::kBlack,
+                                       DarkModeFilter::ElementRole::kSVG));
+
   cc::PaintFlags flags;
   flags.setColor(SK_ColorWHITE);
   auto flags_or_nullopt = filter.ApplyToFlagsIfNeeded(

third_party/blink/renderer/platform/graphics/graphics_context.cc

@@ -1040,24 +1040,24 @@ void GraphicsContext::DrawImageTiled(Image* image,
   paint_controller_.SetImagePainted();
 }
 
-void GraphicsContext::DrawOval(const SkRect& oval, const PaintFlags& flags) {
+void GraphicsContext::DrawOval(const SkRect& oval,
+                               const PaintFlags& flags,
+                               const DarkModeFilter::ElementRole role) {
   if (ContextDisabled())
     return;
   DCHECK(canvas_);
 
-  canvas_->drawOval(
-      oval,
-      DarkModeFlags(this, flags, DarkModeFilter::ElementRole::kBackground));
+  canvas_->drawOval(oval, DarkModeFlags(this, flags, role));
 }
 
-void GraphicsContext::DrawPath(const SkPath& path, const PaintFlags& flags) {
+void GraphicsContext::DrawPath(const SkPath& path,
+                               const PaintFlags& flags,
+                               const DarkModeFilter::ElementRole role) {
   if (ContextDisabled())
     return;
   DCHECK(canvas_);
 
-  canvas_->drawPath(
-      path,
-      DarkModeFlags(this, flags, DarkModeFilter::ElementRole::kBackground));
+  canvas_->drawPath(path, DarkModeFlags(this, flags, role));
 }
 
 void GraphicsContext::DrawRect(const SkRect& rect,

third_party/blink/renderer/platform/graphics/graphics_context.h

@@ -262,8 +262,14 @@ class PLATFORM_EXPORT GraphicsContext {
   // These methods write to the canvas.
   // Also drawLine(const IntPoint& point1, const IntPoint& point2) and
   // fillRoundedRect().
-  void DrawOval(const SkRect&, const PaintFlags&);
-  void DrawPath(const SkPath&, const PaintFlags&);
+  void DrawOval(const SkRect&,
+                const PaintFlags&,
+                const DarkModeFilter::ElementRole role =
+                    DarkModeFilter::ElementRole::kBackground);
+  void DrawPath(const SkPath&,
+                const PaintFlags&,
+                const DarkModeFilter::ElementRole role =
+                    DarkModeFilter::ElementRole::kBackground);
   void DrawRect(const SkRect&,
                 const PaintFlags&,
                 const DarkModeFilter::ElementRole role =

third_party/blink/web_tests/VirtualTestSuites

@@ -246,11 +246,6 @@
     "bases": [],
     "args": ["--blink-settings=darkModeEnabled=true,darkModeInversionAlgorithm=3,darkModePagePolicy=1,darkModeTextBrightnessThreshold=256,darkModeBackgroundBrightnessThreshold=0"]
   },
-  {
-    "prefix": "dark-mode-svg-invert-all",
-    "bases": [],
-    "args": ["--blink-settings=darkModeEnabled=true,darkModeInversionAlgorithm=3,darkModeImagePolicy=0,darkModeTextBrightnessThreshold=256,darkModeBackgroundBrightnessThreshold=0"]
-  },
   {
     "prefix": "presentation",
     "bases": [],

third_party/blink/web_tests/virtual/dark-mode-grayscale-images/desaturate-before-inversion.html

 <!DOCTYPE html>
-<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100">
-  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="2" fill="navy" />
-</svg>
+<img src="resources/svg-image.svg" />

third_party/blink/web_tests/virtual/dark-mode-grayscale-images/resources/svg-image.svg

+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100" width="50px" height="50px">
+  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="4" fill="navy" />
+</svg>

third_party/blink/web_tests/virtual/dark-mode-svg-invert-all/svg-as-image.html → third_party/blink/web_tests/virtual/dark-mode-image-filter-all/svg-as-image.html

 <!DOCTYPE html>
-<img src="resources/circle.svg" />
+<img src="svg-image.svg" />

third_party/blink/web_tests/virtual/dark-mode-svg-invert-all/resources/circle.svg → third_party/blink/web_tests/virtual/dark-mode-image-filter-all/svg-image.svg

-<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100">
-  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="2" fill="white" />
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100" width="50px" height="50px">
+  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="4" fill="white" />
 </svg>

third_party/blink/web_tests/virtual/dark-mode-svg-invert-all/svg-in-html.html → third_party/blink/web_tests/virtual/dark-mode-image-filter-all/svg-in-html.html

 <!DOCTYPE html>
-<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100">
-  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="2" fill="white" />
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 100 100" width="50px" height="50px">
+  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="4" fill="white" />
 </svg>

third_party/blink/web_tests/virtual/dark-mode-svg-invert-all/README.txt

-# This suite runs the tests in LayoutTests/paint/dark-mode
-# with --blink-settings="darkMode=3,darkModeImagePolicy=0"
-# See the virtual_test_suites() method in tools/blinkpy/web_tests/port/base.py.