Bug 1243846 (intersection-observer)

Implement Intersection Observer API

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
2 years ago
14 days ago

People

(Reporter: cpeterson, Assigned: tobytailor)

Tracking

(Depends on: 7 bugs, Blocks: 1 bug, {dev-doc-needed, feature})

Trunk
mozilla55
dev-doc-needed, feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52 disabled, firefox53 disabled, firefox54 disabled, firefox55 fixed)

Details

(URL)

Attachments

(1 attachment, 32 obsolete attachments)

93.19 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
The proposed Intersection Observer API can be used to understand movement of DOM elements relative to another element or the browser top level viewport. Changes are delivered asynchronously and are useful for understanding the visibility of elements, managing pre-loading of DOM and data, as well as deferred loading of "below the fold" page content.

The web's traditional position calculation mechanisms rely on explicit queries of DOM state that are known to cause style recalculation and layout and, frequently, are redundant thanks to the requirement that scripts poll for this information.

https://github.com/WICG/IntersectionObserver
(I believe we're planning on building on top of the visibility-tracking work in bug 1157546 -- adding dependency.)
Depends on: 1157546
Assignee: nobody → seth
Depends on: 1259278
See Also: → bug 1232491
Keywords: dev-doc-needed
See Also: → bug 1272409
Seth, are you actively working on this/have an ETA? I'd like to be able to share our timeline with the Chrome team and other external partners as part of plugin work I'm doing.
Flags: needinfo?(seth)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Seth, are you actively working on this/have an ETA? I'd like to be able to
> share our timeline with the Chrome team and other external partners as part
> of plugin work I'm doing.

Firefox 50 is the current estimate.
Flags: needinfo?(seth)

Updated

a year ago
Target Milestone: --- → mozilla50
Hey Seth, is this something manual QA should be looking at? From Chris' summary in Comment 0, it seems this feature might affect a large spectrum of websites. 

If you think there's any value in testing it manually, please let us know how exactly we can help.
Flags: needinfo?(seth)
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #4)
> Hey Seth, is this something manual QA should be looking at? From Chris'
> summary in Comment 0, it seems this feature might affect a large spectrum of
> websites. 
> 
> If you think there's any value in testing it manually, please let us know
> how exactly we can help.

The feature hasn't shipped, so I think that's a bit premature. Manual QA would probably be useful in the future, though.
Flags: needinfo?(seth)
(Reporter)

Comment 6

a year ago
Tobias is working on Intersection Observer.
Assignee: mozilla.bugzilla → tschneider
Priority: -- → P1
(Reporter)

Comment 7

a year ago
Removing Target Milestone = 50 because we don't have a target release yet.
Target Milestone: mozilla50 → ---
(Assignee)

Comment 8

a year ago
Created attachment 8783670 [details] [diff] [review]
Part 1: DOM

First part of the implementation covers largest part of the DOM integration. There are still some memory leaks to be solved in a follow up patch. Tests are following.
(Assignee)

Updated

a year ago
Attachment #8783670 - Attachment description: bug1243846_1.diff → Part 1: DOM
(Assignee)

Comment 9

a year ago
Created attachment 8784528 [details] [diff] [review]
Part 1 (v2): DOM implementation

Small updates and memory leak fixes.
(Assignee)

Updated

a year ago
Attachment #8783670 - Attachment is obsolete: true
(Reporter)

Comment 10

a year ago
Comment on attachment 8784528 [details] [diff] [review]
Part 1 (v2): DOM implementation

Review of attachment 8784528 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +123,4 @@
>  #include "nsCSSParser.h"
>  #include "prprf.h"
>  #include "nsDOMMutationObserver.h"
> +#include "nsDOMIntersectionObserver.h"

nsDOMIntersectionObserver.h should be included alphabetically before nsDOMMutationObserver.h.

@@ +3869,5 @@
> +Element::RegisterIntersectionObserver(nsDOMIntersectionObserver* aObserver)
> +{
> +  IntersectionObserverRegistration reg;
> +  reg.observer = aObserver;
> +  reg.previousThreshold = 0.0;

Maybe move this initialization to a IntersectionObserverRegistration constructor that takes an observer parameter? And remove the default constructor with `= delete`.

@@ +3876,5 @@
> +
> +void
> +Element::UnregisterIntersectionObserver(nsDOMIntersectionObserver* aObserver)
> +{
> +  for (int32_t i = 0; i < mRegisteredIntersectionObservers.Length(); ++i) {

I'm surprised comparing signed int32_t i to Length() doesn't cause a -Wsign-compare warning-as-error with clang or gcc.

::: dom/base/nsDOMIntersectionObserver.cpp
@@ +22,5 @@
> +double
> +nsDOMIntersectionObserverEntry::IntersectionRatio()
> +{
> +	double targetArea = mBoundingClientRect.Width() * mBoundingClientRect.Height();
> +  double intersectionArea = mIntersectionRect.Width() * mIntersectionRect.Height();

Fix indentation.

@@ +59,5 @@
> +
> +already_AddRefed<nsDOMIntersectionObserver>
> +nsDOMIntersectionObserver::Constructor(const mozilla::dom::GlobalObject& aGlobal,
> +              						             mozilla::dom::IntersectionCallback& aCb,
> +              						             mozilla::ErrorResult& aRv)

Fix indentation. It looks like you are indenting with tabs instead of spaces in a few places in this file. :)

@@ +93,5 @@
> +    observer->mThresholds.SetCapacity(len);
> +    
> +    for (uint32_t i = 0; i < len; ++i) {
> +      double thresh = thresholds[i];
> +        

Remove trailing whitespace here and elsewhere.

@@ +167,5 @@
> +{
> +	for (int32_t i = 0; i < mObservationTargets.Count(); ++i) {
> +    mozilla::dom::Element* target = mObservationTargets.ObjectAt(i);
> +    target->UnregisterIntersectionObserver(this);
> +    mObservationTargets.RemoveElementAt(i);

I think you are skipping every other target in mObservationTargets because you are removing the current element RemoveElementAt(i) and then incrementing i. Should this for loop just be a `while (!mObservationTargets.IsEmpty())` loop? Also, would removing the targets in reverse order, instead of removing the head, avoid resizing or shifting of mObservationTargets in the loop?
(Assignee)

Comment 11

a year ago
Created attachment 8786089 [details] [diff] [review]
Part 1 (v3): DOM implementation

Addressed review comments and small fixes.
Attachment #8784528 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
Created attachment 8786090 [details] [diff] [review]
Part 2: Integration

First shot on the actual integration part. This already covers most use cases. Whats still missing is support for rootMargin.
(Assignee)

Updated

a year ago
Attachment #8786090 - Flags: review?(mstange)
(Assignee)

Updated

a year ago
Attachment #8786089 - Flags: review?(mrbkap)
Comment on attachment 8786089 [details] [diff] [review]
Part 1 (v3): DOM implementation

Review of attachment 8786089 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good overall. I can't really review the CSSParser parts, but I have a few comments on the DOM parts.

::: dom/base/Element.cpp
@@ +3870,5 @@
> +{
> +  IntersectionObserverRegistration reg;
> +  reg.observer = aObserver;
> +  reg.previousThreshold = 0;
> +  mRegisteredIntersectionObservers.AppendElement(reg);

So this would go through the DOMSlots to access the array. Also, I think this can be written as

  mRegisteredIntersectionObservers.AppendElement(IntersectionObserverRegistration { aObserver, 0 });

::: dom/base/Element.h
@@ +1400,5 @@
> +    nsDOMIntersectionObserver* observer;
> +    double previousThreshold;
> +  };
> +
> +  nsTArray<IntersectionObserverRegistration> mRegisteredIntersectionObservers;

I think it would be better to put this list on nsDOMSlots (and then access it through the slots as well) to avoid adding another nsTArray to every element whether or not it uses the intersection API.

::: dom/base/nsDOMIntersectionObserver.cpp
@@ +58,5 @@
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> +
> +already_AddRefed<nsDOMIntersectionObserver>
> +nsDOMIntersectionObserver::Constructor(const mozilla::dom::GlobalObject& aGlobal,
> +                                       mozilla::dom::IntersectionCallback& aCb,

This should be in mozilla::dom anyway, but in general, I'd prefer using declarations to avoid sprinkling mozilla::dom everywhere.

@@ +75,5 @@
> +  if (!window) {
> +    aRv.Throw(NS_ERROR_FAILURE);
> +    return nullptr;
> +  }
> +  MOZ_ASSERT(window->IsInnerWindow());

This assertion is vacuous.

@@ +91,5 @@
> +    const mozilla::dom::Sequence<double>& thresholds = aOptions.mThreshold.GetAsDoubleSequence();
> +    uint32_t len = thresholds.Length();
> +    observer->mThresholds.SetCapacity(len);
> +    
> +    for (uint32_t i = 0; i < len; ++i) {

It's up to you, but I like the new C++ iteration syntax:

for (const auto& thresh : thresholds) { ...

@@ +94,5 @@
> +    
> +    for (uint32_t i = 0; i < len; ++i) {
> +      double thresh = thresholds[i];
> +        
> +      if (thresh < 0.0 || thresh > 1.0) {

Do we have to deal with NaN here?

@@ +139,5 @@
> +
> +void
> +nsDOMIntersectionObserver::GetThresholds(nsTArray<double>& aRetVal)
> +{
> +  aRetVal.SetCapacity(mThresholds.Length());

Why bother calling SetCapacity if we're going to SwapElements anyway?

::: dom/base/nsDOMIntersectionObserver.h
@@ +12,5 @@
> +#include "nsTArray.h"
> +
> +class nsDOMIntersectionObserver;
> +
> +class nsDOMIntersectionObserverEntry final : public nsISupports,

The current style is to avoid the ns prefix (even in the file name, so I'd rename the files to DOMIntersectionObserver.{h,cpp}) and put the classes in the mozilla::dom namespace, so:

namespace mozilla {
namespace dom {

class DOMIntsersectionObserver;

class DOMIntersectionObserverEntry final ...

@@ +15,5 @@
> +
> +class nsDOMIntersectionObserverEntry final : public nsISupports,
> +                                             public nsWrapperCache
> +{
> +  virtual ~nsDOMIntersectionObserverEntry() {}

This doesn't actually have to be be virtual.

@@ +21,5 @@
> +public:
> +  nsDOMIntersectionObserverEntry(nsISupports* aOwner,
> +                                 RefPtr<mozilla::dom::DOMRect> aRootBounds,
> +                                 RefPtr<mozilla::dom::DOMRect> aBoundingClientRect,
> +                                 RefPtr<mozilla::dom::DOMRect> aIntersectionRect,

These should probably take raw pointers to avoid two extra refcounts per rect when we contruct this object.

@@ +126,5 @@
> +  void GetRootMargin(mozilla::dom::DOMString& aRetVal);
> +  void GetThresholds(nsTArray<double>& aRetVal);
> +  void Observe(mozilla::dom::Element& aTarget);
> +  void Unobserve(mozilla::dom::Element& aTarget);
> +  

Nit: trailing whitespace.
Attachment #8786089 - Flags: review?(mrbkap) → review-
Comment on attachment 8786089 [details] [diff] [review]
Part 1 (v3): DOM implementation

Review of attachment 8786089 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMIntersectionObserver.cpp
@@ +99,5 @@
> +        aRv.ThrowTypeError<dom::MSG_THRESHOLD_RANGE_ERROR>();
> +        return nullptr;
> +      }
> +        
> +      observer->mThresholds.InsertElementSorted(thresh);

Doing AppendElement during the iteration, and then one Sort() at the end, should be faster.

@@ +141,5 @@
> +nsDOMIntersectionObserver::GetThresholds(nsTArray<double>& aRetVal)
> +{
> +  aRetVal.SetCapacity(mThresholds.Length());
> +  aRetVal.SwapElements(mThresholds);
> +  mThresholds.Clear();

Is this actually what you want? If you read observer.thresholds, should observer.thresholds be empty the second time you read? I would have expected this method to be just "aRetVal = mThresholds;".

::: dom/base/nsDOMIntersectionObserver.h
@@ +140,5 @@
> +  RefPtr<mozilla::dom::IntersectionCallback>        mCallback;
> +  RefPtr<mozilla::dom::Element>                     mRoot;
> +  nsCSSRect                                         mRootMargin;
> +  nsTArray<double>                                  mThresholds;
> +  nsCOMArray<mozilla::dom::Element>                 mObservationTargets;

It seems like this could use nsCheapSet<nsPtrHashKey<Element>> or nsTHashTable<nsPtrHashKey<Element>>. The latter is easier to iterate over, but the former is more optimized for the zero-or-one-element case. I'd probably choose the latter since most pages have more than one ad. (Unless you want to add nice iterators to nsCheapSet...)
Comment on attachment 8786090 [details] [diff] [review]
Part 2: Integration

Review of attachment 8786090 [details] [diff] [review]:
-----------------------------------------------------------------

r- mostly because of the global vs document-local thing. Also I'm not sure how much should be on nsIDocument vs on nsDocument, maybe look through a few other methods on those interfaces to find a pattern.

::: dom/base/nsDOMIntersectionObserver.cpp
@@ +9,5 @@
>  #include "nsCSSPropertyID.h"
>  #include "nsIFrame.h"
>  
> +AutoTArray<RefPtr<nsDOMIntersectionObserver>, 4>*
> +  nsDOMIntersectionObserver::sAllIntersectionObservers = nullptr;

This should be stored on the nsDocument, not globally. Otherwise you'll need to skip over lots of intersection observers during the UpdateIntersectionObservations loop if you have multiple tabs open, or iframes with intersection observers.
Oh, and nsDOMIntersectionObserver::NotifyIntersectionObservers() currently notifies all intersection observers in all tabs in the same content process. It should only update those who are connected to the refresh driver that's ticking.

@@ +208,5 @@
> +      nsIFrame* targetFrame = target->GetPrimaryFrame();
> +      if (!targetFrame) {
> +        continue;
> +      }
> +      nsRect targetRect = targetFrame->GetVisualOverflowRect();

The spec says:

> Let intersectionRect be the result of running the getBoundingClientRect() algorithm on the target.

so you need to call nsLayoutUtils::GetAllInFlowRectsUnion here, the way Element::GetBoundingClientRect() does it.

GetOverflowRect would give you a rect that is too large if the element has a box-shadow, for example.

@@ +214,5 @@
> +        root->GetPrimaryFrame(), targetFrame, rootIntersectionRect);
> +      double targetArea = targetRect.width * targetRect.height;
> +      double intersectionArea = intersectionRect.width * intersectionRect.height;
> +      double intersectionRatio = targetArea > 0.0 ? intersectionArea / targetArea : 0.0;
> +      uint32_t threshold = 0;

size_t threshold

@@ +219,5 @@
> +      if (intersectionRatio > 0.0) {
> +        if (intersectionRatio == 1.0) {
> +          threshold = observer->mThresholds.Length();
> +        } else {
> +          for (uint32_t k = 0; k < observer->mThresholds.Length(); ++k) {

size_t k

@@ +247,5 @@
> +}
> +
> +nsRect
> +nsDOMIntersectionObserver::ComputeIntersection(nsIFrame* rootFrame, nsIFrame* aTargetFrame,
> +                                               nsRect& rootIntersectionRect)

Arguments should have the form aArgument. This needs to be fixed in many places in this patch.

@@ +252,5 @@
> +{
> +  nsIFrame* frame = aTargetFrame;
> +  nsRect rectRelativeToFrame = frame->GetVisualOverflowRect();
> +  nsIScrollableFrame* enclosingScrollableFrame =
> +    nsLayoutUtils::GetAsyncScrollableProperAncestorFrame(frame);

Actually you want all nsIScrollableFrames, not just async scrollable ones. You even want overflow:hidden ones since the spec wants overflow:hidden clips to be taken into account.

@@ +295,5 @@
> +  RefPtr<mozilla::dom::DOMRect> intersectionRect = new DOMRect(this);
> +  intersectionRect->SetLayoutRect(aIntersectionRect);
> +  RefPtr<nsDOMIntersectionObserverEntry> entry = new nsDOMIntersectionObserverEntry(
> +    this,
> +    (double)PR_Now() / PR_USEC_PER_MSEC,

Hmm, I can see two more places in our code base where we do "(double)PR_Now() / PR_USEC_PER_MSEC", but it feels wrong... where did you get this from?

@@ +315,5 @@
> +  mozilla::dom::Sequence<mozilla::OwningNonNull<nsDOMIntersectionObserverEntry>> entries;
> +  if (entries.SetCapacity(mQueuedEntries.Length(), mozilla::fallible)) {
> +    for (uint32_t i = 0; i < mQueuedEntries.Length(); ++i) {
> +      RefPtr<nsDOMIntersectionObserverEntry> next = mQueuedEntries[i];
> +      *entries.AppendElement(mozilla::fallible) = mQueuedEntries[i];

If you use mozilla::fallible, then you need to null check the result and can't blindly assign to it.

::: dom/base/nsDOMIntersectionObserver.h
@@ +154,3 @@
>  protected:
> +  nsRect ComputeIntersection(nsIFrame* rootFrame, nsIFrame* aTargetFrame,
> +                             nsRect& rootIntersectionRect);

rootIntersectionRect should be "const nsRect&"; you're not using it as an outparameter.

::: dom/base/nsDocument.cpp
@@ +13425,5 @@
> +
> +void
> +nsIDocument::UpdateIntersectionObservations()
> +{
> +  nsIPresShell* shell = GetShell();

Unused variable
Attachment #8786090 - Flags: review?(mstange) → review-
(Assignee)

Comment 16

11 months ago
Created attachment 8790162 [details] [diff] [review]
Implement Intersection Observer API

Addressed all review comments. After all changes I was not able to split the patches in a DOM and integration part. So this comes in a single patch now.
Attachment #8786089 - Attachment is obsolete: true
Attachment #8786090 - Attachment is obsolete: true
Attachment #8790162 - Flags: review?(mstange)
Attachment #8790162 - Flags: review?(mrbkap)
Comment on attachment 8790162 [details] [diff] [review]
Implement Intersection Observer API

Review of attachment 8790162 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good now. r+ with the requested changes.

::: dom/base/DOMIntersectionObserver.cpp
@@ +91,5 @@
> +
> +  if (aOptions.mThreshold.IsDoubleSequence()) {
> +    const mozilla::dom::Sequence<double>& thresholds = aOptions.mThreshold.GetAsDoubleSequence();
> +    uint32_t len = thresholds.Length();
> +    observer->mThresholds.SetCapacity(len);

This is the only place where you use the len variable, so you might as well use thresholds.Length() directly.

@@ +97,5 @@
> +      if (thresh < 0.0 || thresh > 1.0) {
> +        aRv.ThrowTypeError<dom::MSG_THRESHOLD_RANGE_ERROR>();
> +        return nullptr;
> +      }
> +        

end-of-line whitespace

@@ +202,5 @@
> +  Element *docRoot = aDocument->GetRootElement();
> +  Element *root = mRoot;
> +  if (!root) {
> +    root = docRoot;
> +  } else if(!nsContentUtils::ContentIsDescendantOf(docRoot, root)) {

missing space after if

@@ +253,5 @@
> +        }
> +      }
> +    }
> +    if (target->UpdateIntersectionObservation(this, threshold)) {
> +      QueueIntersectionObserverEntry(target, time, 

end-of-line whitespace

@@ +290,5 @@
> +    nsIFrame* enclosingScrollableFrameAsFrame =
> +      do_QueryFrame(enclosingScrollableFrame);
> +    MOZ_ASSERT(enclosingScrollableFrame);
> +
> +    nsRect intersectionRect = enclosingScrollableFrameAsFrame == aRootFrame ?

Add a TODO about missing support for clip-path clips?

@@ +346,5 @@
> +  mozilla::dom::Sequence<mozilla::OwningNonNull<DOMIntersectionObserverEntry>> entries;
> +  if (entries.SetCapacity(mQueuedEntries.Length(), mozilla::fallible)) {
> +    for (uint32_t i = 0; i < mQueuedEntries.Length(); ++i) {
> +      RefPtr<DOMIntersectionObserverEntry> next = mQueuedEntries[i];
> +      if (next != nullptr) {

This null check is not needed, "next" will always be non-null because you're not adding null elements to the array. The null check I was asking for was for the value returned by entries.AppendElement(mozilla::fallible), but now I see that it's not necessary - you allocate the capacity for the "entries" array first, and check that, so there's no need to check every single AppendElement.

::: dom/base/DOMIntersectionObserver.h
@@ +137,5 @@
> +  void GetRootMargin(mozilla::dom::DOMString& aRetVal);
> +  void GetThresholds(nsTArray<double>& aRetVal);
> +  void Observe(Element& aTarget);
> +  void Unobserve(Element& aTarget);
> +  

end-of-line whitespace

::: dom/base/Element.cpp
@@ +3881,5 @@
> +
> +void
> +Element::UnregisterIntersectionObserver(DOMIntersectionObserver* aObserver)
> +{
> +  nsTArray<Element::nsDOMSlots::IntersectionObserverRegistration>* observers =

Do you need "Element::" here? You're already inside Element.

@@ +3897,5 @@
> +{
> +  nsTArray<Element::nsDOMSlots::IntersectionObserverRegistration>* observers =
> +    RegisteredIntersectionObservers();
> +  for (uint32_t i = 0; i < observers->Length(); ++i) {
> +    Element::nsDOMSlots::IntersectionObserverRegistration reg = observers->ElementAt(i);

You can use range for loops here, and a reference to avoid copying the IOR:

for (auto& reg : observers) {

@@ +3899,5 @@
> +    RegisteredIntersectionObservers();
> +  for (uint32_t i = 0; i < observers->Length(); ++i) {
> +    Element::nsDOMSlots::IntersectionObserverRegistration reg = observers->ElementAt(i);
> +    if (reg.observer == aObserver) {
> +      if (reg.previousThreshold != aThreshold) {

These two ifs can be merged into one.

@@ +3909,5 @@
> +  return false;
> +}
> +
> +bool
> +Element::HasRegisteredIntersectionObservers()

Is this method being called anywhere? I don't see a caller. Can you remove it?

::: layout/base/nsRefreshDriver.cpp
@@ +1833,5 @@
> +  document->UpdateIntersectionObservations();
> +  if (aNowTime >= mNextNotifyIntersectionObserversTick) {
> +    mNextNotifyIntersectionObserversTick =
> +      aNowTime + mMinNotifyIntersectionObserversInterval;
> +    

end-of-line whitespace
Attachment #8790162 - Flags: review?(mstange) → review+

Comment 18

11 months ago
Comment on attachment 8790162 [details] [diff] [review]
Implement Intersection Observer API

Review of attachment 8790162 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with my question answered.

::: dom/base/DOMIntersectionObserver.cpp
@@ +93,5 @@
> +    const mozilla::dom::Sequence<double>& thresholds = aOptions.mThreshold.GetAsDoubleSequence();
> +    uint32_t len = thresholds.Length();
> +    observer->mThresholds.SetCapacity(len);
> +    for (const auto& thresh : thresholds) {
> +      if (thresh < 0.0 || thresh > 1.0) {

I didn't see an answer to my question about NaN here and below.
Attachment #8790162 - Flags: review?(mrbkap) → review+
Depends on: 1302270
(Reporter)

Updated

11 months ago
Blocks: 1302270
No longer depends on: 1302270

Updated

11 months ago
No longer depends on: 1259278
(Assignee)

Comment 19

11 months ago
Created attachment 8793462 [details] [diff] [review]
Implement Intersection Observer API (v2)

Finally, review comments addressed and a bunch of fixes made (found by running Chrome's intersection observer tests):

Removed ThrowSyntaxError and use ThrowDomException instead.
Properly register/deregister observers to the documents.
Fixed root intersection calculation.
Fixed wrong basis used in ComputeCBDependentValue.
Use ContentIsCrossDocDescendantOf instead of ContentIsDescentendOf.
Simplyfied intersection computation.
Fixed error when looking for passed thresholds (this was implemented wrong due to a bug in the spec).
Make sure we send a not-intersecting notification (with all Rects empty) if previously intersecting target gets removed/becomes invisible.
Attachment #8790162 - Attachment is obsolete: true
Attachment #8793462 - Flags: review?(mstange)
(In reply to Tobias Schneider [:tobytailor] from comment #19)
> Fixed error when looking for passed thresholds (this was implemented wrong
> due to a bug in the spec).

Is the spec being fixed?
(Assignee)

Comment 21

11 months ago
I filed an issue for it: https://github.com/WICG/IntersectionObserver/issues/156. Me and dholbert both verified that the spec is wrong here.
Thanks!
Comment on attachment 8793462 [details] [diff] [review]
Implement Intersection Observer API (v2)

Review of attachment 8793462 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +249,5 @@
> +  rootIntersectionRect.Inflate(margin);
> +  for (auto iter = mObservationTargets.Iter(); !iter.Done(); iter.Next()) {
> +    Element* target = iter.Get()->GetKey();
> +    nsIFrame* targetFrame = target->GetPrimaryFrame();
> +    

end-of-line whitespace

@@ +250,5 @@
> +  for (auto iter = mObservationTargets.Iter(); !iter.Done(); iter.Next()) {
> +    Element* target = iter.Get()->GetKey();
> +    nsIFrame* targetFrame = target->GetPrimaryFrame();
> +    
> +    if (!(targetFrame && nsContentUtils::ContentIsCrossDocDescendantOf(target, root))) {

Could you use nsLayouUtils::IsAncestorFrameCrossDoc(rootFrame, targetFrame) here, and would that address the TODO about the containing block chain? I'm somewhat guessing here, so you'd definitely need to test this.

@@ +265,5 @@
> +        nsLayoutUtils::RECTS_ACCOUNT_FOR_TRANSFORMS);
> +
> +    nsRect intersectionRect = targetRect;
> +
> +    // TODO: Apply clips-paths. Also should the intersection be edge inclusive?

> Also should the intersection be edge inclusive?

I don't think so - do you have an example where it would make sense?

@@ +279,5 @@
> +        threshold = mThresholds.Length();
> +      } else {
> +        for (size_t k = 0; k < mThresholds.Length(); ++k) {
> +          if (mThresholds[k] < intersectionRatio) {
> +            threshold = k + 1;

I'm just going to trust you (and the tests) on this. I haven't actually thought through this very thoroughly.

::: dom/base/DOMIntersectionObserver.h
@@ +153,5 @@
> +
> +protected:
> +  void Connect();
> +  nsRect ComputeIntersection(nsIFrame* aRootFrame, nsIFrame* aTargetFrame,
> +                             const nsRect& aRootIntersectionRect);

This method no longer exists.
Attachment #8793462 - Flags: review?(mstange) → review+
(Assignee)

Comment 24

11 months ago
Created attachment 8794462 [details] [diff] [review]
Implement Intersection Observer API (v3)

Previous review comments addressed plus more small changes to make all tests pass.
Attachment #8793462 - Attachment is obsolete: true
Attachment #8794462 - Flags: review?(mstange)
(Assignee)

Comment 25

11 months ago
Created attachment 8794463 [details] [diff] [review]
Tests
Comment on attachment 8794462 [details] [diff] [review]
Implement Intersection Observer API (v3)

Review of attachment 8794462 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +130,5 @@
> +  mRootMargin = value.GetRectValue();
> +
> +  for (uint32_t i = 0; i < ArrayLength(nsCSSRect::sides); ++i) {
> +    nsCSSValue value = mRootMargin.*nsCSSRect::sides[i];
> +    if (!(value.IsPixelLengthUnit() || value.IsPercentLengthUnit())) {

I preferred the previous version, to be honest. I find fewer parentheses easier to understand.

@@ +228,5 @@
> +
> +static bool
> +HasIntersection(nsRect& rect)
> +{
> +  return rect.x > 0 || rect.y > 0 || rect.width > 0 || rect.height > 0;

Why is this necessary? Is it needed to pass tests?
Usually we say that two rectangles don't intersect if their intersection is zero width or zero height.

If we really need to this, please give me an example, and maybe also put the example into a comment in the code.

@@ +286,5 @@
> +    nsRect intersectionRect;
> +
> +    if (rootFrame && targetFrame &&
> +       (!mRoot || nsLayoutUtils::IsAncestorFrameCrossDoc(rootFrame, targetFrame))) {
> +      nsIFrame* relativeTo = nsLayoutUtils::GetContainingBlockForClientRect(targetFrame);

Why not make it relative to targetFrame? And then, as you walk up the frame tree, make it relative to containerFrame in each iteration.

@@ +305,5 @@
> +            break;
> +          }
> +        }
> +
> +        // TODO: Apply clips-path.

clip-path

::: dom/base/DOMIntersectionObserver.h
@@ +94,5 @@
> +
> +class DOMIntersectionObserver final : public nsISupports,
> +                                        public nsWrapperCache
> +{
> +  virtual ~DOMIntersectionObserver() { }

Why did you choose to remove the call to document->RemoveIntersectionObserver(this) that was in the previous patch?
Should we have an assertion here that checks that this observer is no longer registered?
Attachment #8794462 - Flags: review?(mstange)
(Assignee)

Comment 27

11 months ago
> @@ +228,5 @@
> > +
> > +static bool
> > +HasIntersection(nsRect& rect)
> > +{
> > +  return rect.x > 0 || rect.y > 0 || rect.width > 0 || rect.height > 0;
> 
> Why is this necessary? Is it needed to pass tests?
> Usually we say that two rectangles don't intersect if their intersection is
> zero width or zero height.
> 
> If we really need to this, please give me an example, and maybe also put the
> example into a comment in the code.

Chrome implements the intersection to be edge-inclusive (zero size but x or y > 0) and supports zero-size targets within the root coordinate space. For this reason everything that is not an all zero rectangle (positions and size being 0) counts as an intersection. This will probably also change in the spec (see https://github.com/WICG/IntersectionObserver/issues/69). BaseRect::Intersect leafs the position untouched which doesn't let us differentiate between a non- or zero-size-intersection.

> @@ +286,5 @@
> > +    nsRect intersectionRect;
> > +
> > +    if (rootFrame && targetFrame &&
> > +       (!mRoot || nsLayoutUtils::IsAncestorFrameCrossDoc(rootFrame, targetFrame))) {
> > +      nsIFrame* relativeTo = nsLayoutUtils::GetContainingBlockForClientRect(targetFrame);
> 
> Why not make it relative to targetFrame? And then, as you walk up the frame
> tree, make it relative to containerFrame in each iteration.

In this case we would have to make it relative to nsLayoutUtils::GetContainingBlockForClientRect(targetFrame) again after doing the final intersection with rootIntersectionRect, since intersectionTarget/targetRect and rootIntersectionRect can be in different coordinate spaces, e.g. of target and root are in different (i)frames.

> ::: dom/base/DOMIntersectionObserver.h
> @@ +94,5 @@
> > +
> > +class DOMIntersectionObserver final : public nsISupports,
> > +                                        public nsWrapperCache
> > +{
> > +  virtual ~DOMIntersectionObserver() { }
> 
> Why did you choose to remove the call to
> document->RemoveIntersectionObserver(this) that was in the previous patch?
> Should we have an assertion here that checks that this observer is no longer
> registered?

Spec says that the observer stays alive as long as its observing a target. As long as its doing this, there is always a reference to it from a document and deregistering in the deconstructor should never be necessary. Now that I think about it tho, do we have to make the pointers in nsDocument::mIntersectionObservers nsRefPtrs, to make sure we keep the observer alive during the documents lifetime?
(Assignee)

Comment 28

11 months ago
Created attachment 8795353 [details] [diff] [review]
Implement Intersection Observer API (v4)

Rebased patch to mc.
Attachment #8794462 - Attachment is obsolete: true
(Assignee)

Comment 29

11 months ago
Created attachment 8795354 [details] [diff] [review]
Tests (v2)

Merged Markus' additions, replaced flaky timeouts with RAF based callbacks.
Attachment #8794463 - Attachment is obsolete: true
(Assignee)

Comment 30

11 months ago
Created attachment 8795405 [details] [diff] [review]
Implement Intersection Observer API (v5)

Fixed compiling error due to unused variable.
Attachment #8795353 - Attachment is obsolete: true
(Assignee)

Comment 31

11 months ago
Created attachment 8795449 [details] [diff] [review]
Implement Intersection Observer API (v6)

Rebased again. Reverted unrelated white-space changes.
Attachment #8795405 - Attachment is obsolete: true
(Assignee)

Comment 32

11 months ago
Created attachment 8796634 [details] [diff] [review]
Implement Intersection Observer API (v7)

Fixed memory leaks.
Attachment #8795449 - Attachment is obsolete: true
(Assignee)

Comment 33

11 months ago
Created attachment 8796636 [details] [diff] [review]
Implement Intersection Observer API (v7)

Sorry, uploaded wrong patch earlier.
Attachment #8796634 - Attachment is obsolete: true
Comment on attachment 8796636 [details] [diff] [review]
Implement Intersection Observer API (v7)

Review of attachment 8796636 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +214,5 @@
> +  mQueuedEntries.Clear();
> +}
> +
> +static nsRect
> +Intersect(nsRect& aRect, nsRect& aOtherRect)

Let's make this:

static Maybe<nsRect>
EdgeInclusiveIntersection(const nsRect& aRect, const nsRect& aOtherRect)
{
  nscoord left = std::max(aRect.x, aOtherRect.x);
  nscoord top = std::max(aRect.y, aOtherRect.y);
  nscoord right = std::min(aRect.XMost(), aOtherRect.XMost());
  nscoord bottom = std::min(aRect.YMost(), aOtherRect.YMost());
  if (left > right || top > bottom) {
    return Nothing();
  }
  return Some(nsRect(left, top, right - left, bottom - top));
}

Your current solution would treat intersections at 0,0 (or to the top or left of 0,0) incorrectly, I think.
The spec says that rootMargin and rootBounds should only be available if the target is in the same unit of related similar-origin browsing contexts as the intersection root. Does this patch implement that restriction?
(Assignee)

Comment 36

11 months ago
It does not. How does a check for that case look like?
I talked to :mystor about this today. He thinks it's a little silly that the spec chose the term "in the same unit of related similar-origin browsing contexts" instead of just "same origin", because "same origin" would be simpler to check for that "similar origin", and accounting for document.domain changes does not seem worth doing.

Michael says the similar-origin would be something like this:

static bool
AreNodesInSimilarOriginBrowsingContext(nsINode* aNode1, nsINode* aNode2)
{
  nsIPrincipal* principal1 = aNode1->NodePrincipal();
  nsIPrincipal* principal2 = aNode2->NodePrincipal();

  if one of the principals does not have a URL: {
    return principal1 == principal2;
  }

  Otherwise, get the URLs, and then the base domains of the URLs using
  nsIEffectiveTLDService::getBaseDomain with aAdditionalParts set to 0, and then
  string compare those base domains.
}

The target and the intersection root will always be in "related" browsing contexts because they'll always be in the same tab, so we don't have to check that part.
Apologies for the typos above.

(In reply to Markus Stange [:mstange] from comment #37)
> because "same origin" would be simpler to check for that "similar origin"

because "same origin" would be simpler to check for *than* "similar origin"

> Michael says the similar-origin would be something like this:

Michael says the similar-origin *check* would be something like this:
(Assignee)

Comment 39

11 months ago
Created attachment 8797332 [details] [diff] [review]
Use EdgeInclusiveIntersection

Uses Markus' recommended EdgeInclusiveIntersection after testing.
Attachment #8797332 - Flags: review?(mstange)
(Assignee)

Comment 40

11 months ago
Created attachment 8797333 [details] [diff] [review]
Make intersection computations spec conform

Addresses reviews re coordinate space transformation and handles cross-origins rootMargin and rootBounds spec conform.
Attachment #8797333 - Flags: review?(mstange)
(Assignee)

Comment 41

11 months ago
Created attachment 8797708 [details] [diff] [review]
Make intersection computations spec conform (v2)

Added missing CheckSimilarOrigin function.
Attachment #8797333 - Attachment is obsolete: true
Attachment #8797333 - Flags: review?(mstange)
Attachment #8797708 - Flags: review?(mstange)
Comment on attachment 8797332 [details] [diff] [review]
Use EdgeInclusiveIntersection

Review of attachment 8797332 [details] [diff] [review]:
-----------------------------------------------------------------

I usually treat Maybes like pointers, and rarely call isSome / isNothing / value.

It would be nice to have a test that this fixes the 0,0,0,0 case as well, e.g. by adding a -20,-20,20,20 case to the -21,20 / -20,-21 tests.

::: dom/base/DOMIntersectionObserver.cpp
@@ +295,5 @@
>              containerFrame->GetContentRectRelativeToSelf(),
>              relativeTo);
> +          intersectionRect = EdgeInclusiveIntersection(intersectionRect.value(),
> +                                                       subFrameRect);
> +          if (intersectionRect.isNothing()) {

Could just be "if (!intersectionRect) {"

@@ +314,3 @@
>      double targetArea = targetRect.width * targetRect.height;
> +    double intersectionArea = intersectionRect.isNothing() ?
> +      0 : intersectionRect.value().width * intersectionRect.value().height;

intersectionRect->width * intersectionRect->height
Attachment #8797332 - Flags: review?(mstange) → review+
(Assignee)

Comment 43

11 months ago
Comment on attachment 8796636 [details] [diff] [review]
Implement Intersection Observer API (v7)

Blake, as mentioned please have a look at the cycle collection changes.
Attachment #8796636 - Flags: review?(mrbkap)
(Assignee)

Comment 44

11 months ago
Created attachment 8798142 [details] [diff] [review]
Make intersection computations spec conform (v3)

Adds more context, uses actual implicit root.
Attachment #8797708 - Attachment is obsolete: true
Attachment #8797708 - Flags: review?(mstange)
Attachment #8798142 - Flags: review?(mstange)
(Assignee)

Comment 45

11 months ago
Created attachment 8798170 [details] [diff] [review]
Use EdgeInclusiveIntersection (v2) r=mstange

Addressed review comments.
Attachment #8797332 - Attachment is obsolete: true
(Assignee)

Comment 46

11 months ago
Created attachment 8798171 [details] [diff] [review]
Make intersection computations spec conform (v3)

Previous patch was missing something.
Attachment #8798142 - Attachment is obsolete: true
Attachment #8798142 - Flags: review?(mstange)
Attachment #8798171 - Flags: review?(mstange)
(Assignee)

Comment 47

11 months ago
Created attachment 8798182 [details] [diff] [review]
Make intersection computations spec conform (v3)

Hopefully the right patch this time ;).
Attachment #8798171 - Attachment is obsolete: true
Attachment #8798171 - Flags: review?(mstange)
Attachment #8798182 - Flags: review?(mstange)

Comment 48

11 months ago
Comment on attachment 8796636 [details] [diff] [review]
Implement Intersection Observer API (v7)

Review of attachment 8796636 [details] [diff] [review]:
-----------------------------------------------------------------

I think there's one missing UNLINK (which could maybe cause a leak?) but this looks good to me otherwise.

::: dom/base/DOMIntersectionObserver.h
@@ +92,5 @@
> +{ 0x8570a575, 0xe303, 0x4d18, \
> +  { 0xb6, 0xb1, 0x4d, 0x2b, 0x49, 0xd8, 0xef, 0x94 } }
> +
> +class DOMIntersectionObserver final : public nsISupports,
> +                                        public nsWrapperCache

Nit: indentation.

::: dom/base/Element.h
@@ +1382,4 @@
>    nsDOMTokenList* GetTokenList(nsIAtom* aAtom,
>                                 const DOMTokenListSupportedTokenArray aSupportedTokens = nullptr);
>  
> +  nsTArray<nsDOMSlots::IntersectionObserverRegistration>* RegisteredIntersectionObservers();

Style-wise, I think I prefer returning a reference to the nsTArray but I don't feel strongly about it and I don't know if there's a style-guide that overrides my personal taste, so feel free to leave this as-is if you want.

::: dom/base/nsDocument.cpp
@@ +1743,4 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOnDemandBuiltInUASheets)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPreloadingImages)
>  
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIntersectionObservers)

I see this TRAVERSE implementation, but no corresponding UNLINK implementation. I think you need one, right?
Attachment #8796636 - Flags: review?(mrbkap) → review+
Comment hidden (mozreview-request)
Comment on attachment 8798236 [details]
Bug 1243846 - Make intersection computations spec conform

Oops, sorry about that, I meant to push a different patch for a different bug.
Attachment #8798236 - Attachment is obsolete: true
(Assignee)

Comment 51

11 months ago
Created attachment 8798617 [details] [diff] [review]
Tests (v3)

Added tests for edge-inclusive intersections, NaN threshold values and same/cross origin observations.
Attachment #8795354 - Attachment is obsolete: true
(Assignee)

Comment 52

11 months ago
Created attachment 8798618 [details] [diff] [review]
Make rootBounds nullable

Updated bindings to make rootBounds nullable.
Attachment #8798618 - Flags: review?(mstange)
Comment on attachment 8798182 [details] [diff] [review]
Make intersection computations spec conform (v3)

Review of attachment 8798182 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +275,5 @@
>      if (presShell) {
>        rootFrame = presShell->GetRootScrollFrame();
> +      while (!rootFrame->PresContext()->IsRootContentDocument()) {
> +        rootFrame = rootFrame->PresContext()->GetParentPresContext()
> +                             ->PresShell()->GetRootScrollFrame();

That's a lot of pointer hopping. Please walk the presContext chain first, and once you have the right presContext, get the presShell + root scroll frame from it.
Attachment #8798182 - Flags: review?(mstange) → review+
Comment on attachment 8798618 [details] [diff] [review]
Make rootBounds nullable

Review of attachment 8798618 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the RefPtr fix

::: dom/base/DOMIntersectionObserver.cpp
@@ +404,4 @@
>                                                          const nsRect& aTargetRect,
>                                                          const Maybe<nsRect>& aIntersectionRect)
>  {
> +  DOMRect* rootBounds = nullptr;

Why did you make them raw pointers? I think these should all be RefPtrs.
Attachment #8798618 - Flags: review?(mstange) → review+
Comment on attachment 8798182 [details] [diff] [review]
Make intersection computations spec conform (v3)

Review of attachment 8798182 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +260,3 @@
>      if (rootFrame) {
>        if (rootFrame->GetType() == nsGkAtoms::scrollFrame) {
>          nsIScrollableFrame *scrollFrame = do_QueryFrame(rootFrame);

* goes on the left

@@ +278,5 @@
> +        rootFrame = rootFrame->PresContext()->GetParentPresContext()
> +                             ->PresShell()->GetRootScrollFrame();
> +      }
> +      root = rootFrame->GetContent()->AsElement();
> +      nsIScrollableFrame *scrollFrame = do_QueryFrame(rootFrame);

* goes on the left, here too

@@ +317,5 @@
>  
>        nsIFrame* containerFrame = nsLayoutUtils::GetCrossDocParentFrame(targetFrame);
>        while (containerFrame && containerFrame != rootFrame) {
>          if (containerFrame->GetType() == nsGkAtoms::scrollFrame) {
> +          nsIScrollableFrame *scrollFrame = do_QueryFrame(containerFrame);

and here
(Assignee)

Comment 56

10 months ago
Created attachment 8800508 [details] [diff] [review]
Implement Intersection Observer API. r=mrbkap,mstange

Final patch to land.
Attachment #8796636 - Attachment is obsolete: true
Attachment #8798170 - Attachment is obsolete: true
Attachment #8798182 - Attachment is obsolete: true
Attachment #8798617 - Attachment is obsolete: true
Attachment #8798618 - Attachment is obsolete: true
(Assignee)

Comment 57

10 months ago
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2bc267450e659d87dbbf4aa592f288401b2daea

Comment 58

10 months ago
A "heads up" bug: 

Quoted from spec: "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."

repro: http://a1.alicdn.com/oss/uploads/2016/10/13/b0539560-9114-11e6-b30e-590985869f14.html

Comment 59

10 months ago
Here also a issue about `observer.thresholds` https://github.com/WICG/IntersectionObserver/issues/162
(Assignee)

Comment 60

10 months ago
Good find, thanks! We actually had the cross-doc check in a previous version of the patch, but got lost somehow. Will add it back and add a test.
(Assignee)

Comment 61

10 months ago
Created attachment 8800919 [details] [diff] [review]
Add Intersection Observer interfaces to  test_interfaces.html. r=mrbkap
(Assignee)

Comment 62

10 months ago
Created attachment 8800922 [details] [diff] [review]
Fix cross-doc+non-implicit root observations and and view port rect.

Fixes handling of cross-doc,non-implicit root observations and uses GetClientAreaRect instead of ScrollPortRect to get the view port rect.
Attachment #8800922 - Flags: review?(mstange)
Why is the scroll port rect not working? If you haven't found out, please do.
(Assignee)

Comment 64

10 months ago
I think I did, but was testing wrong. There is actually no issue with the implementation but with the tests. We were testing rootBounds against document.documentElement.clientWidth/Height. But since we run every mochitest in an iframe we have to make sure we use window.top.document.documentElement.clientWidth/Height instead.
(Assignee)

Comment 65

10 months ago
Created attachment 8801216 [details] [diff] [review]
Handle cross-doc,non-implicit root observations  + test fixes.
Attachment #8800922 - Attachment is obsolete: true
Attachment #8800922 - Flags: review?(mstange)
Attachment #8801216 - Flags: review?(mstange)
Comment on attachment 8801216 [details] [diff] [review]
Handle cross-doc,non-implicit root observations  + test fixes.

Review of attachment 8801216 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +307,5 @@
>      nsRect targetRect;
>      Maybe<nsRect> intersectionRect;
>  
> +    if (rootFrame && targetFrame) {
> +      if (mRoot && !(nsLayoutUtils::IsAncestorFrameCrossDoc(rootFrame, targetFrame) &&

This really needs a comment. I don't understand what it's doing.

::: dom/base/test/test_intersectionobservers.html
@@ +797,4 @@
>  
>        it('uses the viewport when no root is specified', function(done) {
>          io = new IntersectionObserver(function(records) {
> +          var rootDoc = window.top.document;

Allowing this test to run inside an iframe seems like it can easily break. Please move the test into a document that you open in a separate window.
Attachment #8801216 - Flags: review?(mstange)
(Assignee)

Comment 67

10 months ago
Created attachment 8801911 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v2)

Addressed review comments.
Attachment #8801216 - Attachment is obsolete: true
Attachment #8801911 - Flags: review?(mstange)
Comment on attachment 8801911 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v2)

Review of attachment 8801911 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +309,5 @@
>  
> +    if (rootFrame && targetFrame) {
> +      // Skip further processing for target if root is NOT the implicit root (mRoot set) AND
> +      // target is NOT a descendant of root in the containing block chain NOR in the same
> +      // document.

This is not a very useful comment. It's only describing what the code is already saying (except for the part that says "!mRoot means implicit root", I guess). It needs to describe *why* you're doing this.
Maybe list three or four examples for the different cases, with the expected scroll frame chain for each?

::: dom/base/test/intersectionobserver_iframe.html
@@ +14,5 @@
> +<body>
> +<div id="target5"></div>
> +<script>
> +        var io = new IntersectionObserver(function (records) {
> +                window.parent.postMessage(records[0].rootBounds == null, 'http://mochi.test:8888');

Does this stringify the boolean?

::: dom/base/test/test_intersectionobservers.html
@@ +165,4 @@
>      }
>    };
>  
> +  var ASYNC_TIMEOUT = 150;

Why this change? If the answer is "because it was necessary for fewer intermittent failures", then it means that there will still be intermittent failures.

@@ +798,5 @@
>  
>  
>        it('uses the viewport when no root is specified', function(done) {
> +        window.onmessage = function (e) {
> +          expect(e.data).to.be.ok();

Have you tested that this actually fails if the window posts false?
(Assignee)

Comment 69

10 months ago
(In reply to Markus Stange [:mstange] from comment #68)
> This is not a very useful comment. It's only describing what the code is
> already saying (except for the part that says "!mRoot means implicit root",
> I guess). It needs to describe *why* you're doing this.
> Maybe list three or four examples for the different cases, with the expected
> scroll frame chain for each?

Ait, will try to make this more clear.

> Does this stringify the boolean?

No, it gets serialized/deserialized internally.

> Why this change? If the answer is "because it was necessary for fewer
> intermittent failures", then it means that there will still be intermittent
> failures.

Actually yes. Any idea how else to avoid intermittent (timing) failures? Or just living with them for now?

> Have you tested that this actually fails if the window posts false?

Yes.
(Assignee)

Comment 70

10 months ago
Created attachment 8802288 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v3)

Made reasons for skipping processing of target more clear + tests.
Attachment #8801911 - Attachment is obsolete: true
Attachment #8801911 - Flags: review?(mstange)
Attachment #8802288 - Flags: review?(mstange)
(In reply to Tobias Schneider [:tobytailor] from comment #69)
> (In reply to Markus Stange [:mstange] from comment #68)
> Actually yes. Any idea how else to avoid intermittent (timing) failures? Or
> just living with them for now?

Living with them for now is not an option.
One way to avoid them would be to set a test-only pref that disables the time-based throttling, and then you'd change the test to only wait for exactly one animation frame. Or I guess you'd need your check to run right after the refresh driver ticked, so for example you could use executeSoon from the requestAnimationFrame callback so that you get the next available slot in the event loop.
Comment on attachment 8802288 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v3)

Review of attachment 8802288 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +313,5 @@
> +      if (mRoot) {
> +        // Skip further processing of this target if it is not in the same
> +        // Document as the intersection root, e.g. if root is an element of
> +        // the main document and target an element from an embedded iframe.
> +        if (!nsContentUtils::ContentIsDescendantOf(target, root)) {

Would target->GetComposedDoc() == root->GetComposedDoc() achieve the same? That would be cheaper to check, and match the comment better.
Does the spec define the behavior for what happens if either node is not in the node tree of a document? (E.g. an element you created with document.createElement without appending it to anything.) Do we do the right thing in that case?

@@ +318,5 @@
> +          continue;
> +        }
> +        // Skip further processing of this target if is not a descendant of the
> +        // intersection root in the containing block chain. E.g. this could be
> +        // the case if root is a static block (position: static) and doesn't

I'd say: "E.g. this would be the case if the target is in a position:absolute element whose containing block is an ancestor of root."

@@ +321,5 @@
> +        // intersection root in the containing block chain. E.g. this could be
> +        // the case if root is a static block (position: static) and doesn't
> +        // form a ancestor-descendant chain through the containing block
> +        // relation with target.
> +        if (!nsLayoutUtils::IsProperAncestorFrame(rootFrame, targetFrame)) {

Do you need "proper" here? What should happen if target == root?
(Assignee)

Comment 73

10 months ago
(In reply to Markus Stange [:mstange] from comment #72)
> Comment on attachment 8802288 [details] [diff] [review]
> Handle cross-doc,non-implicit root observations + test fixes. (v3)
> 
> Review of attachment 8802288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/DOMIntersectionObserver.cpp
> @@ +313,5 @@
> > +      if (mRoot) {
> > +        // Skip further processing of this target if it is not in the same
> > +        // Document as the intersection root, e.g. if root is an element of
> > +        // the main document and target an element from an embedded iframe.
> > +        if (!nsContentUtils::ContentIsDescendantOf(target, root)) {
> 
> Would target->GetComposedDoc() == root->GetComposedDoc() achieve the same?

Yes, changed it.

> That would be cheaper to check, and match the comment better.
> Does the spec define the behavior for what happens if either node is not in
> the node tree of a document? (E.g. an element you created with
> document.createElement without appending it to anything.) Do we do the right
> thing in that case?

Yes, we do. In this case the intersection rectangle would be empty (since target bounding client rect would be empty). In case there was a non-zero intersection before, we would still have to keep processing target to notify it about the non-intersection.

> @@ +318,5 @@
> > +          continue;
> > +        }
> > +        // Skip further processing of this target if is not a descendant of the
> > +        // intersection root in the containing block chain. E.g. this could be
> > +        // the case if root is a static block (position: static) and doesn't
> 
> I'd say: "E.g. this would be the case if the target is in a
> position:absolute element whose containing block is an ancestor of root."

Like it, will change.

> @@ +321,5 @@
> > +        // intersection root in the containing block chain. E.g. this could be
> > +        // the case if root is a static block (position: static) and doesn't
> > +        // form a ancestor-descendant chain through the containing block
> > +        // relation with target.
> > +        if (!nsLayoutUtils::IsProperAncestorFrame(rootFrame, targetFrame)) {
> 
> Do you need "proper" here? What should happen if target == root?

You're right, we don't, but there was no IsAncestorFrame :). Will add one.
(Assignee)

Comment 74

10 months ago
Created attachment 8803948 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v4)

Addressed latest review comments and made tests not relying on timing. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d049099ed12b53bf22c4746c98d6e7985b24983f
Attachment #8802288 - Attachment is obsolete: true
Attachment #8802288 - Flags: review?(mstange)
Attachment #8803948 - Flags: review?(mstange)
Comment on attachment 8803948 [details] [diff] [review]
Handle cross-doc,non-implicit root observations + test fixes. (v4)

Review of attachment 8803948 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good.
Attachment #8803948 - Flags: review?(mstange) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 76

10 months ago
does not apply cleanly

patching file dom/base/test/mochitest.ini
Hunk #1 FAILED at 235
1 out of 1 hunks FAILED -- saving rejects to file dom/base/test/mochitest.ini.rej
unable to find 'dom/base/test/test_intersectionobservers.html' for patching
29 out of 29 hunks FAILED -- saving rejects to file dom/base/test/test_intersectionobservers.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1243846.diff
Flags: needinfo?(tschneider)
Keywords: checkin-needed

Comment 77

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c55a20f1422b
Implement Intersection Observer API. r=mrbkap, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1185804088
Add Intersection Observer interfaces to test_interfaces.html. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/abaf54d32b12
Handle cross-doc,non-implicit root observations + test fixes. r=mstange
Flags: needinfo?(tschneider)
Backed out for failing modified test test_intersectionobservers.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1832a34a55f7ae0bd7e8e9a96dfd5837dca9329
https://hg.mozilla.org/integration/mozilla-inbound/rev/25a0cd1c3c3c6f4993f0e1d686612365fd64262b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5303fe44c50afa58881d54170e0d81f60bbc6f61

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38257903&repo=mozilla-inbound

[task 2016-10-26T16:30:03.260256Z] 16:30:03     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles root/target elements not yet in the DOM [observe] 
[task 2016-10-26T16:30:03.273071Z] 16:30:03     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles root/target elements not yet in the DOM [observe] 
[task 2016-10-26T16:30:03.276916Z] 16:30:03     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe] 
[task 2016-10-26T16:30:03.286804Z] 16:30:03     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe] 
[task 2016-10-26T16:30:03.289740Z] 16:30:03     INFO - TEST-PASS | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] 
[task 2016-10-26T16:30:03.292505Z] 16:30:03     INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe] - got 1.0016677174461075, expected 1
[task 2016-10-26T16:30:03.295006Z] 16:30:03     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:270:5
[task 2016-10-26T16:30:03.297275Z] 16:30:03     INFO -     get be/fn@dom/base/test/test_intersectionobservers.html:94:13
[task 2016-10-26T16:30:03.300917Z] 16:30:03     INFO -     io<@dom/base/test/test_intersectionobservers.html:873:11
[task 2016-10-26T16:30:03.303040Z] 16:30:03     INFO -     IntersectionCallback*@dom/base/test/test_intersectionobservers.html:871:14
[task 2016-10-26T16:30:03.305084Z] 16:30:03     INFO -     next@dom/base/test/test_intersectionobservers.html:144:9
[task 2016-10-26T16:30:03.307169Z] 16:30:03     INFO -     next/<@dom/base/test/test_intersectionobservers.html:146:11
[task 2016-10-26T16:30:03.311765Z] 16:30:03     INFO -     io<@dom/base/test/test_intersectionobservers.html:856:11
...
Flags: needinfo?(tschneider)
(Assignee)

Comment 79

10 months ago
Created attachment 8804994 [details] [diff] [review]
Implement Intersection Observer API.  r=mrbkap,mstange

Single patch to land. Rebased to m-c. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=366dd73f940e862a803e4104abe86fcb2c7c48f9
Attachment #8800508 - Attachment is obsolete: true
Attachment #8800919 - Attachment is obsolete: true
Attachment #8803948 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 80

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/111c1227f51e
Implement Intersection Observer API. r=mrbkap, r=mstange
Keywords: checkin-needed

Comment 81

10 months ago
I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38314573&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7d830ec82857d197cae4c23d9b1b4fc4cfca23
Flags: needinfo?(tschneider)
(Assignee)

Comment 82

10 months ago
Created attachment 8805607 [details] [diff] [review]
Implement Intersection Observer API. r=mrbkap,mstange

Keep intersectionRatio in range 0-1. Fixes intermittent tests failures. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e41b2bb974b2
Attachment #8804994 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 83

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e781308ebf
Implement Intersection Observer API. r=mrbkap, r=mstange
Keywords: checkin-needed
Depends on: 1313927

Comment 84

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72e781308ebf
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Comment 85

10 months ago
Release Note Request (optional, but appreciated)
[Why is this notable]: We're adding a new API and it will help developers move ad viewability checks from Flash to JavaScript.
[Affects Firefox for Android]: Yes
[Suggested wording]: Introduced IntersectionObserver API
[Links (documentation, blog post, etc)]: API documentation: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver
relnote-firefox: --- → ?

Updated

10 months ago
Depends on: 1314027

Updated

10 months ago
Depends on: 1314032

Updated

10 months ago
Depends on: 1314059

Updated

10 months ago
Depends on: 1314061

Comment 86

10 months ago
Update: 
https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver
https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
Depends on: 1315837
Depends on: 1313972
Depends on: 1313970

Updated

9 months ago
Depends on: 1316277
Depends on: 1317415
No longer depends on: 1317415
Depends on: 1317879
Is any manual testing needed for this feature?
Flags: needinfo?(cpeterson)
No longer depends on: 1317879
(Reporter)

Comment 88

9 months ago
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #87)
> Is any manual testing needed for this feature?

Not at this time. We don't know which websites in the wild use this feature yet. We're talking to some ad network partners about obtaining any tests they might have. In the meantime, we have some automated tests.
Flags: needinfo?(cpeterson)

Comment 89

9 months ago
Added to Fx52 Aurora release notes
relnote-firefox: ? → 52+
(Assignee)

Updated

9 months ago
Depends on: 1319134
(Assignee)

Updated

9 months ago
Depends on: 1319137
(Assignee)

Updated

9 months ago
Depends on: 1319139
(Assignee)

Updated

9 months ago
Depends on: 1319140

Updated

9 months ago
Depends on: 1320704
Depends on: 1317415
Updating status to reflect that this was backed out (temporarily) in:
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ae9b39276fa (52)
https://hg.mozilla.org/mozilla-central/rev/ef0db9c25541 (53)
in bug 1320704.
status-firefox51: --- → affected
status-firefox52: fixed → disabled
status-firefox53: --- → disabled
Sorry, meant "disabled" rather than "backed out".
Depends on: 1322717

Updated

8 months ago
Depends on: 1321865

Updated

8 months ago
Depends on: 1326194
This was re-enabled for Fx53 in bug 1321865. We should make sure the relnotes for 52/53 properly reflect the current status of things.
status-firefox47: affected → ---
status-firefox51: affected → wontfix
status-firefox53: disabled → fixed
relnote-firefox: 52+ → ?
Tentatively moved this entry from the 52 to the 53 release notes.  Still listed on https://developer.mozilla.org/en-US/Firefox/Releases/52 though, that needs to be updated.
relnote-firefox: ? → 53+
Flags: needinfo?(jypenator)
I've removed the note again and added it to the experimental features:
https://developer.mozilla.org/en-US/Firefox/Experimental_features#DOM

I didn't add it to the 53 notes yet, because bug 1321865 is still open (and we only have 4 days left until we're at 54).

I've also updated the compatibility on the following pages, accordingly:
https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver
https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/IntersectionObserver
https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry

The documentation needs to be updated again once bug 1321865 is fixed.

Sebastian

[1] https://developer.mozilla.org/en-US/Firefox/Experimental_features#DOM
Flags: needinfo?(jypenator)
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1332922

Updated

7 months ago
Depends on: 1335644
Thanks Sebastian.

Given that we have only partial documentation (not all properties pages created) I would like to keep the dev-doc-needed flag here, even if we will likely document these when bug 1321865 will land.
Keywords: dev-doc-complete → dev-doc-needed
(Assignee)

Updated

6 months ago
Depends on: 1337936
(Reporter)

Updated

6 months ago
Depends on: 1341259
No longer depends on: 1313972

Updated

4 months ago
relnote-firefox: 53+ → 54+
Move this to MDN.
relnote-firefox: 54+ → ---
(Assignee)

Updated

4 months ago
Depends on: 1359311
(Assignee)

Updated

4 months ago
Depends on: 1358666
(Assignee)

Updated

4 months ago
Depends on: 1358668
(Assignee)

Updated

4 months ago
Depends on: 1359316
(Assignee)

Updated

4 months ago
Depends on: 1359317
(Assignee)

Updated

4 months ago
Depends on: 1359318
(Assignee)

Updated

3 months ago
Depends on: 1363650
Depends on: 1366371
Depends on: 1369363
Depends on: 1365688
Depends on: 1369253
Depends on: 1369360
Depends on: 1369395
Depends on: 1374355

Updated

2 months ago
Depends on: 1377581

Updated

2 months ago
Depends on: 1377590
Should this bug be reopened since it keeps getting new open dependent bugs and the feature is not ready yet?
(Assignee)

Comment 98

a month ago
This bug turned into sort of a meta bug to keep track of IntersectionObserver dependencies and future work. Yes, we should either reopen this bug or create a new tracking bug for open dependencies.

Comment 99

a month ago
Reopening. Please mark fixed after all uplifts land in FF55.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

a month ago
status-firefox53: fixed → disabled
status-firefox54: --- → disabled
(Assignee)

Comment 100

a month ago
Waiting for Bugs 1359317 and 1363650 to be uplifted to FF55.
(Assignee)

Comment 101

a month ago
Mentioned bugs have been uplifted.
Status: REOPENED → RESOLVED
Last Resolved: 10 months agoa month ago
Resolution: --- → FIXED
(Assignee)

Comment 102

a month ago
I created a new bug (Bug 1381574) to keep track of future work on the Intersection Observer API. Please don't add any new dependencies to this bug.

Updated

a month ago
status-firefox55: --- → fixed
Target Milestone: mozilla52 → mozilla55
You need to log in before you can comment on or make changes to this bug.