From c6bcab8a5ca0b63b90d32a8865c4212b91a373ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 28 May 2020 10:38:51 +0000 Subject: [PATCH] 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 | 16 +++++++++++----- dom/base/DOMIntersectionObserver.h | 7 ++++--- .../initial-observation-with-threshold.html.ini | 4 ---- .../isIntersecting-threshold.html | 3 ++- 4 files changed, 17 insertions(+), 13 deletions(-) delete mode 100644 testing/web-platform/meta/intersection-observer/initial-observation-with-threshold.html.ini diff --git a/dom/base/DOMIntersectionObserver.cpp b/dom/base/DOMIntersectionObserver.cpp index dd9284e354c6..71372712401a 100644 --- a/dom/base/DOMIntersectionObserver.cpp +++ b/dom/base/DOMIntersectionObserver.cpp @@ -618,7 +618,9 @@ void DOMIntersectionObserver::Update(Document* aDocument, // 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) { @@ -628,18 +630,22 @@ void DOMIntersectionObserver::Update(Document* aDocument, // 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); } } } @@ -647,7 +653,7 @@ void DOMIntersectionObserver::Update(Document* aDocument, void DOMIntersectionObserver::QueueIntersectionObserverEntry( Element* aTarget, DOMHighResTimeStamp time, const Maybe& aRootRect, const nsRect& aTargetRect, const Maybe& aIntersectionRect, - double aIntersectionRatio) { + bool aIsIntersecting, double aIntersectionRatio) { RefPtr rootBounds; if (aRootRect.isSome()) { rootBounds = new DOMRect(this); @@ -661,7 +667,7 @@ void DOMIntersectionObserver::QueueIntersectionObserverEntry( } RefPtr entry = new DOMIntersectionObserverEntry( this, time, rootBounds.forget(), boundingClientRect.forget(), - intersectionRect.forget(), aIntersectionRect.isSome(), aTarget, + intersectionRect.forget(), aIsIntersecting, aTarget, aIntersectionRatio); mQueuedEntries.AppendElement(entry.forget()); } diff --git a/dom/base/DOMIntersectionObserver.h b/dom/base/DOMIntersectionObserver.h index f6e5b5b8b79c..f8dcd5c97cab 100644 --- a/dom/base/DOMIntersectionObserver.h +++ b/dom/base/DOMIntersectionObserver.h @@ -31,9 +31,9 @@ class DOMIntersectionObserverEntry final : public nsISupports, 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) {} @@ -138,6 +138,7 @@ class DOMIntersectionObserver final : public nsISupports, const Maybe& aRootRect, const nsRect& aTargetRect, const Maybe& aIntersectionRect, + bool aIsIntersecting, double aIntersectionRatio); nsCOMPtr mOwner; diff --git a/testing/web-platform/meta/intersection-observer/initial-observation-with-threshold.html.ini b/testing/web-platform/meta/intersection-observer/initial-observation-with-threshold.html.ini deleted file mode 100644 index a96ac726459c..000000000000 --- 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 - diff --git a/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html b/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html index 106b65edd752..842c8e2c9f96 100644 --- a/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html +++ b/testing/web-platform/tests/intersection-observer/isIntersecting-threshold.html @@ -43,7 +43,8 @@ function step3() { 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; }