[CompositeClipPathAnimations] Fix crash caused by position: fixed.

With a translation of 0,0, a DCHECK in DrawingDisplayItem::CalculateRectKnownToBeOpaqueForRecord would fail, as the 'image size' for a deferred image is set by a parameter in the paint worklet input, which in this case was the reference box. This would fail the DCHECK when the clip path was larger than the reference box. An extra parameter has been added to the clip path paint worklet input so that the reference box is independent from the size of the clip area. The area painted when producing the deferred image has also been translated so that its origin is 0,0. This is also necessary to avoid the DCHECK. More about this change: https://docs.google.com/document/d/1gYI6G6ylpxKX0N1L23KPnaKsNOvNMaE-vM3l6RIroqU/edit Note that the test for a version of this crash caused by a rounding error is also affected by another issue (1249071), so currently this test only guards against a crash and not visually incorrect clip paths. This will be resolved in a future CL. This change also sets up a future change that resolves https://bugs.chromium.org/p/chromium/issues/detail?id=1248610 Bug: 1353038, 1249071 Change-Id: I1d62a00a92f38e06e2ae090caefced5fdf289fd9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866054 Reviewed-by: Robert Flack <flackr@chromium.org> Commit-Queue: Claire Chambers <clchambers@microsoft.com> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/main@{#1052298}
chromium · Sep 28, 2022 · 45a1cd0 · 45a1cd0
1 parent 0530ee4
commit 45a1cd0
Show file tree

Hide file tree

Showing 11 changed files with 198 additions and 20 deletions.
diff --git a/cc/paint/paint_image.cc b/cc/paint/paint_image.cc
@@ -160,8 +160,8 @@ SkImageInfo PaintImage::GetSkImageInfo() const {
     return cached_sk_image_->imageInfo();
   } else if (paint_worklet_input_) {
     auto size = paint_worklet_input_->GetSize();
-    return SkImageInfo::MakeUnknown(static_cast<int>(size.width()),
-                                    static_cast<int>(size.height()));
+    return SkImageInfo::MakeUnknown(base::ClampCeil(size.width()),
+                                    base::ClampCeil(size.height()));
   } else {
     return SkImageInfo::MakeUnknown();
   }

diff --git a/third_party/blink/renderer/core/css/clip_path_paint_image_generator.h b/third_party/blink/renderer/core/css/clip_path_paint_image_generator.h
@@ -31,6 +31,7 @@ class CORE_EXPORT ClipPathPaintImageGenerator
 
   virtual scoped_refptr<Image> Paint(float zoom,
                                      const gfx::RectF& reference_box,
+                                     const gfx::SizeF& clip_area_size,
                                      const Node&) = 0;
 };
 

diff --git a/third_party/blink/renderer/core/paint/clip_path_clipper.cc b/third_party/blink/renderer/core/paint/clip_path_clipper.cc
@@ -105,19 +105,32 @@ static void PaintWorkletBasedClip(GraphicsContext& context,
   ClipPathPaintImageGenerator* generator =
       clip_path_owner.GetFrame()->GetClipPathPaintImageGenerator();
 
-  scoped_refptr<Image> paint_worklet_image =
-      generator->Paint(zoom, reference_box, *clip_path_owner.GetNode());
-
   // TODO(crbug.com/1248610): Fix bounding box. It should enclose affected area
   // of the animation.
+  // The bounding rect of the clip-path animation, relative to the layout
+  // object.
   absl::optional<gfx::RectF> bounding_box =
       ClipPathClipper::LocalClipPathBoundingBox(clip_path_owner);
   DCHECK(bounding_box);
-  gfx::RectF src_rect = bounding_box.value();
+  // The mask image should be the same size as the bounding rect, but will have
+  // an origin of 0,0 as it has its own coordinate space.
+  gfx::RectF src_rect = gfx::RectF(bounding_box.value().size());
+  gfx::RectF dst_rect = bounding_box.value();
+
+  scoped_refptr<Image> paint_worklet_image = generator->Paint(
+      zoom,
+      /* Translate the reference box such that it is relative to the origin of
+         the mask image, and not the origin of the layout object. This ensures
+         the clip path remains within the bounds of the mask image and has the
+         correct translation. */
+      gfx::RectF(reference_box.origin() - dst_rect.origin().OffsetFromOrigin(),
+                 reference_box.size()),
+
+      dst_rect.size(), *clip_path_owner.GetNode());
   // Dark mode should always be disabled for clip mask.
   context.DrawImage(paint_worklet_image.get(), Image::kSyncDecode,
                     ImageAutoDarkMode::Disabled(), ImagePaintTimingInfo(),
-                    src_rect, &src_rect, SkBlendMode::kSrcOver,
+                    dst_rect, &src_rect, SkBlendMode::kSrcOver,
                     kRespectImageOrientation);
 }
 
@@ -254,6 +267,7 @@ void ClipPathClipper::PaintClipPathAsMaskImage(
   if (HasCompositeClipPathAnimation(layout_object)) {
     if (!layout_object.GetFrame())
       return;
+
     PaintWorkletBasedClip(context, layout_object, reference_box,
                           uses_zoomed_reference_box);
   } else {

diff --git a/third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition.cc b/third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition.cc
@@ -32,19 +32,19 @@ namespace {
 class ClipPathPaintWorkletInput : public PaintWorkletInput {
  public:
   ClipPathPaintWorkletInput(
-      const gfx::RectF& container_rect,
+      const gfx::RectF& reference_box,
+      const gfx::SizeF& clip_area_size,
       int worklet_id,
       float zoom,
       const Vector<scoped_refptr<BasicShape>>& animated_shapes,
       const Vector<double>& offsets,
       const absl::optional<double>& progress,
       cc::PaintWorkletInput::PropertyKeys property_keys)
-      : PaintWorkletInput(container_rect.size(),
-                          worklet_id,
-                          std::move(property_keys)),
+      : PaintWorkletInput(clip_area_size, worklet_id, std::move(property_keys)),
         zoom_(zoom),
         offsets_(offsets),
-        progress_(progress) {
+        progress_(progress),
+        reference_box_(reference_box) {
     for (const auto& basic_shape : animated_shapes) {
       InterpolationValue interpolation_value =
           CreateInterpolationValue(*basic_shape.get());
@@ -71,6 +71,8 @@ class ClipPathPaintWorkletInput : public PaintWorkletInput {
     return PaintWorkletInputType::kClipPath;
   }
 
+  gfx::RectF GetReferenceBox() const { return reference_box_; }
+
  private:
   InterpolationValue CreateInterpolationValue(const BasicShape& basic_shape) {
     if (basic_shape.GetType() == BasicShape::kStylePathType) {
@@ -86,6 +88,7 @@ class ClipPathPaintWorkletInput : public PaintWorkletInput {
   Vector<InterpolationValue> interpolation_values_;
   Vector<double> offsets_;
   absl::optional<double> progress_;
+  gfx::RectF reference_box_;
 };
 
 bool ShapesAreCompatible(const NonInterpolableValue& a,
@@ -228,7 +231,8 @@ sk_sp<PaintRecord> ClipPathPaintDefinition::Paint(
         animated_property_values) {
   const ClipPathPaintWorkletInput* input =
       To<ClipPathPaintWorkletInput>(compositor_input);
-  gfx::SizeF container_size = input->ContainerSize();
+  gfx::SizeF clip_area_size = input->ContainerSize();
+  gfx::RectF reference_box = input->GetReferenceBox();
 
   const Vector<InterpolationValue>& interpolation_values =
       input->InterpolationValues();
@@ -296,11 +300,11 @@ sk_sp<PaintRecord> ClipPathPaintDefinition::Paint(
   scoped_refptr<ShapeClipPathOperation> current_shape =
       ShapeClipPathOperation::Create(result_shape);
 
-  Path path = current_shape->GetPath(gfx::RectF(container_size), input->Zoom());
+  Path path = current_shape->GetPath(reference_box, input->Zoom());
   PaintRenderingContext2DSettings* context_settings =
       PaintRenderingContext2DSettings::Create();
   auto* rendering_context = MakeGarbageCollected<PaintRenderingContext2D>(
-      gfx::ToRoundedSize(container_size), context_settings, 1, 1);
+      gfx::ToRoundedSize(clip_area_size), context_settings, 1, 1);
 
   cc::PaintFlags flags;
   flags.setAntiAlias(true);
@@ -309,9 +313,13 @@ sk_sp<PaintRecord> ClipPathPaintDefinition::Paint(
   return rendering_context->GetRecord();
 }
 
+// Creates a deferred image of size clip_area_size that will be painted via
+// paint worklet. The clip paths will be scaled and translated according to
+// reference_box.
 scoped_refptr<Image> ClipPathPaintDefinition::Paint(
     float zoom,
     const gfx::RectF& reference_box,
+    const gfx::SizeF& clip_area_size,
     const Node& node) {
   DCHECK(node.IsElementNode());
   const Element* element = static_cast<Element*>(const_cast<Node*>(&node));
@@ -350,11 +358,10 @@ scoped_refptr<Image> ClipPathPaintDefinition::Paint(
       CompositorPaintWorkletInput::NativePropertyType::kClipPath, element_id);
   scoped_refptr<ClipPathPaintWorkletInput> input =
       base::MakeRefCounted<ClipPathPaintWorkletInput>(
-          reference_box, worklet_id_, zoom, animated_shapes, offsets, progress,
-          std::move(input_property_keys));
+          reference_box, clip_area_size, worklet_id_, zoom, animated_shapes,
+          offsets, progress, std::move(input_property_keys));
 
-  return PaintWorkletDeferredImage::Create(std::move(input),
-                                           reference_box.size());
+  return PaintWorkletDeferredImage::Create(std::move(input), clip_area_size);
 }
 
 void ClipPathPaintDefinition::Trace(Visitor* visitor) const {

diff --git a/third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition.h b/third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition.h
@@ -37,6 +37,7 @@ class MODULES_EXPORT ClipPathPaintDefinition final
 
   scoped_refptr<Image> Paint(float zoom,
                              const gfx::RectF& reference_box,
+                             const gfx::SizeF& clip_area_size,
                              const Node&);
   static Animation* GetAnimationIfCompositable(const Element* element);
   void Trace(Visitor* visitor) const override;

diff --git a/...party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_image_generator_impl.cc b/...party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_image_generator_impl.cc
@@ -29,8 +29,10 @@ ClipPathPaintImageGeneratorImpl::ClipPathPaintImageGeneratorImpl(
 scoped_refptr<Image> ClipPathPaintImageGeneratorImpl::Paint(
     float zoom,
     const gfx::RectF& reference_box,
+    const gfx::SizeF& clip_area_size,
     const Node& node) {
-  return clip_path_paint_definition_->Paint(zoom, reference_box, node);
+  return clip_path_paint_definition_->Paint(zoom, reference_box, clip_area_size,
+                                            node);
 }
 
 Animation* ClipPathPaintImageGeneratorImpl::GetAnimationIfCompositable(

diff --git a/..._party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_image_generator_impl.h b/..._party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_image_generator_impl.h
@@ -24,6 +24,7 @@ class MODULES_EXPORT ClipPathPaintImageGeneratorImpl final
 
   scoped_refptr<Image> Paint(float zoom,
                              const gfx::RectF& reference_box,
+                             const gfx::SizeF& clip_area_size,
                              const Node&) final;
   Animation* GetAnimationIfCompositable(const Element* element) final;
 

diff --git a/...rnal/wpt/css/css-masking/clip-path/animations/clip-path-animation-fixed-position-ref.html b/...rnal/wpt/css/css-masking/clip-path/animations/clip-path-animation-fixed-position-ref.html
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<style>
+  .container {
+    position: fixed;
+    top: 100px;
+    left: 100px;
+    width: 100px;
+    height: 100px;
+    background-color: green;
+    clip-path: circle(200% at 35% 35%);
+  }
+
+  .big {
+    position: absolute;
+    top: 100px;
+    width: 500px;
+    height: 500px;
+    background-color: blue;
+  }
+</style>
+
+<body>
+  <div class="container">
+    <div class="big"></div>
+  </div>
+</body>
+
+</html>
diff --git a/...s-masking/clip-path/animations/clip-path-animation-fixed-position-rounding-error-ref.html b/...s-masking/clip-path/animations/clip-path-animation-fixed-position-rounding-error-ref.html
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<style>
+  .container {
+    position: fixed;
+    width: 70px;
+    height: 126px;
+    background-color: green;
+    clip-path: inset(5% 5%)
+  }
+
+</style>
+
+<body>
+  <div class="container"></div>
+</body>
+
+</html>
diff --git a/...s/css-masking/clip-path/animations/clip-path-animation-fixed-position-rounding-error.html b/...s/css-masking/clip-path/animations/clip-path-animation-fixed-position-rounding-error.html
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<meta name=fuzzy content="0-129;0-115">
+<link rel="help" href="https://drafts.csswg.org/css-shapes-1/#basic-shape-interpolation">
+<link rel="match" href="clip-path-animation-fixed-position-rounding-error-ref.html">
+<!--
+  Test that clip paths on elements with position: fixed draw correctly,
+  even in scenarios that involve partial pixels
+
+  Currently uses fuzzy diff because of crbug.com/1249071
+-->
+<style>
+  .container {
+    background-color: green;
+    /* Use a long animation that start at 50% progress where the slope of the
+     selected timing function is zero. By setting up the animation in this way,
+     we accommodate lengthy delays in running the test without a potential drift
+     in the animated property value. This is important for avoiding flakes,
+     especially on debug builds. The screenshots are taken as soon as the
+     animation is ready, thus the long animation duration has no bearing on
+     the actual duration of the test. */
+    animation: clippath 1000000s cubic-bezier(0, 1, 1, 0) -500000s;
+    position: fixed;
+    width: 70px;
+    height: 126px;
+  }
+
+  @keyframes clippath {
+    0% {
+      clip-path: inset(0% 0%);
+    }
+
+    100% {
+      clip-path: inset(10% 10%);
+    }
+  }
+
+</style>
+<script src="/common/reftest-wait.js"></script>
+
+<body>
+  <div class="container"></div>
+
+  <script>
+    document.getAnimations()[0].ready.then(takeScreenshot);
+  </script>
+</body>
+
+</html>
diff --git a/...external/wpt/css/css-masking/clip-path/animations/clip-path-animation-fixed-position.html b/...external/wpt/css/css-masking/clip-path/animations/clip-path-animation-fixed-position.html
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<link rel="help" href="https://drafts.csswg.org/css-shapes-1/#basic-shape-interpolation">
+<link rel="match" href="clip-path-animation-fixed-position-ref.html">
+<!--
+  Test that clip paths on elements with position: fixed draw correctly,
+  including clipping children that are out of bounds.
+-->
+<style>
+  .container {
+    width: 100px;
+    height: 100px;
+    position: fixed;
+    top: 100px;
+    left: 100px;
+    background-color: green;
+    /* Use a long animation that start at 50% progress where the slope of the
+     selected timing function is zero. By setting up the animation in this way,
+     we accommodate lengthy delays in running the test without a potential drift
+     in the animated property value. This is important for avoiding flakes,
+     especially on debug builds. The screenshots are taken as soon as the
+     animation is ready, thus the long animation duration has no bearing on
+     the actual duration of the test. */
+    animation: clippath 1000000s cubic-bezier(0, 1, 1, 0) -500000s;
+  }
+
+  .big {
+    position: absolute;
+    top: 100px;
+    width: 500px;
+    height: 500px;
+    background-color: blue;
+  }
+
+  @keyframes clippath {
+    0% {
+      clip-path: circle(50% at 50% 50%);
+    }
+
+    100% {
+      clip-path: circle(350% at 20% 20%);
+    }
+  }
+
+</style>
+<script src="/common/reftest-wait.js"></script>
+
+<body>
+  <div class="container">
+    <div class="big"></div>
+  </div>
+
+  <script>
+    document.getAnimations()[0].ready.then(takeScreenshot);
+  </script>
+</body>
+
+</html>