Bug 1611204 - Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange
☠☠ backed out by ce8ad8ef1a2c ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 28 May 2020 01:10:35 +0000
changeset 532670 0dba6079675375c8934301b57a284a0988804441
parent 532669 6e7282ed160be0587e8c874af9d92d4fb3b23207
child 532671 da0e27f7f17ae3ec188997ecc92688175ab1299b
push id117314
push userealvarez@mozilla.com
push dateThu, 28 May 2020 01:11:24 +0000
treeherderautoland@0dba60796753 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1611204
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1611204 - Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange Note that no browser matches the spec (see https://github.com/w3c/IntersectionObserver/issues/432), but that our behavior is reasonably close to them. So do this to match them. Differential Revision: https://phabricator.services.mozilla.com/D76603
dom/base/DOMIntersectionObserver.cpp
dom/base/DOMIntersectionObserver.h
testing/web-platform/meta/intersection-observer/initial-observation-with-threshold.html.ini
testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html
--- a/dom/base/DOMIntersectionObserver.cpp
+++ b/dom/base/DOMIntersectionObserver.cpp
@@ -613,60 +613,66 @@ void DOMIntersectionObserver::Update(Doc
       intersectionRatio = isIntersecting ? 1.0 : 0.0;
     }
 
     // 2.9 Let thresholdIndex be the index of the first entry in
     // observer.thresholds whose value is greater than intersectionRatio, or the
     // length of observer.thresholds if intersectionRatio is greater than or
     // equal to the last entry in observer.thresholds.
     int32_t thresholdIndex = -1;
-    // FIXME(emilio): Why the isIntersecting check?
+
+    // If not intersecting, we can just shortcut, as we know that the thresholds
+    // are always between 0 and 1.
     if (isIntersecting) {
       thresholdIndex = mThresholds.IndexOfFirstElementGt(intersectionRatio);
       if (thresholdIndex == 0) {
         // Per the spec, we should leave threshold at 0 and distinguish between
         // "less than all thresholds and intersecting" and "not intersecting"
         // (queuing observer entries as both cases come to pass). However,
         // neither Chrome nor the WPT tests expect this behavior, so treat these
         // two cases as one.
         //
-        // FIXME(emilio): Looks like a good candidate for a spec issue.
+        // See https://github.com/w3c/IntersectionObserver/issues/432 about
+        // this.
         thresholdIndex = -1;
       }
     }
 
     // Steps 2.10 - 2.15.
     if (target->UpdateIntersectionObservation(this, thresholdIndex)) {
+      // See https://github.com/w3c/IntersectionObserver/issues/432 about
+      // why we use thresholdIndex > 0 rather than isIntersecting for the
+      // entry's isIntersecting value.
       QueueIntersectionObserverEntry(
           target, time,
           origin == BrowsingContextOrigin::Similar ? Some(rootBounds)
                                                    : Nothing(),
-          targetRect, intersectionRect, intersectionRatio);
+          targetRect, intersectionRect, thresholdIndex > 0, intersectionRatio);
     }
   }
 }
 
 void DOMIntersectionObserver::QueueIntersectionObserverEntry(
     Element* aTarget, DOMHighResTimeStamp time, const Maybe<nsRect>& aRootRect,
     const nsRect& aTargetRect, const Maybe<nsRect>& aIntersectionRect,
-    double aIntersectionRatio) {
+    bool aIsIntersecting, double aIntersectionRatio) {
   RefPtr<DOMRect> rootBounds;
   if (aRootRect.isSome()) {
     rootBounds = new DOMRect(this);
     rootBounds->SetLayoutRect(aRootRect.value());
   }
   RefPtr<DOMRect> boundingClientRect = new DOMRect(this);
   boundingClientRect->SetLayoutRect(aTargetRect);
   RefPtr<DOMRect> intersectionRect = new DOMRect(this);
   if (aIntersectionRect.isSome()) {
     intersectionRect->SetLayoutRect(aIntersectionRect.value());
   }
   RefPtr<DOMIntersectionObserverEntry> entry = new DOMIntersectionObserverEntry(
       this, time, rootBounds.forget(), boundingClientRect.forget(),
-      intersectionRect.forget(), aIntersectionRect.isSome(), aTarget,
+      intersectionRect.forget(), aIsIntersecting, aTarget,
       aIntersectionRatio);
   mQueuedEntries.AppendElement(entry.forget());
 }
 
 void DOMIntersectionObserver::Notify() {
   if (!mQueuedEntries.Length()) {
     return;
   }
--- a/dom/base/DOMIntersectionObserver.h
+++ b/dom/base/DOMIntersectionObserver.h
@@ -26,19 +26,19 @@ class DOMIntersectionObserverEntry final
   DOMIntersectionObserverEntry(nsISupports* aOwner, DOMHighResTimeStamp aTime,
                                RefPtr<DOMRect> aRootBounds,
                                RefPtr<DOMRect> aBoundingClientRect,
                                RefPtr<DOMRect> aIntersectionRect,
                                bool aIsIntersecting, Element* aTarget,
                                double aIntersectionRatio)
       : mOwner(aOwner),
         mTime(aTime),
-        mRootBounds(aRootBounds),
-        mBoundingClientRect(aBoundingClientRect),
-        mIntersectionRect(aIntersectionRect),
+        mRootBounds(std::move(aRootBounds)),
+        mBoundingClientRect(std::move(aBoundingClientRect)),
+        mIntersectionRect(std::move(aIntersectionRect)),
         mIsIntersecting(aIsIntersecting),
         mTarget(aTarget),
         mIntersectionRatio(aIntersectionRatio) {}
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(DOMIntersectionObserverEntry)
 
   nsISupports* GetParentObject() const { return mOwner; }
 
@@ -133,16 +133,17 @@ class DOMIntersectionObserver final : pu
 
  protected:
   void Connect();
   void QueueIntersectionObserverEntry(Element* aTarget,
                                       DOMHighResTimeStamp time,
                                       const Maybe<nsRect>& aRootRect,
                                       const nsRect& aTargetRect,
                                       const Maybe<nsRect>& aIntersectionRect,
+                                      bool aIsIntersecting,
                                       double aIntersectionRatio);
 
   nsCOMPtr<nsPIDOMWindowInner> mOwner;
   RefPtr<Document> mDocument;
   Variant<RefPtr<dom::IntersectionCallback>, NativeCallback> mCallback;
   RefPtr<nsINode> mRoot;
   StyleRect<LengthPercentage> mRootMargin;
   nsTArray<double> mThresholds;
deleted file mode 100644
--- a/testing/web-platform/meta/intersection-observer/initial-observation-with-threshold.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[initial-observation-with-threshold.html]
-  [First rAF]
-    expected: FAIL
-
--- a/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html
+++ b/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html
@@ -28,17 +28,18 @@ window.onload = function() {
   observer.observe(target);
 };
 
 function step2() {
   runTestCycle(step3, "Scrolled to half way through target element");
 
   assert_equals(entries.length, 1);
   assert_equals(entries[0].intersectionRatio, 1);
-  assert_equals(entries[0].isIntersecting, true);
+  // See https://github.com/w3c/IntersectionObserver/issues/432
+  assert_equals(entries[0].isIntersecting, false);
   scroller.scrollTop = 50;
 }
 
 function step3() {
   runTestCycle(step4, "Scrolled to target element completely off screen");
 
   assert_equals(entries.length, 2);
   assert_true(entries[1].intersectionRatio >= 0.5 &&