1354163 - Intermittent dom/base/test/test_intersectionobservers.html | supports getting records before the callback is invoked [takeRecords] - got +0, expected 1

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

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

https://queue.taskcluster.net/v1/task/La-nCKs6TFm0iRe5-h2vMw/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Jet Villegas (inactive)]

I don't understand how this test is supposed to be testing that IntersectionObserver "supports getting records before the callback is invoked". My questions as comments below:

    it('supports getting records before the callback is invoked',

      function(done) {

        var lastestRecords = [];
        io = new IntersectionObserver(function(records) {
          lastestRecords = lastestRecords.concat(records);
        }, {root: rootEl});
        
        io.observe(targetEl1);  // doesn't callback gets called immediately when an intersection occurs?

        window.requestAnimationFrame && requestAnimationFrame(function() {
          // why does this test assume that takeRecords() will happen before the callback above?
          lastestRecords = lastestRecords.concat(io.takeRecords());
        });

        callDelayed(function() {
          
          // when this fails, lastestRecords.length == 0
          // why would lastestRecords.length == 1 indicate that takeRecords did happen before the callback?
          expect(lastestRecords.length).to.be(1);
          expect(lastestRecords[0].intersectionRatio).to.be(1);
          done();
        }, ASYNC_TIMEOUT);

      }
    );

Comment 2

[Tobias Schneider [:tobytailor]]

(In reply to Jet Villegas (:jet) from comment #1)
>         
>         io.observe(targetEl1);  // doesn't callback gets called immediately
> when an intersection occurs?
> 

No, not immediately. Intersection check happens at next painting iteration, scheduling a task to be executed in a following event loop turn.

>         window.requestAnimationFrame && requestAnimationFrame(function() {
>           // why does this test assume that takeRecords() will happen before
> the callback above?
>           lastestRecords = lastestRecords.concat(io.takeRecords());
>         });

RAF's are supposed to happen before the the intersection check/observer notification.

>         callDelayed(function() {
>           
>           // when this fails, lastestRecords.length == 0
>           // why would lastestRecords.length == 1 indicate that takeRecords
> did happen before the callback?

It's not testing the order of which the callbacks above fire, only making sure that the current list of records is cleared after receiving them either via the observer callback or takeRecords. If that isn't the case, lastestRecords.length would be 2.

Comment 3

[Tobias Schneider [:tobytailor]]

After second thought. I'm pretty sure takeRecords will become obsolete. There are justified concerns about its usefulness, especially after https://bugzilla.mozilla.org/show_bug.cgi?id=1319134. Also see https://github.com/WICG/IntersectionObserver/issues/133. According to the spec, the 100ms delay mentioned in that thread is not used anymore. intersection Observer notifications are now fired as early as possible, event before rAF's. I confirmed that this is even already the case in Chrome. Anyhow, we should keep that test as long as takeRecords is officially part of the spec. Will add a patch to fix the intermittent failure for now.

Comment 4

[Tobias Schneider [:tobytailor]]

Attachment: Fix intermittent test failure

Comment 5

[Tobias Schneider [:tobytailor]]

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

Comment 6

[Jet Villegas (inactive)]

Comment on attachment 8858444 [details] [diff] [review]
Fix intermittent test failure

r+ with a better commit message, maybe:

Wait until there are intersection records to observe. Prevents an intermittent test failure.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Wait until there are intersection records to observe. Prevents an intermittent test failure.

Better commit message.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cf5283b925
Wait until there are intersection records to observe. Prevents an intermittent test failure, r=jet

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/65cf5283b925

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f86465621ed