1400971 - Intersection Observer does not send an initial notification after .observe() is called by late-executing scripts.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[calhounsm]

Attachment: A simple page that satisfies steps 1 and 2 in reproduce.

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.91 Safari/537.36

Steps to reproduce:

1. Create a simple web page with an element and script that executes some time after page load (e.g. via setTimeout, async script load).
2. In the late-executing script, create an intersection observer and call observe on the element.
3. Open the page in Firefox 55 (Windows or Linux, OSX has the expected behavior).
4. Do not interact with the page after opening (e.g. no mouse movement, typing, etc).


Actual results:

Intersection Observer's callback is not called until some interaction with the page occurs, regardless of whether the element is within the viewport.


Expected results:

Intersection Observer's callback should be called in a timely manner, regardless of interaction with the page.

Comment 1

[Pascal Chevrel:pascalc (PTO until April 26)]

Moving to core::DOM as it seems to be a better component.

Comment 2

[Tobias Schneider [:tobytailor]]

This seam to happen on MacOS too for me. Investigating....

Comment 3

[Tobias Schneider [:tobytailor]]

Not able to reproduce anymore in FF 58.

Comment 4

[Andrew Overholt [:overholt]]

Maire, do you know who's covering Intersection Observer bugs these days? I *think* I can still reproduce it.

Comment 5

[Maire Reavy [:mreavy]]

Hi Andrew -- I haven't needed to touch Intersection Observer yet (as a dev or as a manager).  If you need my help tracking down the right owner, just needinfo me back.  Thanks.

Comment 6

[Andrew Overholt [:overholt]]

Maybe Sean knows who on the layout team can take a look?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So why is this even a valid bug? If anything hasn't changed in the page, per HTML spec 
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model the document maybe removed from docs in 'Update the rendering' and then observer isn't called.
Does Chrome do extra paints?

Comment 8

[Sean Voisen (:svoisen)]

I think smaug is right here.

That said, Hiro might be best contact on layout side to confirm. Hiro - Any thoughts here? I saw you did some work last on this in `nsRefreshDriver::UpdateIntersectionObservations`. Is this our intended behavior?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I agree with Olli.  Chrome triggers the callback for some reasons.  In the test case in comment 0, the observed entry's timestamp is around 1014 on my environment (chromium 68.0.3440.75 on Linux), that means that the entry happened after the callback for setTimeout(1000).

calhounsm, would you mind checking whether chrome behavior is correct or not?

What I am still concerned is that Tobias who implemented Intersection Observer in Firefox seemed to think this is a valid bug (as per comment 2).  So there may be some other test cases without setTimeout calls.  (Even with setTimeout calls, it depends on paint timing whether some entries are observed or not, I believe)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Degrading priority for now, I think this is not a valid bug.

Comment 11

[chrishtr]

Chromium browsers always send at least one observation when observe is called().

Otherwise scripts will not be able to use this API reliably, because without this observation
they will not be able to reliably initialize the internal state of the object under observation.
For example, an would not know whether it intersects the viewport until some future
change happens to dirty rendering.

I've followed up with szager-chromium to see if a spec clarification needs to be made.
We landed commits specifically to ensure this is the case, and I'll post an update also
with any WPTs for this that exist.

Comment 12

[chrishtr]

s/an would/an ad would/

Comment 13

[chrishtr]

See resolution here:

https://github.com/w3c/IntersectionObserver/issues/426

"previousThresholdIndex gets an initial value of -1 for the express purpose of forcing a notification at the earliest opportunity, because the Run the Update Intersection Observations Steps procedure in the spec can never produce a thresholdIndex less than zero (see step 2.2.9)."

Comment 14

[chrishtr]

This Chromium CL implements, and adds tests. The tests in question were subsequently upstreamed to WPT.

https://codereview.chromium.org/2645283008

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Looks like Tobias already fixed the case in bug 1335644. And the test case in comment 0 works fine on the latest nightly. Maybe some refactoring by Emilio addressed this issue? Or am I missing something?

I will try the web platform tests on the nightly.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I am confused. IIUC, one of the test case is here in edge-inclusive-intersection.html, and it's been passed since Tobias fixed bug 1335644. Is this still an issue?

Comment 17

[chrishtr]

Quick update: first, thanks for testing all the examples.

Second: I asked some more questions internally at Google and it seems that the issue is a bit hard to reproduce and happens most often in our automated testing of the product on Windows.

From my POV it's fine to just close this bug as fixed, since it sounds like a duplicate of issue 1335644. Some of the Google teams had found this bug and were unsure about whether the spec requires the behavior (which it does, hence 1335644). This bug being closed would make that more clear.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

chrishtr, thanks for the info. That's super helpful!

(In reply to chrishtr from comment #17)

Quick update: first, thanks for testing all the examples.

Second: I asked some more questions internally at Google and it seems that the issue is a bit hard to reproduce and happens most often in our automated testing of the product on Windows.

At least on our end, the test seems to have been stable. (An intermittent failure I could find is bug 1578639, but it's not related to this issue I think).
That said, Tobias said he could reproduce the original test case on Firefox 56 on his Mac in comment 2, then he said he can no longer reproduce the issue on Firefox 59. So, it's highly possible there was a race condition in Firefox as well. One thing come to mind was a micro task change (bug 1193394) but it landed in Firefox 60, so it's not.

From my POV it's fine to just close this bug as fixed, since it sounds like a duplicate of issue 1335644. Some of the Google teams had found this bug and were unsure about whether the spec requires the behavior (which it does, hence 1335644). This bug being closed would make that more clear.

Yeah, before I read https://github.com/w3c/IntersectionObserver/issues/165, I had a conclusion that IntersectionObserver shouldn't report the case. :)

Thanks for revisiting this issue, our issue. I am gong to close this.

Comment 19

[szager]

I think smaug's first comment is the reall issue:

(In reply to Olli Pettay [:smaug] from comment #7)

So why is this even a valid bug? If anything hasn't changed in the page, per
HTML spec
https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-
model the document maybe removed from docs in 'Update the rendering' and
then observer isn't called.
Does Chrome do extra paints?

Specifically, step 11.4:

<quote>
Unnecessary rendering: Remove from docs all Document objects which meet both of the following conditions:

• The user agent believes that updating the rendering of the Document's browsing context would have no visible effect, and
• The Document's map of animation frame callbacks is empty.
  </quote>

To answer smaug's question: yes, Chrome does an extra paint if there are any new observations that haven't generated a notification yet. We reasoned that the overhead of one extra paint (which is largely a no-op) is a worthwhile trade-off in this case.

This could be clarified by adding a third condition for removing a doc, along these lines:

• There are no Elements in any browsing context that have an IntersectionObserverRegistration in its RegisteredIntersectionObservers slot for which:
  • The IntersectionObserver's root is in the DOM tree of the Document, and
  • The previousThresholdIndex property is -1.

Comment 20

[szager]

I filed a bug on the IntersectionObserver spec:

https://github.com/w3c/IntersectionObserver/issues/430

This does seem like something that should be resolved to ensure interoperability, as it seems to be an ongoing issue for at least some developers; for example:

https://github.com/w3c/IntersectionObserver/issues/426