Bug 1670327 - Fix more IntersectionObserver issues. r=hiro

Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166

dom/base/DOMIntersectionObserver.cpp

@@ -499,10 +499,9 @@ void DOMIntersectionObserver::Update(Document* aDocument,
   if (mRoot && mRoot->IsElement()) {
     if ((rootFrame = mRoot->AsElement()->GetPrimaryFrame())) {
       nsRect rootRectRelativeToRootFrame;
-      if (rootFrame->IsScrollFrame()) {
+      if (nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame)) {
         // 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.
@@ -532,9 +531,11 @@ 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->GetRootScrollFrame();
-        if (rootFrame) {
-          nsIScrollableFrame* scrollFrame = do_QueryFrame(rootFrame);
+        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()) {
           rootRect = scrollFrame->GetScrollPortRect();
         }
       }
@@ -563,33 +564,53 @@ 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;
-    if (targetFrame && rootFrame) {
+    nsRect rootBounds;
+
+    const bool canComputeIntersection = [&] {
+      if (!targetFrame || !rootFrame) {
+        return false;
+      }
+
       // 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.
-      if (mRoot && !nsLayoutUtils::IsProperAncestorFrameCrossDoc(rootFrame,
-                                                                 targetFrame)) {
-        continue;
+      //
+      // 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);
       }
 
       // 2.3. Let targetRect be a DOMRectReadOnly obtained by running the

dom/base/test/test_intersectionobservers.html

@@ -333,20 +333,6 @@
       });
 
 
-      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) {
 
@@ -929,25 +915,6 @@
 
     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,2 +1,4 @@
 [target-in-different-window.html]
-  disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1497420
+  bug: https://github.com/w3c/IntersectionObserver/issues/456
+  [IntersectionObserver with target in a different window.]
+    expected: FAIL

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

@@ -0,0 +1,27 @@
+<!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

@@ -0,0 +1,25 @@
+<!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,5 +1,9 @@
 <!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>