Closed Bug 1313927 Opened 8 years ago Closed 7 years ago

Intermittent test_intersectionobservers.html | uses the viewport when no root is specified [observe]

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: aryx, Assigned: tschneider)

References

(Depends on 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 4 obsolete files)

Create separated test to disable it on Android only. r=jet
4.00 KB, patch
Details | Diff | Splinter Review
Patch for beta uplift
5.02 KB, patch
Details | Diff | Splinter Review 
Bug 1243846 added the intersection observers api.

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

20:49:02     INFO -  229 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | handles root/target elements not yet in the DOM [observe]
20:49:02     INFO -  230 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe]
20:49:02     INFO -  231 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | handles sub-root element scrolling [observe]
20:49:02     INFO -  232 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe]
20:49:02     INFO -  233 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | supports CSS transitions and transforms [observe]
20:49:02     INFO -  234 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_intersectionobservers.html | uses the viewport when no root is specified [observe]
20:49:02     INFO -      get be/fn.ok@dom/base/test/test_intersectionobservers.html:97:13
20:49:02     INFO -      window.onmessage@dom/base/test/test_intersectionobservers.html:886:11
20:49:14     INFO -  235 INFO TEST-PASS | dom/base/test/test_intersectionobservers.html | should not trigger if target and root are not in the same document [observe subframe]
Flags: needinfo?(tschneider)
Assignee: nobody → tschneider
Flags: needinfo?(tschneider)
See Also: → 1313972
Does this test need the same change from to.be.ok() to to.be.true() as the one in bug 1313972?

BTW, what's to prevent the postMessage() from intersectionobserver_window.html from racing with the postMessage() from intersectionobserver_cross_domain_iframe.html?

This specific test seems to fail most on android though some linux in here as well.
Flags: needinfo?(tschneider)
(In reply to Jet Villegas (:jet) from comment #9)
> Does this test need the same change from to.be.ok() to to.be.true() as the
> one in bug 1313972?

They are the same except that later would give us a bit more information if we keep failing.

> BTW, what's to prevent the postMessage() from
> intersectionobserver_window.html from racing with the postMessage() from
> intersectionobserver_cross_domain_iframe.html?

Its postMessage gets consumed by a test. So is the one from intersectionobserver_cross_domain_iframe.html. We had issues with non-consumed postMessage events.
Flags: needinfo?(tschneider)
(In reply to Tobias Schneider [:tobytailor] from comment #10)
> (In reply to Jet Villegas (:jet) from comment #9)
> > Does this test need the same change from to.be.ok() to to.be.true() as the
> > one in bug 1313972?
> 
> They are the same except that later would give us a bit more information if
> we keep failing.

bug 1321865 comment 74 seems to indicate that this is needed.

More questions as comments:

 // test_intersectionobservers.html

it('uses the viewport when no root is specified', function(done) {
  window.onmessage = function (e) {
    expect(e.data).to.be.ok();
    win.close(); // var win used before declaration?
    done();
};

var win = window.open("intersectionobserver_window.html");
});


// intersectionobserver_window.html

<!DOCTYPE HTML>
<html>
<head>
<style>
#target5 {
        position: absolute;
        top: 0px;
        left: 0px;
        width: 20px;
        height: 20px;
        background: #f00;
}
</style>
<body>
<!--- note that this div has no specified styles. intentional?  --->
<div id="target"></div>
<script>
        var io = new IntersectionObserver(function(records) {
          var viewportWidth =
              document.documentElement.clientWidth || document.body.clientWidth;
          var viewportHeight =
              document.documentElement.clientHeight || document.body.clientHeight;

          // this test can fail in 7 different ways but returns 1 result?
          var passed = records.length === 1 &&
                       records[0].rootBounds.top === 0 &&
                       records[0].rootBounds.left === 0 &&
                       records[0].rootBounds.right === viewportWidth &&
                       records[0].rootBounds.width === viewportWidth &&
                       records[0].rootBounds.bottom === viewportHeight &&
                       records[0].rootBounds.height === viewportHeight;
          window.opener.postMessage(passed, '*');
        });
        io.observe(document.getElementById("target"));
</script>
</body>
</html>
Flags: needinfo?(tschneider)
BTW, as written, this test looks like it can make multiple postMessage() messages (e.g., if records.length > 1).
(In reply to Jet Villegas (:jet) from comment #11)
> it('uses the viewport when no root is specified', function(done) {

Actually, why does this test needs a second window at all if all you're testing is that rootBounds === viewportBounds when a root element isn't specified?
> <!--- note that this div has no specified styles. intentional?  --->

No, but also not relevant for this test.

>           // this test can fail in 7 different ways but returns 1 result?

One result since all tests need to pass. Sure we can have several ones, will give us more info when failing.

> Actually, why does this test needs a second window at all if all you're testing is that rootBounds === viewportBounds when a root element isn't specified?

We want to make sure this test is running in a top level frame, not in unknown levels of iframes, like tests might run via the mochitest harness.
Flags: needinfo?(tschneider)
(In reply to Tobias Schneider [:tobytailor] from comment #14)
> > <!--- note that this div has no specified styles. intentional?  --->
> 
> No, but also not relevant for this test.

Why not? What is the size and location of the element you're expecting to intersect against the viewport?

> 
> >           // this test can fail in 7 different ways but returns 1 result?
> 
> One result since all tests need to pass. Sure we can have several ones, will
> give us more info when failing.

Exactly. I have no idea which of these 7 tests are returning false. Do you know?
> Why not? What is the size and location of the element you're expecting to
> intersect against the viewport?

We don't care about the intersection in this test. Only if rootBounds === viewport rect.
(In reply to Tobias Schneider [:tobytailor] from comment #16)
> > Why not? What is the size and location of the element you're expecting to
> > intersect against the viewport?
> 
> We don't care about the intersection in this test. Only if rootBounds ===
> viewport rect.

Isn't an intersection how you invoke the test? That is, what does an element with no size and position return in an intersection record?
By spec, it triggers an intersection notification with an empty intersection rectangle. See the 'handles zero-size targets within the root coordinate space' test.
I tried replicating this test here:
http://media.junglecode.net/test/1313927/testio.html

...which loads this file in a new window:
http://media.junglecode.net/test/1313927/io3.html

...which then sends the results of the 7 tests in a postMessage() back to the first window. 

Tests 6 & 7 fail for me using Nightly but passes on Chrome. Does it pass on your Android build?
Flags: needinfo?(tschneider)
(In reply to Jet Villegas (:jet) from comment #11)
>           var viewportWidth =
>               document.documentElement.clientWidth ||
> document.body.clientWidth;
>           var viewportHeight =
>               document.documentElement.clientHeight ||
> document.body.clientHeight;

The failures I see in comment 19 are because:

document.documentElement.clientWidth == 1280
document.body.clientWidth == 1280
document.documentElement.clientHeight == 8
document.body.clientHeight == 395

It works for me by replacing document.documentElement with document.body
(In reply to Tobias Schneider [:tobytailor] from comment #14)
> > Actually, why does this test needs a second window at all if all you're testing is that rootBounds === viewportBounds when a root element isn't specified?
> 
> We want to make sure this test is running in a top level frame, not in
> unknown levels of iframes, like tests might run via the mochitest harness.

This test should return the viewportBounds == rootBounds regardless of iframe nesting. That seems to be the original tests' intent:
https://github.com/WICG/IntersectionObserver/blob/gh-pages/polyfill/intersection-observer-test.js#L722

Can't we rewrite it this way and lose the flaky second window?

it('uses the viewport when no root is specified', function(done) {
  io = new IntersectionObserver(function(records) {
    var viewportWidth = document.body.clientWidth;
    var viewportHeight = document.body.clientHeight;

    expect(records.length).to.be(1);
    expect(records[0].rootBounds.top).to.be(0);
    expect(records[0].rootBounds.left).to.be(0);
    expect(records[0].rootBounds.right).to.be(viewportWidth);
    expect(records[0].rootBounds.width).to.be(viewportWidth);
    expect(records[0].rootBounds.bottom).to.be(viewportHeight);
    expect(records[0].rootBounds.height).to.be(viewportHeight);
    done();
  });

  io.observe(targetEl1);
});
If you would run that test in an iframe then viewportWidth/viewportHeight is the iframe's document dimensions and records[0].rootBounds the viewport rect of the top level page.
Flags: needinfo?(tschneider)
I separated the test from the suite so we can disable it individually.
Attachment #8860218 - Flags: review?(bugs)
Comment on attachment 8860218 [details] [diff] [review]
Create separated test to disable it on Android only

>+<script type="application/javascript">
>+
>+	var win = window.open("intersectionobserver_window.html");

Don't open the window before window.onmessage. Declare var win here, but move the window.open call to below onmessage

>+    window.onmessage = function (e) {
>+        win.close();
>+        ok(e.data);

e.data contains a single boolean. I'd like to see a way to inspect the failure beyond true/false since the test in intersectionobserver_window.html can fail several ways.
Make test more verbose when failing.
Attachment #8860218 - Attachment is obsolete: true
Attachment #8860218 - Flags: review?(bugs)
Attachment #8860230 - Flags: review?(bugs)
Address review comment.
Attachment #8860230 - Attachment is obsolete: true
Attachment #8860230 - Flags: review?(bugs)
Attachment #8860235 - Flags: review?(bugs)
Clean up indentation.
Attachment #8860235 - Attachment is obsolete: true
Attachment #8860235 - Flags: review?(bugs)
Attachment #8860249 - Flags: review?(bugs)
Attachment #8860249 - Flags: review?(bugs) → review+
Keywords: checkin-needed
needs rebasing
Flags: needinfo?(tschneider)
Rebased patch.
Attachment #8860249 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/827141143dc9
Create separated test to disable it on Android only. r=jet
Keywords: checkin-needed
sorry backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=93641110&repo=mozilla-inbound
Flags: needinfo?(tschneider)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7457db3be275
Backed out changeset 827141143dc9 for test failures in own test
Mh, there are quite a bunch of other tests that timed out during that run. Just bad timing maybe?
Flags: needinfo?(tschneider)
(In reply to Tobias Schneider [:tobytailor] from comment #34)
> Mh, there are quite a bunch of other tests that timed out during that run.
> Just bad timing maybe?

As checked in, this test will fail unless IntersectionObserver is enabled, right?

 [test_intersectionobservers.html]
 skip-if = true # Track Bug 1320704
+[test_bug1313927.html]
+skip-if = (os == "android") # bug 1313927
Flags: needinfo?(tschneider)
Thats correct... my try run from Comment 27 had the included from Bug 1321865. So we can either:

Land this patch with the new test disabled and enable it in Bug 1321865 or
Move this patch to Bug 1321865 and land it together or
Land this patch after Bug 1321865.

First option is probably the safest.
Flags: needinfo?(tschneider)
(In reply to Tobias Schneider [:tobytailor] from comment #36)
> Thats correct... my try run from Comment 27 had the included from Bug
> 1321865. So we can either:
> 
> Land this patch with the new test disabled and enable it in Bug 1321865 or
> Move this patch to Bug 1321865 and land it together or
> Land this patch after Bug 1321865.
> 
> First option is probably the safest.

I'd prefer to land all the dependent patches in one commit. Please prepare a Try run with all of them and we'll land them together when ready.
(In reply to Jet Villegas (:jet) from comment #35)
> As checked in, this test will fail unless IntersectionObserver is enabled,
> right?
(In reply to Tobias Schneider [:tobytailor] from comment #36)
> Thats correct... my try run from Comment 27 had the included from Bug
> 1321865.

The best solution for this generally (testing a feature that may or may not be enabled, or which might conceivably get disabled down the line in one of the release trains) is to either:
 (1) Use SpecialPowers.pushPrefEnv(...) in the mochitest to *actively* enable the feature, just for the test
...or:
 (2) SpecialPowers.getBoolPref(...) or similar in the mochitest, to check whether the feature is enabled, before you do any testing -- and then spamming a trivial test-result like:
   todo(false, "Skipping test since intersection observer support is disabled");
...because the mochitest harness requires you to run at least one check (todo/is/ok etc.) inside of your mochitest.
Comment on attachment 8860499 [details] [diff] [review]
Create separated test to disable it on Android only. r=jet

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

A few nits before this lands, to be sure it's landing in a state that makes sense:

Commit message:
> Bug 1313927 - Create separated test to disable it on Android only.

This is a bit too vague -- I'd like to see a clearer summary here.  How about something like the following:
> Bug 1313927 - Split a chunk of test_intersectionobservers.html into its own mochitest, so that we can skip that part on Android.

And one nit on mochitest.ini:

::: dom/base/test/mochitest.ini
@@ +655,5 @@
>  [test_integer_attr_with_leading_zero.html]
>  [test_intersectionobservers.html]
>  skip-if = true # Track Bug 1320704
> +[test_bug1313927.html]
> +skip-if = (os == "android") # bug 1313927

This annotation doesn't quite make sense.

Test-skipping annotations (with bug numbers) are supposed to link to an open bug that is tracking the fact that the test fails on that platform (and that bug is where the investigation will presumably happen).  In this case though, you're using the bug number for the bug that added the test, which will presumably be a closed bug as soon as this patch lands.  (Unless you want to tag this bug as leave-open and have this bug have patches land across multiple releases -- but that gets confusing to follow/track, too.)

So, you should probably file a followup bug for investigation of the android test failure, and use *that* bug number in the annotation here.
Flags: needinfo?(tschneider)
Depends on: 1360041
Flags: needinfo?(tschneider)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d35b1c5ab5
Split no-root test from test_intersectionobservers.html into its own mochitest, so that we can skip that part on Android. r=jet
https://hg.mozilla.org/mozilla-central/rev/c0d35b1c5ab5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8860499 [details] [diff] [review]
Create separated test to disable it on Android only. r=jet

Approval Request Comment
[Feature/Bug causing the regression]: 1321865
[User impact if declined]: No test coverage
[Is this code covered by automated tests?]: It is one
[Has the fix been verified in Nightly?]: Yes
[Is the change risky?]: No
Attachment #8860499 - Flags: approval-mozilla-beta?
Comment on attachment 8860499 [details] [diff] [review]
Create separated test to disable it on Android only. r=jet

Test-only changes don't need uplift approval, since the fall into the category of "not part of the build" (they're not part of the bits that we ship to users).

You should just tag a tree sheriff to land this on Beta for you, or add "checkin-needed" with some instructions about landing on beta.  (And you might want to double-check that the patch applies cleanly to beta, too, & post a different version if not.)
Attachment #8860499 - Flags: approval-mozilla-beta?
I did not know. Thanks for clarifying!
Blocks: 1362168
Whiteboard: [checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-beta/rev/c45159d6fe1f

And a follow-up for the busted Beta patch I didn't notice until after pushing :\
https://hg.mozilla.org/releases/mozilla-beta/rev/633207b5b91d
status-firefox54: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
Depends on: 1369253
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: