IntersectionObserver rootMargin detection fails when `root` is an element
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2019 13:05:13 +0000 (13:05 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2019 13:05:13 +0000 (13:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198784

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

Import https://github.com/web-platform-tests/wpt/pull/17323.

* web-platform-tests/intersection-observer/root-margin-root-element-expected.txt: Added.
* web-platform-tests/intersection-observer/root-margin-root-element.html: Added.

Source/WebCore:

When computing a target's bounds in root space, we were applying the root's
clip rect (if any), and then intersecting with the root rect expanded by the
root margin. This meant that if a target did not intersect the non-expanded root
rect, we would get an empty intersection even if the target did intersect the
expanded root rect. Fix this by not applying the root's clip rect when computing
a target's bounds in root space. Add a new VisibleRectContextOption::ApplyContainerClip
that determines whether RenderObject::computeVisibleRectInContainer should apply
the container's clip.

Test: imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html

* rendering/RenderBox.cpp:
(WebCore::RenderBox::applyCachedClipAndScrollPosition const):
* rendering/RenderObject.cpp:
(WebCore::RenderObject::visibleRectContextForRepaint):
* rendering/RenderObject.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246432 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element-expected.txt [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBox.cpp
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h

index daacf16..65f5c8c 100644 (file)
@@ -1,3 +1,15 @@
+2019-06-14  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserver rootMargin detection fails when `root` is an element
+        https://bugs.webkit.org/show_bug.cgi?id=198784
+
+        Reviewed by Simon Fraser.
+
+        Import https://github.com/web-platform-tests/wpt/pull/17323.
+
+        * web-platform-tests/intersection-observer/root-margin-root-element-expected.txt: Added.
+        * web-platform-tests/intersection-observer/root-margin-root-element.html: Added.
+
 2019-06-13  Youenn Fablet  <youenn@apple.com>
 
         Import WPT websockets tests
diff --git a/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element-expected.txt
new file mode 100644 (file)
index 0000000..360d7e0
--- /dev/null
@@ -0,0 +1,9 @@
+
+PASS Root margin with explicit root. 
+PASS First rAF 
+PASS document.scrollingElement.scrollTop = window.innerHeight. 
+PASS root.scrollTop = 50, putting target into root margin 
+PASS document.scrollingElement.scrollTop = 0. 
+PASS root.scrollTop = 0 
+PASS root.scrollTop = 50 with root scrolled out of view. 
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html b/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html
new file mode 100644 (file)
index 0000000..6016d45
--- /dev/null
@@ -0,0 +1,90 @@
+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="./resources/intersection-observer-test-utils.js"></script>
+
+<style>
+pre, #log {
+  position: absolute;
+  top: 0;
+  left: 200px;
+}
+.spacer {
+  height: calc(100vh + 100px);
+}
+#root {
+  display: inline-block;
+  overflow-y: scroll;
+  height: 200px;
+  border: 3px solid black;
+}
+#target {
+  width: 100px;
+  height: 100px;
+  background-color: green;
+}
+</style>
+
+<div class="spacer"></div>
+<div id="root">
+  <div style="height: 300px;"></div>
+  <div id="target"></div>
+</div>
+<div class="spacer"></div>
+
+<script>
+var vw = document.documentElement.clientWidth;
+var vh = document.documentElement.clientHeight;
+
+var entries = [];
+var root, target;
+
+runTestCycle(function() {
+  target = document.getElementById("target");
+  assert_true(!!target, "target exists");
+  root = document.getElementById("root");
+  assert_true(!!root, "root exists");
+  var observer = new IntersectionObserver(function(changes) {
+    entries = entries.concat(changes)
+  }, { root: root, rootMargin: "10px 20% 40% 30px" });
+  observer.observe(target);
+  entries = entries.concat(observer.takeRecords());
+  assert_equals(entries.length, 0, "No initial notifications.");
+  runTestCycle(step0, "First rAF");
+}, "Root margin with explicit root.");
+
+function step0() {
+  document.scrollingElement.scrollTop = vh;
+  runTestCycle(step1, "document.scrollingElement.scrollTop = window.innerHeight.");
+  checkLastEntry(entries, 0, [ 11, 111, vh + 411, vh + 511, 0, 0, 0, 0, -19, 131, vh + 101, vh + 391, false]);
+}
+
+function step1() {
+  root.scrollTop = 50;
+  runTestCycle(step2, "root.scrollTop = 50, putting target into root margin");
+  assert_equals(entries.length, 1, "No notifications after scrolling frame.");
+}
+
+function step2() {
+  document.scrollingElement.scrollTop = 0;
+  runTestCycle(step3, "document.scrollingElement.scrollTop = 0.");
+  checkLastEntry(entries, 1, [11, 111, 361, 461, 11, 111, 361, 391, -19, 131, 101, 391, true]);
+}
+
+function step3() {
+  root.scrollTop = 0;
+  runTestCycle(step4, "root.scrollTop = 0");
+  checkLastEntry(entries, 1);
+}
+
+function step4() {
+  root.scrollTop = 50;
+  runTestCycle(step5, "root.scrollTop = 50 with root scrolled out of view.");
+  checkLastEntry(entries, 2, [ 11, 111, vh + 411, vh + 511, 0, 0, 0, 0, -19, 131, vh + 101, vh + 391, false]);
+}
+
+// This tests that notifications are generated even when the root element is off screen.
+function step5() {
+  checkLastEntry(entries, 3, [11, 111, vh + 361, vh + 461, 11, 111, vh + 361, vh + 391, -19, 131, vh + 101, vh + 391, true]);
+}
+</script>
index 329a516..c8f594e 100644 (file)
@@ -1,3 +1,27 @@
+2019-06-14  Ali Juma  <ajuma@chromium.org>
+
+        IntersectionObserver rootMargin detection fails when `root` is an element
+        https://bugs.webkit.org/show_bug.cgi?id=198784
+
+        Reviewed by Simon Fraser.
+
+        When computing a target's bounds in root space, we were applying the root's
+        clip rect (if any), and then intersecting with the root rect expanded by the
+        root margin. This meant that if a target did not intersect the non-expanded root
+        rect, we would get an empty intersection even if the target did intersect the
+        expanded root rect. Fix this by not applying the root's clip rect when computing
+        a target's bounds in root space. Add a new VisibleRectContextOption::ApplyContainerClip
+        that determines whether RenderObject::computeVisibleRectInContainer should apply
+        the container's clip.
+
+        Test: imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html
+
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::applyCachedClipAndScrollPosition const):
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::visibleRectContextForRepaint):
+        * rendering/RenderObject.h:
+
 2019-06-14  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [cairo] Entering text into forms on github.com creates a trapezoid artifact
index c995bfd..c7ed223 100644 (file)
@@ -993,7 +993,8 @@ bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const RenderL
         rect.moveBy(-scrollPosition()); // For overflow:auto/scroll/hidden.
 
     // Do not clip scroll layer contents to reduce the number of repaints while scrolling.
-    if (!context.m_options.contains(VisibleRectContextOption::ApplyCompositedClips) && usesCompositedScrolling()) {
+    if ((!context.m_options.contains(VisibleRectContextOption::ApplyCompositedClips) && usesCompositedScrolling())
+        || (!context.m_options.contains(VisibleRectContextOption::ApplyContainerClip) && this == container)) {
         flipForWritingMode(rect);
         return true;
     }
index ed6a517..b9310df 100644 (file)
@@ -986,7 +986,7 @@ bool RenderObject::shouldApplyCompositedContainerScrollsForRepaint()
 
 RenderObject::VisibleRectContext RenderObject::visibleRectContextForRepaint()
 {
-    VisibleRectContext context;
+    VisibleRectContext context(false, false, { VisibleRectContextOption::ApplyContainerClip });
     if (shouldApplyCompositedContainerScrollsForRepaint())
         context.m_options.add(VisibleRectContextOption::ApplyCompositedContainerScrolls);
     return context;
index 09af0e3..04daa99 100644 (file)
@@ -668,6 +668,7 @@ public:
         UseEdgeInclusiveIntersection = 1 << 0,
         ApplyCompositedClips = 1 << 1,
         ApplyCompositedContainerScrolls  = 1 << 2,
+        ApplyContainerClip = 1 << 3,
     };
     struct VisibleRectContext {
         VisibleRectContext(bool hasPositionFixedDescendant = false, bool dirtyRectIsFlipped = false, OptionSet<VisibleRectContextOption> options = { })