Attachment 372075 Details for Bug 198784 – Patch

[patch] Patch

bug-198784-20190613160107.patch (text/plain), 8.58 KB, created by Ali Juma on 2019-06-13 13:01:08 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ali Juma

Created: 2019-06-13 13:01:08 PDT

Size: 8.58 KB

patch

obsolete

>Subversion Revision: 246367
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 40af09c66197ec572408367328cd3cbd6a7885a3..9eb92a8dc9231a78edddacfa3cf5decc7294607f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2019-06-13  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 NOBODY (OOPS!).
>+
>+        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-12  Antti Koivisto  <antti@apple.com>
> 
>         (Async scrolling) Handle 'position:fixed' inside 'position:sticky' correctly.
>diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
>index c71e73cf723475c2e28c5ce45c005cdc7d00910c..9190d2ae1e34e11af7c64d6f1e0e532094220044 100644
>--- a/Source/WebCore/rendering/RenderBox.cpp
>+++ b/Source/WebCore/rendering/RenderBox.cpp
>@@ -983,7 +983,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;
>     }
>diff --git a/Source/WebCore/rendering/RenderObject.cpp b/Source/WebCore/rendering/RenderObject.cpp
>index ed6a5179c76b1620e9d675e68b8366fa2b380c35..4da054af5e317f360708f0bc2ac526580f00ce75 100644
>--- a/Source/WebCore/rendering/RenderObject.cpp
>+++ b/Source/WebCore/rendering/RenderObject.cpp
>@@ -987,6 +987,7 @@ bool RenderObject::shouldApplyCompositedContainerScrollsForRepaint()
> RenderObject::VisibleRectContext RenderObject::visibleRectContextForRepaint()
> {
>     VisibleRectContext context;
>+    context.m_options.add(VisibleRectContextOption::ApplyContainerClip);
>     if (shouldApplyCompositedContainerScrollsForRepaint())
>         context.m_options.add(VisibleRectContextOption::ApplyCompositedContainerScrolls);
>     return context;
>diff --git a/Source/WebCore/rendering/RenderObject.h b/Source/WebCore/rendering/RenderObject.h
>index 09af0e333449ed0b52c631e2494fe0482e6e1151..04daa99bad83e951c469a365d6fc69a7d415e8f0 100644
>--- a/Source/WebCore/rendering/RenderObject.h
>+++ b/Source/WebCore/rendering/RenderObject.h
>@@ -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 = { })
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index 9534b3fc9baad1b67e62970c63fd6d7cab7b9270..3edaca0fa617a22e821ae9558e53af1dbddfb545 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-06-13  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 NOBODY (OOPS!).
>+
>+        * 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-12  Youenn Fablet  <youenn@apple.com>
> 
>         Update WPT service workers test up to 0df7c68
>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
>index 0000000000000000000000000000000000000000..360d7e09cab1f2078e7424de4b26721fa3f2d6a5
>--- /dev/null
>+++ b/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..6016d45bdf52ab8455ff20edc7da8fdcef3a24b9
>--- /dev/null
>+++ b/LayoutTests/imported/w3c/web-platform-tests/intersection-observer/root-margin-root-element.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 198784: 372075 | 372076 | 372078