Make sure layout is triggered when changing clip-path resource

When changing the 'clip-path' property on an (SVG) element we need to
recompute the visual rect since it includes contributions from the clip
bounds (as well as the mask and filter bounds). This wasn't happening,
which meant that the element wouldn't repaint properly if the old bounds
were smaller than the new bounds.
Add some detection of bounds-affecting changes to SVGResourcesCache and
Make use of that in SVGResourcesCache::ClientStyleChanged() to mark the
element for layout if needed so that its bounds are updated properly.

Fixed: 1045915
Change-Id: Ia08904945b26c382e0b9c90c15b7cc45d40434cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023561Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#736486}

third_party/blink/renderer/core/layout/svg/svg_resources.cc

@@ -277,6 +277,18 @@ void SVGResources::LayoutIfNeeded() {
     linked_resource_->LayoutIfNeeded();
 }
 
+bool SVGResources::DifferenceNeedsLayout(const SVGResources* a,
+                                         const SVGResources* b) {
+  bool a_has_bounds_affecting_resource = a && a->clipper_filter_masker_data_;
+  bool b_has_bounds_affecting_resource = b && b->clipper_filter_masker_data_;
+  if (a_has_bounds_affecting_resource != b_has_bounds_affecting_resource)
+    return true;
+  if (!a_has_bounds_affecting_resource)
+    return false;
+  return a->Clipper() != b->Clipper() || a->Filter() != b->Filter() ||
+         a->Masker() != b->Masker();
+}
+
 InvalidationModeMask SVGResources::RemoveClientFromCacheAffectingObjectBounds(
     SVGResourceClient& client) const {
   if (!clipper_filter_masker_data_)

third_party/blink/renderer/core/layout/svg/svg_resources.h

@@ -116,6 +116,8 @@ class SVGResources {
   void ResourceDestroyed(LayoutSVGResourceContainer*);
   void ClearReferencesTo(LayoutSVGResourceContainer*);
 
+  static bool DifferenceNeedsLayout(const SVGResources*, const SVGResources*);
+
 #if DCHECK_IS_ON()
   void Dump(const LayoutObject*);
 #endif

third_party/blink/renderer/core/layout/svg/svg_resources_cache.cc

@@ -32,7 +32,7 @@ SVGResourcesCache::SVGResourcesCache() = default;
 
 SVGResourcesCache::~SVGResourcesCache() = default;
 
-bool SVGResourcesCache::AddResourcesFromLayoutObject(
+SVGResources* SVGResourcesCache::AddResourcesFromLayoutObject(
     LayoutObject& object,
     const ComputedStyle& style) {
   DCHECK(!cache_.Contains(&object));
@@ -41,7 +41,7 @@ bool SVGResourcesCache::AddResourcesFromLayoutObject(
   std::unique_ptr<SVGResources> new_resources =
       SVGResources::BuildResources(object, style);
   if (!new_resources)
-    return false;
+    return nullptr;
 
   // Put object in cache.
   SVGResources* resources =
@@ -56,7 +56,7 @@ bool SVGResourcesCache::AddResourcesFromLayoutObject(
     if (solver.FindCycle(resource_container))
       resources->ClearReferencesTo(resource_container);
   }
-  return true;
+  return resources;
 }
 
 bool SVGResourcesCache::RemoveResourcesFromLayoutObject(LayoutObject& object) {
@@ -64,12 +64,15 @@ bool SVGResourcesCache::RemoveResourcesFromLayoutObject(LayoutObject& object) {
   return !!resources;
 }
 
-bool SVGResourcesCache::UpdateResourcesFromLayoutObject(
+SVGResourcesCache::ResourceUpdateInfo
+SVGResourcesCache::UpdateResourcesFromLayoutObject(
     LayoutObject& object,
     const ComputedStyle& new_style) {
-  bool did_update = RemoveResourcesFromLayoutObject(object);
-  did_update |= AddResourcesFromLayoutObject(object, new_style);
-  return did_update;
+  std::unique_ptr<SVGResources> old_resources = cache_.Take(&object);
+  SVGResources* new_resources = AddResourcesFromLayoutObject(object, new_style);
+  return {
+      old_resources || new_resources,
+      SVGResources::DifferenceNeedsLayout(old_resources.get(), new_resources)};
 }
 
 static inline SVGResourcesCache& ResourcesCache(Document& document) {
@@ -135,17 +138,26 @@ void SVGResourcesCache::ClientStyleChanged(LayoutObject& layout_object,
   // TODO(fs): Avoid passing in a useless StyleDifference, but instead compare
   // oldStyle/newStyle to see which resources changed to be able to selectively
   // rebuild individual resources, instead of all of them.
+  bool needs_layout = false;
   if (LayoutObjectCanHaveResources(layout_object)) {
     SVGResourcesCache& cache = ResourcesCache(layout_object.GetDocument());
-    if (cache.UpdateResourcesFromLayoutObject(layout_object, new_style))
+    auto update_info =
+        cache.UpdateResourcesFromLayoutObject(layout_object, new_style);
+    if (update_info) {
       layout_object.SetNeedsPaintPropertyUpdate();
+      // Since the visual rect has the bounds of the clip-path, mask and filter
+      // baked in, and the visual rect is updated during layout, we need to
+      // trigger layout if the style change could somehow have affected the
+      // bounds that form the visual rect.
+      needs_layout = update_info.needs_layout;
+    }
   }
 
   // If this layoutObject is the child of ResourceContainer and it require
   // repainting that changes of CSS properties such as 'visibility',
   // request repainting.
-  bool needs_layout = diff.NeedsFullPaintInvalidation() &&
-                      IsLayoutObjectOfResourceContainer(layout_object);
+  needs_layout |= diff.NeedsFullPaintInvalidation() &&
+                  IsLayoutObjectOfResourceContainer(layout_object);
 
   LayoutSVGResourceContainer::MarkForLayoutAndParentResourceInvalidation(
       layout_object, needs_layout);

third_party/blink/renderer/core/layout/svg/svg_resources_cache.h

@@ -84,9 +84,17 @@ class SVGResourcesCache {
   };
 
  private:
-  bool AddResourcesFromLayoutObject(LayoutObject&, const ComputedStyle&);
+  struct ResourceUpdateInfo {
+    bool changed;
+    bool needs_layout;
+
+    explicit operator bool() const { return changed; }
+  };
+  SVGResources* AddResourcesFromLayoutObject(LayoutObject&,
+                                             const ComputedStyle&);
   bool RemoveResourcesFromLayoutObject(LayoutObject&);
-  bool UpdateResourcesFromLayoutObject(LayoutObject&, const ComputedStyle&);
+  ResourceUpdateInfo UpdateResourcesFromLayoutObject(LayoutObject&,
+                                                     const ComputedStyle&);
 
   typedef HashMap<const LayoutObject*, std::unique_ptr<SVGResources>> CacheMap;
   CacheMap cache_;

third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/clip-path-url-reference-change-from-empty.html

+<!DOCTYPE html>
+<html class="reftest-wait">
+<title>Switch from an empty to a non-empty clip-path url()</title>
+<link rel="help" href="http://www.w3.org/TR/css-masking-1/#clipping-paths">
+<link rel="help" href="http://www.w3.org/TR/css-masking-1/#the-clip-path">
+<link rel="match" href="reference/green-100x100.html">
+<script src="/common/reftest-wait.js"></script>
+<svg>
+  <clipPath id="empty"/>
+  <clipPath id="rect">
+    <rect width="100" height="100"/>
+  </clipPath>
+  <rect width="100" height="100" fill="red"/>
+  <rect width="100" height="100" fill="green" id="target" clip-path="url(#empty)"/>
+</svg>
+<script>
+  requestAnimationFrame(() => {
+    requestAnimationFrame(() => {
+      document.getElementById('target').setAttribute('clip-path', 'url(#rect)');
+      takeScreenshot();
+    });
+  });
+</script>

third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/clip-path-url-reference-change.html

+<!DOCTYPE html>
+<html class="reftest-wait">
+<title>Switch from one clip-path url() to another with the same bounds</title>
+<link rel="help" href="http://www.w3.org/TR/css-masking-1/#clipping-paths">
+<link rel="help" href="http://www.w3.org/TR/css-masking-1/#the-clip-path">
+<link rel="match" href="reference/green-100x100.html">
+<script src="/common/reftest-wait.js"></script>
+<svg>
+  <clipPath id="circle">
+    <circle cx="50" cy="50" r="50"/>
+  </clipPath>
+  <clipPath id="rect">
+    <rect width="100" height="100"/>
+  </clipPath>
+  <rect width="100" height="100" fill="red"/>
+  <rect width="100" height="100" fill="green" id="target" clip-path="url(#circle)"/>
+</svg>
+<script>
+  requestAnimationFrame(() => {
+    requestAnimationFrame(() => {
+      document.getElementById('target').setAttribute('clip-path', 'url(#rect)');
+      takeScreenshot();
+    });
+  });
+</script>

third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/reference/green-100x100.html

+<!DOCTYPE html>
+<div style="width: 100px; height: 100px; background-color: green"></div>