1243846 - (intersection-observer-impl) Implement Intersection Observer API

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Daniel Holbert [:dholbert]]

(I believe we're planning on building on top of the visibility-tracking work in bug 1157546 -- adding dependency.)

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Jet Villegas (inactive)]

(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.

Comment 4

[Andrei Vaida [:avaida]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 6

[Chris Peterson [:cpeterson]]

Tobias is working on Intersection Observer.

Comment 7

[Chris Peterson [:cpeterson]]

Removing Target Milestone = 50 because we don't have a target release yet.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: 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.

Comment 9

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v2): DOM implementation

Small updates and memory leak fixes.

Comment 10

[Chris Peterson [:cpeterson]]

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?

Comment 11

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v3): DOM implementation

Addressed review comments and small fixes.

Comment 12

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Integration

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

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 14

[Markus Stange [:mstange]]

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 15

[Markus Stange [:mstange]]

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

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: 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.

Comment 17

[Markus Stange [:mstange]]

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

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Tobias Schneider [:tobytailor]]

Attachment: 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.

Comment 20

[Markus Stange [: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?

Comment 21

[Tobias Schneider [:tobytailor]]

I filed an issue for it: https://github.com/WICG/IntersectionObserver/issues/156. Me and dholbert both verified that the spec is wrong here.

Comment 22

[Markus Stange [:mstange]]

Thanks!

Comment 23

[Markus Stange [:mstange]]

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.

Comment 24

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v3)

Previous review comments addressed plus more small changes to make all tests pass.

Comment 25

[Tobias Schneider [:tobytailor]]

Attachment: Tests

Comment 26

[Markus Stange [:mstange]]

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?

Comment 27

[Tobias Schneider [:tobytailor]]

> @@ +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?

Comment 28

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v4)

Rebased patch to mc.

Comment 29

[Tobias Schneider [:tobytailor]]

Attachment: Tests (v2)

Merged Markus' additions, replaced flaky timeouts with RAF based callbacks.

Comment 30

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v5)

Fixed compiling error due to unused variable.

Comment 31

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v6)

Rebased again. Reverted unrelated white-space changes.

Comment 32

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v7)

Fixed memory leaks.

Comment 33

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API (v7)

Sorry, uploaded wrong patch earlier.

Comment 34

[Markus Stange [:mstange]]

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.

Comment 35

[Markus Stange [:mstange]]

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?

Comment 36

[Tobias Schneider [:tobytailor]]

It does not. How does a check for that case look like?

Comment 37

[Markus Stange [:mstange]]

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.

Comment 38

[Markus Stange [:mstange]]

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:

Comment 39

[Tobias Schneider [:tobytailor]]

Attachment: Use EdgeInclusiveIntersection

Uses Markus' recommended EdgeInclusiveIntersection after testing.

Comment 40

[Tobias Schneider [:tobytailor]]

Attachment: Make intersection computations spec conform

Addresses reviews re coordinate space transformation and handles cross-origins rootMargin and rootBounds spec conform.

Comment 41

[Tobias Schneider [:tobytailor]]

Attachment: Make intersection computations spec conform (v2)

Added missing CheckSimilarOrigin function.

Comment 42

[Markus Stange [: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

Comment 43

[Tobias Schneider [:tobytailor]]

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

Blake, as mentioned please have a look at the cycle collection changes.

Comment 44

[Tobias Schneider [:tobytailor]]

Attachment: Make intersection computations spec conform (v3)

Adds more context, uses actual implicit root.

Comment 45

[Tobias Schneider [:tobytailor]]

Attachment: Use EdgeInclusiveIntersection (v2) r=mstange

Addressed review comments.

Comment 46

[Tobias Schneider [:tobytailor]]

Attachment: Make intersection computations spec conform (v3)

Previous patch was missing something.

Comment 47

[Tobias Schneider [:tobytailor]]

Attachment: Make intersection computations spec conform (v3)

Hopefully the right patch this time ;).

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 49

[Markus Stange [:mstange]]

Attachment: Bug 1243846 - Make intersection computations spec conform

Review commit: https://reviewboard.mozilla.org/r/83762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83762/

Comment 50

[Markus Stange [:mstange]]

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.

Comment 51

[Tobias Schneider [:tobytailor]]

Attachment: Tests (v3)

Added tests for edge-inclusive intersections, NaN threshold values and same/cross origin observations.

Comment 52

[Tobias Schneider [:tobytailor]]

Attachment: Make rootBounds nullable

Updated bindings to make rootBounds nullable.

Comment 53

[Markus Stange [: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.

Comment 54

[Markus Stange [:mstange]]

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.

Comment 55

[Markus Stange [: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
@@ +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

Comment 56

[Tobias Schneider [:tobytailor]]

Attachment: Implement Intersection Observer API. r=mrbkap,mstange

Final patch to land.

Comment 57

[Tobias Schneider [:tobytailor]]

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

Comment 58

[ziyunfei]

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

[ziyunfei]

Here also a issue about `observer.thresholds` https://github.com/WICG/IntersectionObserver/issues/162

Comment 60

[Tobias Schneider [:tobytailor]]

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.

Comment 61

[Tobias Schneider [:tobytailor]]

Attachment: Add Intersection Observer interfaces to test_interfaces.html. r=mrbkap

Comment 62

[Tobias Schneider [:tobytailor]]

Attachment: 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.

Comment 63

[Markus Stange [:mstange]]

Why is the scroll port rect not working? If you haven't found out, please do.

Comment 64

[Tobias Schneider [:tobytailor]]

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.

Comment 65

[Tobias Schneider [:tobytailor]]

Attachment: Handle cross-doc,non-implicit root observations + test fixes.

Comment 66

[Markus Stange [: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.

Comment 67

[Tobias Schneider [:tobytailor]]

Attachment: Handle cross-doc,non-implicit root observations + test fixes. (v2)

Addressed review comments.

Comment 68

[Markus Stange [: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?

Comment 69

[Tobias Schneider [:tobytailor]]

(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.

Comment 70

[Tobias Schneider [:tobytailor]]

Attachment: Handle cross-doc,non-implicit root observations + test fixes. (v3)

Made reasons for skipping processing of target more clear + tests.

Comment 71

[Markus Stange [: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 72

[Markus Stange [:mstange]]

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?

Comment 73

[Tobias Schneider [:tobytailor]]

(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.

Comment 74

[Tobias Schneider [:tobytailor]]

Attachment: 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

Comment 75

[Markus Stange [: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.

Comment 76

[Carsten Book [:Tomcat]]

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

Comment 77

[Pulsebot]

Pushed by [e-mail]:
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

Comment 78

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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
...

Comment 79

[Tobias Schneider [:tobytailor]]

Attachment: 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

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/111c1227f51e
Implement Intersection Observer API. r=mrbkap, r=mstange

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 82

[Tobias Schneider [:tobytailor]]

Attachment: 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

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e781308ebf
Implement Intersection Observer API. r=mrbkap, r=mstange

Comment 84

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/72e781308ebf

Comment 85

[Chris Peterson [:cpeterson]]

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

Comment 86

[yisibl]

Update: 
https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver
https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

Comment 87

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is any manual testing needed for this feature?

Comment 88

[Chris Peterson [:cpeterson]]

(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.

Comment 89

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx52 Aurora release notes

Comment 90

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 91

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, meant "disabled" rather than "backed out".

Comment 92

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 93

[Julien Cristau [:jcristau]]

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.

Comment 94

[Sebastian Zartner [:sebo]]

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

Comment 95

[Jean-Yves Perrier [:teoli]]

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.

Comment 96

[Gerry Chang [:gchang]]

Move this to MDN.

Comment 97

[Michał Gołębiowski-Owczarek [:mgol]]

Should this bug be reopened since it keeps getting new open dependent bugs and the feature is not ready yet?

Comment 98

[Tobias Schneider [:tobytailor]]

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

[Jet Villegas (inactive)]

Reopening. Please mark fixed after all uplifts land in FF55.

Comment 100

[Tobias Schneider [:tobytailor]]

Waiting for Bugs 1359317 and 1363650 to be uplifted to FF55.

Comment 101

[Tobias Schneider [:tobytailor]]

Mentioned bugs have been uplifted.

Comment 102

[Tobias Schneider [:tobytailor]]

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.