IntersectionObserver delivers incorrect records and fires at the wrong time

[Elliott Sprehn]

The implementation of scheduling done here https://bugs.webkit.org/show_bug.cgi?id=189007 does not follow the spec which requires that the observations be computed after the next layout. Instead they seem to be on a one shot timer that's not connected to the rendering system. This means they report about elements not in the document, and also that there's both ordering bugs compared to other browsers as well as race conditions where sometimes the wrong records are delivered.

ex.
// Does not notify in Chrome or Firefox, notifies with a zero sized rect in Safari.

o = new IntersectionObserver((records) => console.log(records))
d = document.createElement('div')
o.observe(d)


ex.
// Delivers intersection records once with the right width and height in Chrome and Firefox. Delivers zero sized rects in Safari and then the correct ones later.

o = new IntersectionObserver((records) => console.log(records))
d = document.createElement('div')
o.observe(d)
requestAnimationFrame(() => { document.body.append(d); d.textContent = 'Hello World' })


ex.
// Delivers a record with zero height and *never* delivers the correct rects.

document.body.style.fontSize = 32;
o = new IntersectionObserver((records) => {
  // prints [0], but should print a number greater than 32.
  console.log(records.map(r => r.boundingClientRect.height));
})
d = document.createElement('div')
o.observe(d)
setTimeout(() => {
  let x = document.createElement('div')
  x.textContent = 'Hello World'
  d.appendChild(x);
});
document.body.append(d);


The code also skips computing intersections if there's a pending style recalc or layout:
https://github.com/WebKit/webkit/blob/9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.cpp#L7540
and then restarts the timer if no layout is required in Document::resolveStyle
https://github.com/WebKit/webkit/blob/9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.cpp#L1918
but if layout was required nothing seems to restart the timer. I think this works today because updateLayoutAndStyleIfNeededRecursive will always run all the steps twice so the timer gets restarted on the second pass. If that loop wasn't there I don't think IntersectionObserver would work.

[Elliott Sprehn]

Note that the first two issues we can workaround by ignoring zero sized rects, but the second has no workaround because the IO reports zero sized rects and then never reports the correct values. 

This impacts airbnb.com where our IntersectionObservers are unreliable on Safari because they report zero sized rects even when the element is non-zero in size.

It seems to reliably reproduce on about:blank, and unreliably on other sites (I think because there's racing issues). One way I can reproduce it multiple times:

1. Load about:blank
2. Execute this in the console:

var i = 0; setInterval(() => { document.body.style.fontSize = i++ % 32 + 1 });

3. Paste this into the console:

o = new IntersectionObserver((records) => {
  console.log(records.map(r => r.boundingClientRect.height));
})
d = document.createElement('div')
o.observe(d)
setTimeout(() => {
  let x = document.createElement('div')
  x.textContent = 'Hello World'
  d.appendChild(x);
});
document.body.append(d)

It prints [0] for me every time I paste that snippet while the setInterval is running.

[Radar WebKit Bug Importer]

<rdar://problem/49798756>

[Simon Fraser (smfr)]

We're in the middle of fixing our rAF timing (bug 177484) which should also fix this.

[Ali Juma]

As Simon points out, the root cause of the timing differences between WebKit and other engines was the lack of support for the HTMLEventLoop spec. Bug 177484 addresses that. Some more details inline below.

(In reply to Elliott Sprehn from comment #0)
> The implementation of scheduling done here
> https://bugs.webkit.org/show_bug.cgi?id=189007 does not follow the spec
> which requires that the observations be computed after the next layout.
> Instead they seem to be on a one shot timer that's not connected to the
> rendering system.

The initial implementation purely relied on timers. In bug 189091, this was changed so that most observations happened when layers were flushed from the WebProcess to the UIProcess; the exception being the initial observation, which happened on a timer if there were was no pending layout that would trigger a flush. Bug 177484 removes the timer altogether.

> This means they report about elements not in the document,

Other engines will also report about unattached elements, as long as they are in the unattached state after the rendering steps are carried out.

> and also that there's both ordering bugs compared to other browsers as well
> as race conditions where sometimes the wrong records are delivered.
> 
> ex.
> // Does not notify in Chrome or Firefox, notifies with a zero sized rect in
> Safari.
> 
> o = new IntersectionObserver((records) => console.log(records))
> d = document.createElement('div')
> o.observe(d)

I tested this in Chrome and Firefox, and both will notify with a zero sized rect the next time they run through their rendering loop.

> ex.
> // Delivers intersection records once with the right width and height in
> Chrome and Firefox. Delivers zero sized rects in Safari and then the correct
> ones later.
> 
> o = new IntersectionObserver((records) => console.log(records))
> d = document.createElement('div')
> o.observe(d)
> requestAnimationFrame(() => { document.body.append(d); d.textContent =
> 'Hello World' })

This difference in behavior is indeed because of the difference in rAF timing. We compute the initial intersection before the rAF, and then again after the rAF, while Chrome and Firefox would only compute the intersection after rAF.

> ex.
> // Delivers a record with zero height and *never* delivers the correct rects.
> 
> document.body.style.fontSize = 32;
> o = new IntersectionObserver((records) => {
>   // prints [0], but should print a number greater than 32.
>   console.log(records.map(r => r.boundingClientRect.height));
> })
> d = document.createElement('div')
> o.observe(d)
> setTimeout(() => {
>   let x = document.createElement('div')
>   x.textContent = 'Hello World'
>   d.appendChild(x);
> });
> document.body.append(d);

I was actually able to reproduce this (printing [0]) in Chrome too. It comes down to whether the setTimeout executes before or after intersection observations are computed. So in Chrome it depends on whether the setTimeout happens before or after rAF, and in WebKit it depended on whether the setTimeout happened before or after layer flush. That's the race you're observing.

If observations are computed before the setTimeout fires, we have an initial observation with zero area and intersection ratio 1 (this is as defined by spec, for the case of an attached element with zero area). This means that after the font size is changed, there's no additional observation (since the intersection ratio is unchanged, still 1).


> The code also skips computing intersections if there's a pending style
> recalc or layout:
> https://github.com/WebKit/webkit/blob/
> 9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.
> cpp#L7540
> and then restarts the timer if no layout is required in
> Document::resolveStyle
> https://github.com/WebKit/webkit/blob/
> 9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.
> cpp#L1918
> but if layout was required nothing seems to restart the timer. I think this
> works today because updateLayoutAndStyleIfNeededRecursive will always run
> all the steps twice so the timer gets restarted on the second pass. If that
> loop wasn't there I don't think IntersectionObserver would work.

No, if layout is is required, we would eventually call FrameView::performPostLayoutTasks ---> FrameView::viewportContentsChanged, and this would add the Document to the set of Documents needing intersection observations updated at the next flush. This call path has been removed in bug 177484 though, and observations are updated after rAF.

[Elliott Sprehn]

(In reply to Ali Juma from comment #4)
> As Simon points out, the root cause of the timing differences between WebKit
> and other engines was the lack of support for the HTMLEventLoop spec. Bug
> 177484 addresses that. Some more details inline below.
> 
> (In reply to Elliott Sprehn from comment #0)
> > The implementation of scheduling done here
> > https://bugs.webkit.org/show_bug.cgi?id=189007 does not follow the spec
> > which requires that the observations be computed after the next layout.
> > Instead they seem to be on a one shot timer that's not connected to the
> > rendering system.
> 
> The initial implementation purely relied on timers. In bug 189091, this was
> changed so that most observations happened when layers were flushed from the
> WebProcess to the UIProcess; the exception being the initial observation,
> which happened on a timer if there were was no pending layout that would
> trigger a flush. Bug 177484 removes the timer altogether.

The timer breaks sites that depend on requestAnimationFrame() happening before the observations are reported. This is pretty bad because by shipping this in Safari all the polyfills now default to the native implementation and report the wrong sizes compared to Firefox and Chrome.

https://github.com/w3c/IntersectionObserver/blob/master/polyfill/intersection-observer.js#L14

> 
> > This means they report about elements not in the document,
> 
> Other engines will also report about unattached elements, as long as they
> are in the unattached state after the rendering steps are carried out.
> 
> > and also that there's both ordering bugs compared to other browsers as well
> > as race conditions where sometimes the wrong records are delivered.
> > 
> > ex.
> > // Does not notify in Chrome or Firefox, notifies with a zero sized rect in
> > Safari.
> > 
> > o = new IntersectionObserver((records) => console.log(records))
> > d = document.createElement('div')
> > o.observe(d)
> 
> I tested this in Chrome and Firefox, and both will notify with a zero sized
> rect the next time they run through their rendering loop.

Yeah that was my misunderstanding (and an error I had in my test). Thanks!

> 
> > ex.
> > // Delivers intersection records once with the right width and height in
> > Chrome and Firefox. Delivers zero sized rects in Safari and then the correct
> > ones later.
> > 
> > o = new IntersectionObserver((records) => console.log(records))
> > d = document.createElement('div')
> > o.observe(d)
> > requestAnimationFrame(() => { document.body.append(d); d.textContent =
> > 'Hello World' })
> 
> This difference in behavior is indeed because of the difference in rAF
> timing. We compute the initial intersection before the rAF, and then again
> after the rAF, while Chrome and Firefox would only compute the intersection
> after rAF.
> 

Right, which breaks everyone now that the polyfill is disabled because the implementation is not compatible.

> > ex.
> > // Delivers a record with zero height and *never* delivers the correct rects.
> > 
> > document.body.style.fontSize = 32;
> > o = new IntersectionObserver((records) => {
> >   // prints [0], but should print a number greater than 32.
> >   console.log(records.map(r => r.boundingClientRect.height));
> > })
> > d = document.createElement('div')
> > o.observe(d)
> > setTimeout(() => {
> >   let x = document.createElement('div')
> >   x.textContent = 'Hello World'
> >   d.appendChild(x);
> > });
> > document.body.append(d);
> 
> I was actually able to reproduce this (printing [0]) in Chrome too. It comes
> down to whether the setTimeout executes before or after intersection
> observations are computed. So in Chrome it depends on whether the setTimeout
> happens before or after rAF, and in WebKit it depended on whether the
> setTimeout happened before or after layer flush. That's the race you're
> observing.
> 
> If observations are computed before the setTimeout fires, we have an initial
> observation with zero area and intersection ratio 1 (this is as defined by
> spec, for the case of an attached element with zero area). This means that
> after the font size is changed, there's no additional observation (since the
> intersection ratio is unchanged, still 1).

Ah yeah, I realize that the timer case is a race. We're seeing this on regular content changes though. We do:

setTimeout(() => {
  o.unobserve(d);
  o.observe(d);
  d.textContent = 'Hello world';
});

and expect the very next observation to contain the boundingClientRect. Safari seems to sometimes report a zero sized rect though, while Chrome and Firefox never do.

> 
> 
> > The code also skips computing intersections if there's a pending style
> > recalc or layout:
> > https://github.com/WebKit/webkit/blob/
> > 9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.
> > cpp#L7540
> > and then restarts the timer if no layout is required in
> > Document::resolveStyle
> > https://github.com/WebKit/webkit/blob/
> > 9029c43e695bf886fffb15eec951f0605e34509b/Source/WebCore/dom/Document.
> > cpp#L1918
> > but if layout was required nothing seems to restart the timer. I think this
> > works today because updateLayoutAndStyleIfNeededRecursive will always run
> > all the steps twice so the timer gets restarted on the second pass. If that
> > loop wasn't there I don't think IntersectionObserver would work.
> 
> No, if layout is is required, we would eventually call
> FrameView::performPostLayoutTasks ---> FrameView::viewportContentsChanged,
> and this would add the Document to the set of Documents needing intersection
> observations updated at the next flush. This call path has been removed in
> bug 177484 though, and observations are updated after rAF.

Ah I see. Does that mean there's situations where the layout keeps being dirtied before that timer fires and then it never reports an update?

What's currently shipping breaks sites by reporting incorrect rects because DOM updates aren't reflected in the reported sizes though. Would it be possible to ship an update to Safari that disables this feature until it works properly? By shipping this it forced the polyfill off for everyone, which then breaks sites that depended on getting the correct sizes back (ex. from requestAnimationFrame).

[Elliott Sprehn]

(In reply to Elliott Sprehn from comment #5)
> (In reply to Ali Juma from comment #4)
> > ...
> > 
> > I was actually able to reproduce this (printing [0]) in Chrome too. It comes
> > down to whether the setTimeout executes before or after intersection
> > observations are computed. So in Chrome it depends on whether the setTimeout
> > happens before or after rAF, and in WebKit it depended on whether the
> > setTimeout happened before or after layer flush. That's the race you're
> > observing.
> > 
> > If observations are computed before the setTimeout fires, we have an initial
> > observation with zero area and intersection ratio 1 (this is as defined by
> > spec, for the case of an attached element with zero area). This means that
> > after the font size is changed, there's no additional observation (since the
> > intersection ratio is unchanged, still 1).
> 
> Ah yeah, I realize that the timer case is a race. We're seeing this on
> regular content changes though. We do:
> 
> setTimeout(() => {
>   o.unobserve(d);
>   o.observe(d);
>   d.textContent = 'Hello world';
> });
> 
> and expect the very next observation to contain the boundingClientRect.
> Safari seems to sometimes report a zero sized rect though, while Chrome and
> Firefox never do.
> 


Oh I figured this out (thanks for the explanations and help!), we're seeing the [0] sized result and then unobserving assuming the content is empty, and then we get the correct result right after. Looking at Chrome's code it seems to clear the set of entries when you unobserve so it never reports about stale observations:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc?rcl=db03577a49d9bc9202922755a2119efa65335567&l=102

while WebKit will still deliver entries from a previous observation because they're scheduled on another timer and unobserve doesn't remove the stale entries.

https://github.com/WebKit/webkit/blob/7fe9b45a28870a7ffd79603602156f68dfe9e1f8/Source/WebCore/page/IntersectionObserver.cpp#L160

I think both unobserve() and disconnect() need to clear the pending entries?

ex. https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc?rcl=038c4233dfa81bf41de923d9c20b1cfc3af200f7&l=334

[Elliott Sprehn]

Firefox apparently doesn't clear the entries either:

https://dxr.mozilla.org/mozilla-central/rev/b5bcfc2617667e3fc7a095b262b80377b2542446/dom/base/DOMIntersectionObserver.cpp#139

and the spec doesn't say to do it. So the issue with Safari is that it both doesn't clear the entries and has the race with requestAnimationFrame which combines into this bug for us.

Seems like Chrome should update to not clear the set of entries or Firefox/Safari/the spec should change.

That's my bad for not digging deeper into this beforehand. :) I do still think it would be nice to disable this feature in Safari until the ordering is right though. JS frameworks are broken by requestAnimationFrame based DOM batching and IntersectionObserver not being ordered.

[Simon Fraser (smfr)]

(In reply to Elliott Sprehn from comment #7)
> I do still
> think it would be nice to disable this feature in Safari until the ordering
> is right though. JS frameworks are broken by requestAnimationFrame based DOM
> batching and IntersectionObserver not being ordered.

Sadly we can't retroactively disable it in shipped software, and the next release with have the rAF bug fix.

It sounds like this bug should just address the issue with clearing entries (after the spec is fixed to clarify this).

[Ali Juma]

In reply to Simon Fraser (smfr) from comment #8)
> 
> It sounds like this bug should just address the issue with clearing entries
> (after the spec is fixed to clarify this).

I've filed a spec issue to get the expected behavior clarified: https://github.com/w3c/IntersectionObserver/issues/356

[Ali Juma]

(In reply to Ali Juma from comment #9)
> In reply to Simon Fraser (smfr) from comment #8)
> > 
> > It sounds like this bug should just address the issue with clearing entries
> > (after the spec is fixed to clarify this).
> 
> I've filed a spec issue to get the expected behavior clarified:
> https://github.com/w3c/IntersectionObserver/issues/356

To workaround this in the meanwhile, calling o.takeRecords() will clear out the queued entries. e.g., if you do:

o.unobserve(d);
o.takeRecords();
o.observe(d);

Then you're guaranteed that any observations delivered after the latter call to observe() aren't stale.

Any news on the incorrect timing of the event being fired?

I ran into this when I had a ResizeObserver and an IntersectionObserver. Chrome and FF both fire the RO first, then the IO, but Safari does it in the reverse order.

I currently wrap the IO callback in a rAF() to fix it, but would be lovely if I could remove the hack from my codebase (if only because it relies on browser detection via userAgent string).

Until that time, here's the (pseudo) code to work around it for anyone running into the same issue:

```
const fixTiming = isSafar() ? requestAnimationFrame : fn => fn();
new IntersectionObserver( fixTiming(callback), {...opts} );
```

[Simon Fraser (smfr)]

Ordering should be fixed by bug 245946.

Great, thanks!