Bug 1611204 - Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 28 May 2020 10:38:51 +0000
changeset 596502 867528d1d35bdec48758f8aa899b0491d7ef10e5
parent 596501 3e61fd04eac7893a5a98a874ec9bb20fe218d310
child 596503 f9f4bdf52d9b437a8a95484dd2563c4b45baabfd
push id13186
push userffxbld-merge
push dateMon, 01 Jun 2020 09:52:46 +0000
treeherdermozilla-beta@3e7c70a1e4a1 [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
@@ -38,16 +38,17 @@ function step2() {
 }
 
 function step3() {
   runTestCycle(step4, "Scrolled to target element completely off screen");
 
   assert_equals(entries.length, 2);
   assert_true(entries[1].intersectionRatio >= 0.5 &&
               entries[1].intersectionRatio < 1);
-  assert_equals(entries[1].isIntersecting, true);
+  // See https://github.com/w3c/IntersectionObserver/issues/432
+  assert_equals(entries[1].isIntersecting, false);
   scroller.scrollTop = 100;
 }
 
 function step4() {
   assert_equals(entries.length, 2);
 }
 </script>