Use layout coordinates for LayoutClipRect of ClipPathClip and MaskClip

This is to ensure correct inclusive intersection for empty objects with empty clip-path/mask. Previously, we failed to detect viewport intersection and start loading a lazy loading <img> which was empty with an empty clip-path before the image was loaded. A change (crrev.com/936552) from blink::EnclosingIntRect(FloatRect) to gfx::ToEnclosingRect(gfx::RectF) exposed the issue. The difference between the two functions was that for an empty input rect with non-integral origin, the former returned an non-empty IntRect, while the latter returns an empty gfx::Rect. An alternative to this CL is to add a version of gfx::ToEnclosingRect() behaving the same as blink::EnclosingIntRect(), but I think this CL is better by making LayoutClipRect for ClipPathClip and MaskClip correct. Bug: 1308299, 1248598 Change-Id: I242c1d38277cdd91aa596fda15156d991f2626d7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3568746 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/main@{#989750}
chromium · Apr 7, 2022 · cc453fd · cc453fd
1 parent 5357c2e
commit cc453fd
Show file tree

Hide file tree

Showing 11 changed files with 118 additions and 38 deletions.
diff --git a/third_party/blink/renderer/core/paint/css_mask_painter.cc b/third_party/blink/renderer/core/paint/css_mask_painter.cc
@@ -8,11 +8,10 @@
 #include "third_party/blink/renderer/core/layout/layout_inline.h"
 #include "third_party/blink/renderer/core/layout/svg/layout_svg_resource_masker.h"
 #include "third_party/blink/renderer/core/layout/svg/svg_resources.h"
-#include "ui/gfx/geometry/rect_conversions.h"
 
 namespace blink {
 
-absl::optional<gfx::Rect> CSSMaskPainter::MaskBoundingBox(
+absl::optional<gfx::RectF> CSSMaskPainter::MaskBoundingBox(
     const LayoutObject& object,
     const PhysicalOffset& paint_offset) {
   if (!object.IsBoxModelObject() && !object.IsSVGChild())
@@ -28,8 +27,7 @@ absl::optional<gfx::Rect> CSSMaskPainter::MaskBoundingBox(
             SVGResources::ReferenceBoxForEffects(object);
         const float reference_box_zoom =
             object.IsSVGForeignObject() ? object.StyleRef().EffectiveZoom() : 1;
-        return gfx::ToEnclosingRect(
-            masker->ResourceBoundingBox(reference_box, reference_box_zoom));
+        return masker->ResourceBoundingBox(reference_box, reference_box_zoom);
       }
     }
   }
@@ -56,7 +54,7 @@ absl::optional<gfx::Rect> CSSMaskPainter::MaskBoundingBox(
   if (style.HasMaskBoxImageOutsets())
     maximum_mask_region.Expand(style.MaskBoxImageOutsets());
   maximum_mask_region.offset += paint_offset;
-  return ToPixelSnappedRect(maximum_mask_region);
+  return gfx::RectF(maximum_mask_region);
 }
 
 }  // namespace blink
diff --git a/third_party/blink/renderer/core/paint/css_mask_painter.h b/third_party/blink/renderer/core/paint/css_mask_painter.h
@@ -8,7 +8,7 @@
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
-#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_f.h"
 
 namespace blink {
 
@@ -22,7 +22,7 @@ class CORE_EXPORT CSSMaskPainter {
   // Returns the bounding box of the computed mask, which could be
   // smaller or bigger than the reference box. Returns nullopt if the
   // there is no mask or the mask is invalid.
-  static absl::optional<gfx::Rect> MaskBoundingBox(
+  static absl::optional<gfx::RectF> MaskBoundingBox(
       const LayoutObject&,
       const PhysicalOffset& paint_offset);
 };

diff --git a/third_party/blink/renderer/core/paint/css_mask_painter_test.cc b/third_party/blink/renderer/core/paint/css_mask_painter_test.cc
@@ -25,10 +25,10 @@ TEST_F(CSSMaskPainterTest, MaskBoundingBoxSVG) {
     </svg>
   )HTML");
   auto& masked = *GetLayoutObjectByElementId("masked");
-  absl::optional<gfx::Rect> mask_bounding_box =
+  absl::optional<gfx::RectF> mask_bounding_box =
       CSSMaskPainter::MaskBoundingBox(masked, PhysicalOffset());
   ASSERT_TRUE(mask_bounding_box.has_value());
-  EXPECT_EQ(gfx::Rect(35, 35, 180, 180), *mask_bounding_box);
+  EXPECT_EQ(gfx::RectF(35, 35, 180, 180), *mask_bounding_box);
 }
 
 TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSBlock) {
@@ -37,10 +37,15 @@ TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSBlock) {
                             width:300px; height:200px;"></div>
   )HTML");
   auto& masked = *GetLayoutObjectByElementId("masked");
-  absl::optional<gfx::Rect> mask_bounding_box =
+  absl::optional<gfx::RectF> mask_bounding_box =
       CSSMaskPainter::MaskBoundingBox(masked, PhysicalOffset(8, 8));
   ASSERT_TRUE(mask_bounding_box.has_value());
-  EXPECT_EQ(gfx::Rect(8, 8, 300, 200), *mask_bounding_box);
+  EXPECT_EQ(gfx::RectF(8, 8, 300, 200), *mask_bounding_box);
+
+  mask_bounding_box = CSSMaskPainter::MaskBoundingBox(
+      masked, PhysicalOffset(LayoutUnit(8.25f), LayoutUnit(8.75f)));
+  ASSERT_TRUE(mask_bounding_box.has_value());
+  EXPECT_EQ(gfx::RectF(8.25, 8.75, 300, 200), *mask_bounding_box);
 }
 
 TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSMaskBoxImageOutset) {
@@ -50,10 +55,10 @@ TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSMaskBoxImageOutset) {
         -webkit-mask-box-image-outset:10px; width:300px; height:200px;"></div>
   )HTML");
   auto& masked = *GetLayoutObjectByElementId("masked");
-  absl::optional<gfx::Rect> mask_bounding_box =
+  absl::optional<gfx::RectF> mask_bounding_box =
       CSSMaskPainter::MaskBoundingBox(masked, PhysicalOffset(8, 8));
   ASSERT_TRUE(mask_bounding_box.has_value());
-  EXPECT_EQ(gfx::Rect(-2, -2, 320, 220), *mask_bounding_box);
+  EXPECT_EQ(gfx::RectF(-2, -2, 320, 220), *mask_bounding_box);
 }
 
 TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSInline) {
@@ -68,10 +73,10 @@ TEST_F(CSSMaskPainterTest, MaskBoundingBoxCSSInline) {
     </div>
   )HTML");
   auto& masked = *GetLayoutObjectByElementId("masked");
-  absl::optional<gfx::Rect> mask_bounding_box =
+  absl::optional<gfx::RectF> mask_bounding_box =
       CSSMaskPainter::MaskBoundingBox(masked, PhysicalOffset(8, 8));
   ASSERT_TRUE(mask_bounding_box.has_value());
-  EXPECT_EQ(gfx::Rect(8, 8, 260, 20), *mask_bounding_box);
+  EXPECT_EQ(gfx::RectF(8, 8, 260, 20), *mask_bounding_box);
 }
 
 }  // unnamed namespace

diff --git a/third_party/blink/renderer/core/paint/fragment_data.cc b/third_party/blink/renderer/core/paint/fragment_data.cc
@@ -166,7 +166,7 @@ void FragmentData::InvalidateClipPathCache() {
   rare_data_->clip_path_path = nullptr;
 }
 
-void FragmentData::SetClipPathCache(const gfx::Rect& bounding_box,
+void FragmentData::SetClipPathCache(const gfx::RectF& bounding_box,
                                     scoped_refptr<const RefCountedPath> path) {
   EnsureRareData().is_clip_path_cache_valid = true;
   rare_data_->clip_path_bounding_box = bounding_box;

diff --git a/third_party/blink/renderer/core/paint/fragment_data.h b/third_party/blink/renderer/core/paint/fragment_data.h
@@ -102,15 +102,15 @@ class CORE_EXPORT FragmentData final : public GarbageCollected<FragmentData> {
   }
   void InvalidateClipPathCache();
 
-  absl::optional<gfx::Rect> ClipPathBoundingBox() const {
+  absl::optional<gfx::RectF> ClipPathBoundingBox() const {
     DCHECK(IsClipPathCacheValid());
     return rare_data_ ? rare_data_->clip_path_bounding_box : absl::nullopt;
   }
   const RefCountedPath* ClipPathPath() const {
     DCHECK(IsClipPathCacheValid());
     return rare_data_ ? rare_data_->clip_path_path.get() : nullptr;
   }
-  void SetClipPathCache(const gfx::Rect& bounding_box,
+  void SetClipPathCache(const gfx::RectF& bounding_box,
                         scoped_refptr<const RefCountedPath>);
   void ClearClipPathCache() {
     if (rare_data_) {
@@ -248,7 +248,7 @@ class CORE_EXPORT FragmentData final : public GarbageCollected<FragmentData> {
     std::unique_ptr<ObjectPaintProperties> paint_properties;
     std::unique_ptr<RefCountedPropertyTreeState> local_border_box_properties;
     bool is_clip_path_cache_valid = false;
-    absl::optional<gfx::Rect> clip_path_bounding_box;
+    absl::optional<gfx::RectF> clip_path_bounding_box;
     scoped_refptr<const RefCountedPath> clip_path_path;
     CullRect cull_rect_;
     CullRect contents_cull_rect_;

diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc b/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
@@ -1166,13 +1166,13 @@ void FragmentPaintPropertyTreeBuilder::UpdateEffect() {
 
   if (NeedsPaintPropertyUpdate()) {
     if (NeedsEffect(object_, full_context_.direct_compositing_reasons)) {
-      absl::optional<gfx::Rect> mask_clip = CSSMaskPainter::MaskBoundingBox(
+      absl::optional<gfx::RectF> mask_clip = CSSMaskPainter::MaskBoundingBox(
           object_, context_.current.paint_offset);
       bool has_clip_path =
           style.HasClipPath() && fragment_data_.ClipPathBoundingBox();
       bool has_mask_based_clip_path =
           has_clip_path && !fragment_data_.ClipPathPath();
-      absl::optional<gfx::Rect> clip_path_clip;
+      absl::optional<gfx::RectF> clip_path_clip;
       if (has_mask_based_clip_path)
         clip_path_clip = fragment_data_.ClipPathBoundingBox();
 
@@ -1181,18 +1181,15 @@ void FragmentPaintPropertyTreeBuilder::UpdateEffect() {
                                     : nullptr;
 
       if (mask_clip || clip_path_clip) {
-        gfx::Rect combined_clip = mask_clip ? *mask_clip : *clip_path_clip;
+        gfx::RectF combined_clip = mask_clip ? *mask_clip : *clip_path_clip;
         if (mask_clip && clip_path_clip)
           combined_clip.Intersect(*clip_path_clip);
 
-        // TODO(crbug.com/1248598): We use pixel-snapped mask clip and clip path
-        // clip as the layout clip rect, which may cause wrong result when
-        // mapping in layout coordinates.
         OnUpdate(properties_->UpdateMaskClip(
             *context_.current.clip,
-            ClipPaintPropertyNode::State(context_.current.transform,
-                                         gfx::RectF(combined_clip),
-                                         FloatRoundedRect(combined_clip))));
+            ClipPaintPropertyNode::State(
+                context_.current.transform, combined_clip,
+                FloatRoundedRect(gfx::ToEnclosingRect(combined_clip)))));
         output_clip = properties_->MaskClip();
       } else {
         OnClear(properties_->ClearMaskClip());
@@ -1585,13 +1582,10 @@ void FragmentPaintPropertyTreeBuilder::UpdateClipPathClip() {
     if (!NeedsClipPathClip(object_, fragment_data_)) {
       OnClear(properties_->ClearClipPathClip());
     } else {
-      // TODO(crbug.com/1248598): We use pixel-snapped clip path clip as the
-      // layout clip rect, which may cause wrong result when mapping in layout
-      // coordinates.
       ClipPaintPropertyNode::State state(
-          context_.current.transform,
-          gfx::RectF(*fragment_data_.ClipPathBoundingBox()),
-          FloatRoundedRect(*fragment_data_.ClipPathBoundingBox()));
+          context_.current.transform, *fragment_data_.ClipPathBoundingBox(),
+          FloatRoundedRect(
+              gfx::ToEnclosingRect(*fragment_data_.ClipPathBoundingBox())));
       state.clip_path = fragment_data_.ClipPathPath();
       OnUpdate(properties_->UpdateClipPathClip(*context_.current.clip,
                                                std::move(state)));
@@ -2688,7 +2682,7 @@ void FragmentPaintPropertyTreeBuilder::UpdateClipPathCache() {
   if (path)
     path->Translate(gfx::Vector2dF(fragment_data_.PaintOffset()));
   fragment_data_.SetClipPathCache(
-      gfx::ToEnclosingRect(*bounding_box),
+      *bounding_box,
       path ? AdoptRef(new RefCountedPath(std::move(*path))) : nullptr);
 }
 

diff --git a/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc b/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
@@ -5154,9 +5154,9 @@ TEST_P(PaintPropertyTreeBuilderTest, MaskSimple) {
   EXPECT_EQ(output_clip,
             &target->FirstFragment().LocalBorderBoxProperties().Clip());
   EXPECT_EQ(DocContentClip(), output_clip->Parent());
-  // For now we always pixel-snap both LayoutClipRect and PaintClipRect for
-  // mask clip.
-  EXPECT_CLIP_RECT(FloatRoundedRect(8, 8, 300, 201), output_clip);
+  EXPECT_EQ(FloatClipRect(gfx::RectF(8, 8, 300, 200.5)),
+            output_clip->LayoutClipRect());
+  EXPECT_EQ(FloatRoundedRect(8, 8, 300, 201), output_clip->PaintClipRect());
 
   EXPECT_EQ(properties->Effect(),
             &target->FirstFragment().LocalBorderBoxProperties().Effect());
@@ -5771,6 +5771,42 @@ TEST_P(PaintPropertyTreeBuilderTest, ClearClipPathEffectNode) {
   }
 }
 
+TEST_P(PaintPropertyTreeBuilderTest, EmptyClipPathSubpixelOffset) {
+  SetBodyInnerHTML(R"HTML(
+    <style>body { margin: 0; }</style>
+    <div id="target"
+         style="clip-path: polygon(0 0, 100% 0, 100% 100%, 0 100%, 0 0);
+                position: relative; top: 0.75px; left: 0.25px; width: 0">
+    </div>
+  )HTML");
+
+  const auto* target = GetLayoutObjectByElementId("target");
+  ASSERT_TRUE(target->FirstFragment().PaintProperties());
+  const auto* clip_path_clip =
+      target->FirstFragment().PaintProperties()->ClipPathClip();
+  ASSERT_TRUE(clip_path_clip);
+  EXPECT_EQ(gfx::RectF(0.25, 0.75, 0, 0),
+            clip_path_clip->LayoutClipRect().Rect());
+  EXPECT_EQ(FloatRoundedRect(), clip_path_clip->PaintClipRect());
+}
+
+TEST_P(PaintPropertyTreeBuilderTest, EmptyMaskSubpixelOffset) {
+  SetBodyInnerHTML(R"HTML(
+    <style>body { margin: 0; }</style>
+    <div id="target"
+         style="-webkit-mask: linear-gradient(blue, white);
+                position: relative; top: 0.75px; left: 0.25px; width: 0">
+    </div>
+  )HTML");
+
+  const auto* target = GetLayoutObjectByElementId("target");
+  ASSERT_TRUE(target->FirstFragment().PaintProperties());
+  const auto* mask_clip = target->FirstFragment().PaintProperties()->MaskClip();
+  ASSERT_TRUE(mask_clip);
+  EXPECT_EQ(gfx::RectF(0.25, 0.75, 0, 0), mask_clip->LayoutClipRect().Rect());
+  EXPECT_EQ(FloatRoundedRect(), mask_clip->PaintClipRect());
+}
+
 TEST_P(PaintPropertyTreeBuilderTest, RootHasCompositedScrolling) {
   SetBodyInnerHTML(R"HTML(
     <div id='forceScroll' style='height: 2000px'></div>

diff --git a/...wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-clip-path-ref.html b/...wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-clip-path-ref.html
@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<img src="resources/image.png">
diff --git a/...nal/wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-clip-path.html b/...nal/wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-clip-path.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<link rel="author" title="Xianzhu Wang" href="mailto:wangxianzhu@chromium.org">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes">
+<link rel="help" href="https://crbug.com/1308299">
+<link rel="match" href="image-loading-lazy-clip-path-ref.html">
+<script src="/common/reftest-wait.js"></script>
+<style>
+img {
+  clip-path: polygon(10% 0, 90% 0, 90% 90%, 0 90%, 0 10%);
+  vertical-align: middle;
+}
+</style>
+<img id=target loading="lazy"
+      src="resources/image.png"
+      style="vertical-align: middle; clip-path: polygon(0 0, 110% 0, 110% 110%, 0 110%, 0 0)">
+<script>
+  target.onload = takeScreenshot;
+</script>
+</html>
diff --git a/...external/wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-mask.html b/...external/wpt/html/semantics/embedded-content/the-img-element/image-loading-lazy-mask.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<link rel="author" title="Xianzhu Wang" href="mailto:wangxianzhu@chromium.org">
+<link rel="help" href="https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes">
+<link rel="help" href="https://crbug.com/1308299">
+<link rel="match" href="image-loading-lazy-clip-path-ref.html">
+<script src="/common/reftest-wait.js"></script>
+<style>
+img {
+  mask: url(#mask);
+  vertical-align: middle;
+}
+</style>
+<svg style="display: none">
+  <mask id="mask">
+    <rect width="1000" height="1000" fill="white"/>
+  </mask>
+</svg>
+<img id=target loading="lazy"
+      src="resources/image.png"
+      style="vertical-align: middle; clip-path: polygon(0 0, 110% 0, 110% 110%, 0 110%, 0 0)">
+<script>
+  target.onload = takeScreenshot;
+</script>
+</html>
diff --git a/...ty/blink/web_tests/flag-specific/highdpi/paint/masks/fieldset-mask-expected.png b/...ty/blink/web_tests/flag-specific/highdpi/paint/masks/fieldset-mask-expected.png