InteractionRegions: guards should not interfere with other regions

https://bugs.webkit.org/show_bug.cgi?id=265200 <rdar://115893635> Reviewed by Tim Horton. When multiple small links are laid out in close proximity, the "guards" regions we generate end up hurting more than they help. At the end of the InteractionRegion generation process, filter out guards that overlap with other regions too much. * Source/WebCore/rendering/EventRegion.h: * Source/WebCore/rendering/EventRegion.cpp: (WebCore::EventRegionContext::copyInteractionRegionsToEventRegion): (WebCore::EventRegionContext::removeSuperfluousInteractionRegions): Extract the final `removeAllMatching` pass on InteractionRegions to its own method and add the guards filtering logic. A guard shouldn't overlap with any interaction too much except: - the interaction it is guarding - any container interaction that fully contains the guard itself. (WebCore::EventRegionContext::uniteInteractionRegions): Stop adding guards for multi-rect regions. They are easily targetable as a whole. * LayoutTests/interaction-region/wrapped-inline-link-expected.txt: Update expectations now that multi-rect regions don't get guards. * LayoutTests/interaction-region/guard-overlap-expected.txt: Added. * LayoutTests/interaction-region/guard-overlap.html: Added. Add a new test covering the guards filtering logic. Canonical link: https://commits.webkit.org/271269@main
WebKit · Nov 29, 2023 · 3991417 · 3991417
1 parent 7687d03
commit 3991417
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 19 deletions.
diff --git a/LayoutTests/interaction-region/guard-overlap-expected.txt b/LayoutTests/interaction-region/guard-overlap-expected.txt
@@ -0,0 +1,29 @@
+
+(GraphicsLayer
+  (anchor 0.00 0.00)
+  (bounds 800.00 600.00)
+  (children 1
+    (GraphicsLayer
+      (bounds 800.00 600.00)
+      (contentsOpaque 1)
+      (drawsContent 1)
+      (backgroundColor #FFFFFF)
+      (event region
+        (rect (0,0) width=800 height=600)
+
+      (interaction regions [
+        (interaction (0,0) width=100 height=10)
+        (borderRadius 0.00),
+        (interaction (0,10) width=50 height=10)
+        (borderRadius 0.00),
+        (interaction (0,40) width=100 height=25)
+        (borderRadius 0.00),
+        (guard (-10,55) width=30 height=30)
+        (borderRadius 0.00),
+        (interaction (0,65) width=10 height=10)
+        (borderRadius 0.00)])
+      )
+    )
+  )
+)
+
diff --git a/LayoutTests/interaction-region/guard-overlap.html b/LayoutTests/interaction-region/guard-overlap.html
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<style>
+    body { margin: 0; }
+
+    .interaction {
+        height: 10px;
+        background-color: green;
+
+        cursor: pointer;
+    }
+</style>
+<body>
+<div class="interaction" style="width: 100px" onclick="click()"></div>
+<div class="interaction" style="width: 50px" onclick="click()"></div>
+
+<br />
+
+<div class="interaction" style="height: 25px; width: 100px" onclick="click()"></div>
+<div class="interaction" style="width: 10px" onclick="click()"></div>
+
+<pre id="results"></pre>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+window.onload = function () {
+    if (window.internals)
+       results.textContent = internals.layerTreeAsText(document, internals.LAYER_TREE_INCLUDES_EVENT_REGION | internals.LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES);
+};
+</script>
+</body>
+</html>
diff --git a/LayoutTests/interaction-region/wrapped-inline-link-expected.txt b/LayoutTests/interaction-region/wrapped-inline-link-expected.txt
@@ -33,12 +33,8 @@ Line
         (borderRadius 0 8 0 0),
         (interaction (6,109) width=107 height=12)
         (borderRadius 0 0 8 0),
-        (guard (6,111) width=71 height=32)
-        (borderRadius 0.00),
         (interaction (6,121) width=71 height=12)
         (borderRadius 0 0 8 0),
-        (guard (6,123) width=38 height=32)
-        (borderRadius 0.00),
         (interaction (6,133) width=38 height=12)
         (borderRadius 0 0 8 8)])
       )

diff --git a/Source/WebCore/rendering/EventRegion.cpp b/Source/WebCore/rendering/EventRegion.cpp
@@ -139,15 +139,6 @@ void EventRegionContext::uniteInteractionRegions(const Region& region, RenderObj
 
             discoveredRegion.unite(tempRegion);
             m_discoveredRegionsByElement.set(interactionRegion->elementIdentifier, discoveredRegion);
-
-            auto occlusionRect = guardRectForRegionBounds(tempRegion.bounds());
-            if (occlusionRect) {
-                m_interactionRegions.append({
-                    InteractionRegion::Type::Guard,
-                    interactionRegion->elementIdentifier,
-                    occlusionRect.value()
-                });
-            }
 
             for (auto rect : tempRegion.rects()) {
                 m_interactionRegions.append({
@@ -159,17 +150,19 @@ void EventRegionContext::uniteInteractionRegions(const Region& region, RenderObj
                 });
             }
             return;
-        } else
-            m_discoveredRegionsByElement.add(interactionRegion->elementIdentifier, interactionRegion->rectInLayerCoordinates);
+        }
+
+        m_discoveredRegionsByElement.add(interactionRegion->elementIdentifier, interactionRegion->rectInLayerCoordinates);
 
-        auto occlusionRect = guardRectForRegionBounds(interactionRegion->rectInLayerCoordinates);
-        if (occlusionRect) {
+        auto guardRect = guardRectForRegionBounds(interactionRegion->rectInLayerCoordinates);
+        if (guardRect) {
             m_interactionRegions.append({
                 InteractionRegion::Type::Guard,
                 interactionRegion->elementIdentifier,
-                occlusionRect.value()
+                guardRect.value()
             });
         }
+
         m_interactionRegions.append(*interactionRegion);
     }
 }
@@ -298,12 +291,45 @@ void EventRegionContext::shrinkWrapInteractionRegions()
     }
 }
 
-void EventRegionContext::copyInteractionRegionsToEventRegion()
+void EventRegionContext::removeSuperfluousInteractionRegions()
 {
     m_interactionRegions.removeAllMatching([&] (auto& region) {
+        if (region.type == InteractionRegion::Type::Guard) {
+            for (auto& entry : m_discoveredRegionsByElement) {
+                // This is the element being guarded.
+                if (entry.key == region.elementIdentifier)
+                    continue;
+
+                auto guardRect = region.rectInLayerCoordinates;
+                auto interactionRegion = entry.value;
+
+                auto intersection = interactionRegion;
+                intersection.intersect(guardRect);
+                if (intersection.isEmpty())
+                    continue;
+
+                // This is an interactive container of the guarded region.
+                if (intersection.contains(guardRect))
+                    continue;
+
+                auto originalBounds = interactionRegion.bounds();
+                auto intersectionBounds = intersection.bounds();
+                bool tooMuchOverlap = originalBounds.width() / 2 < intersectionBounds.width()
+                    || originalBounds.height() / 2 < intersectionBounds.height();
+
+                if (tooMuchOverlap)
+                    return true;
+            }
+            return false;
+        }
+
         return m_containersToRemove.contains(region.elementIdentifier);
     });
+}
 
+void EventRegionContext::copyInteractionRegionsToEventRegion()
+{
+    removeSuperfluousInteractionRegions();
     shrinkWrapInteractionRegions();
     m_eventRegion.appendInteractionRegions(m_interactionRegions);
 }

diff --git a/Source/WebCore/rendering/EventRegion.h b/Source/WebCore/rendering/EventRegion.h
@@ -58,6 +58,7 @@ class EventRegionContext : public RegionContext {
 #if ENABLE(INTERACTION_REGIONS_IN_EVENT_REGION)
     void uniteInteractionRegions(const Region&, RenderObject&);
     bool shouldConsolidateInteractionRegion(IntRect, RenderObject&);
+    void removeSuperfluousInteractionRegions();
     void shrinkWrapInteractionRegions();
     void copyInteractionRegionsToEventRegion();
 #endif