Backed out changeset a083e04138aa (bug 1670327) for causing wpt failures in intersection-observer/root-margin.html

CLOSED TREE

dom/base/DOMIntersectionObserver.cpp

@@ -499,9 +499,10 @@ void DOMIntersectionObserver::Update(Document* aDocument,
   if (mRoot && mRoot->IsElement()) {
     if ((rootFrame = mRoot->AsElement()->GetPrimaryFrame())) {
       nsRect rootRectRelativeToRootFrame;
-      if (nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame)) {
+      if (rootFrame->IsScrollFrame()) {
         // rootRectRelativeToRootFrame should be the content rect of rootFrame,
         // not including the scrollbars.
+        nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame);
         rootRectRelativeToRootFrame = scrollFrame->GetScrollPortRect();
       } else {
         // rootRectRelativeToRootFrame should be the border rect of rootFrame.
@@ -531,11 +532,9 @@ void DOMIntersectionObserver::Update(Document* aDocument,
       // there's any OOP iframe in between `rootDocument` and `aDocument`, to
       // handle the OOP iframe positions.
       if (PresShell* presShell = rootDocument->GetPresShell()) {
-        rootFrame = presShell->GetRootFrame();
-        // We use the root scrollable frame's scroll port to account the
-        // scrollbars in rootRect, if needed.
-        if (nsIScrollableFrame* scrollFrame =
-                presShell->GetRootScrollFrameAsScrollable()) {
+        rootFrame = presShell->GetRootScrollFrame();
+        if (rootFrame) {
+          nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame);
           rootRect = scrollFrame->GetScrollPortRect();
         }
       }
@@ -564,53 +563,33 @@ void DOMIntersectionObserver::Update(Document* aDocument,
   // processed in the same order that observe() was called on each target:
   for (Element* target : mObservationTargets) {
     nsIFrame* targetFrame = target->GetPrimaryFrame();
+    // 2.2. If the intersection root is not the implicit root, and target is not
+    // in the same Document as the intersection root, skip further processing
+    // for target.
+    if (mRoot && mRoot->OwnerDoc() != target->OwnerDoc()) {
+      continue;
+    }
+
+    nsRect rootBounds;
+    if (rootFrame && targetFrame) {
+      // FIXME(emilio): Why only if there are frames?
+      rootBounds = rootRect;
+    }
+
     BrowsingContextOrigin origin = SimilarOrigin(*target, root);
+    if (origin == BrowsingContextOrigin::Similar) {
+      rootBounds.Inflate(rootMargin);
+    }
 
     Maybe<nsRect> intersectionRect;
     nsRect targetRect;
-    nsRect rootBounds;
-
-    const bool canComputeIntersection = [&] {
-      if (!targetFrame || !rootFrame) {
-        return false;
-      }
-
+    if (targetFrame && rootFrame) {
       // 2.1. If the intersection root is not the implicit root and target is
       // not a descendant of the intersection root in the containing block
       // chain, skip further processing for target.
-      //
-      // NOTE(emilio): We don't just "skip further processing" because that
-      // violates the invariant that there's at least one observation for a
-      // target (though that is also violated by 2.2), but it also causes
-      // different behavior when `target` is `display: none`, or not, which is
-      // really really odd, see:
-      // https://github.com/w3c/IntersectionObserver/issues/457
-      //
-      // NOTE(emilio): We also do this if target is the implicit root, pending
-      // clarification in
-      // https://github.com/w3c/IntersectionObserver/issues/456.
-      if (!nsLayoutUtils::IsAncestorFrameCrossDoc(rootFrame, targetFrame)) {
-        return false;
-      }
-
-      // 2.2. If the intersection root is not the implicit root, and target is
-      // not in the same Document as the intersection root, skip further
-      // processing for target.
-      //
-      // NOTE(emilio): We don't just "skip further processing", because that
-      // doesn't match reality and other browsers, see
-      // https://github.com/w3c/IntersectionObserver/issues/457.
-      if (mRoot && mRoot->OwnerDoc() != target->OwnerDoc()) {
-        return false;
-      }
-
-      return true;
-    }();
-
-    if (canComputeIntersection) {
-      rootBounds = rootRect;
-      if (origin == BrowsingContextOrigin::Similar) {
-        rootBounds.Inflate(rootMargin);
+      if (mRoot && !nsLayoutUtils::IsProperAncestorFrameCrossDoc(rootFrame,
+                                                                 targetFrame)) {
+        continue;
       }
 
       // 2.3. Let targetRect be a DOMRectReadOnly obtained by running the

dom/base/test/test_intersectionobservers.html

@@ -333,6 +333,20 @@ limitations under the License.
       });
 
 
+      it('does not trigger if target is not a descendant of the intersection root in the containing block chain',
+          function(done) {
+
+        var spy = sinon.spy();
+        io = new IntersectionObserver(spy, {root: parentEl});
+
+        parentEl.style.position = 'static';
+        io.observe(targetEl2);
+        callDelayed(function() {
+          expect(spy.callCount).to.be(0);
+          done();
+        });
+      });
+
       it('triggers if target or root becomes invisible',
           function(done) {
 
@@ -915,6 +929,25 @@ limitations under the License.
 
     describe('observe subframe', function () {
 
+      it('should not trigger if target and root are not in the same document',
+          function(done) {
+
+        var spy = sinon.spy();
+        io = new IntersectionObserver(spy, {root: rootEl});
+
+        targetEl4.onload = function () {
+          targetEl5 = targetEl4.contentDocument.getElementById('target5');
+          io.observe(targetEl5);
+          callDelayed(function() {
+            expect(spy.callCount).to.be(0);
+            done();
+          });
+        }
+
+        targetEl4.src = "intersectionobserver_iframe.html";
+
+      });
+
       it('boundingClientRect matches target.getBoundingClientRect() for an element inside an iframe',
           function(done) {
 

testing/web-platform/meta/intersection-observer/target-in-different-window.html.ini

@@ -1,4 +1,2 @@
 [target-in-different-window.html]
-  bug: https://github.com/w3c/IntersectionObserver/issues/456
-  [IntersectionObserver with target in a different window.]
-    expected: FAIL
+  disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1497420

testing/web-platform/tests/intersection-observer/explicit-root-different-document.tentative.html

@@ -1,27 +0,0 @@
-<!doctype html>
-<meta name="viewport" content="width=device-width,initial-scale=1">
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<link rel="help" href="https://github.com/w3c/IntersectionObserver/issues/457">
-<style>
-  div {
-    width: 100px;
-    height: 100px;
-    background: blue;
-    margin: 10px
-  }
-</style>
-<div id="root"></div>
-<script>
-let t = async_test("IntersectionObserver reports a (non-intersecting) entry if different-document from the doc");
-let doc = document.implementation.createHTMLDocument("");
-let target = doc.createElement("div");
-doc.body.appendChild(target);
-new IntersectionObserver(
-  t.step_func_done(function(records) {
-    assert_equals(records.length, 1);
-    assert_false(records[0].isIntersecting);
-  }),
-  { root: document.querySelector("#root") }
-).observe(target);
-</script>

testing/web-platform/tests/intersection-observer/not-in-containing-block-chain.tentative.html

@@ -1,25 +0,0 @@
-<!doctype html>
-<meta name="viewport" content="width=device-width,initial-scale=1">
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<link rel="help" href="https://github.com/w3c/IntersectionObserver/issues/457">
-<style>
-  div {
-    width: 100px;
-    height: 100px;
-    background: blue;
-    margin: 10px
-  }
-</style>
-<div id="target"></div>
-<div id="root"></div>
-<script>
-let t = async_test("IntersectionObserver reports a (non-intersecting) entry even if not in the containing block chain");
-new IntersectionObserver(
-  t.step_func_done(function(records) {
-    assert_equals(records.length, 1);
-    assert_false(records[0].isIntersecting);
-  }),
-  { root: document.querySelector("#root") }
-).observe(document.querySelector("#target"));
-</script>

testing/web-platform/tests/intersection-observer/target-in-different-window.html

@@ -1,9 +1,5 @@
 <!DOCTYPE html>
 <meta name="viewport" content="width=device-width,initial-scale=1">
-<!--
-  NOTE(emilio): This tests Chrome's behavior but it's not clear that's what the
-  spec asks for, see https://github.com/w3c/IntersectionObserver/issues/456
--->
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="./resources/intersection-observer-test-utils.js"></script>