1411775 - [intersection-observer] Add tests for cross-document root/target and same root/target

Status: RESOLVED WORKSFORME

(Core :: Layout, enhancement, P3)

Description

[Tobias Schneider [:tobytailor]]

Current web-platform tests are missing tests for observers using explicit root and target being in different documents (prohibited by spec) or being the same element.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Tests

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8922102 [details] [diff] [review]
Tests

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

::: testing/web-platform/meta/MANIFEST.json
@@ +614593,4 @@
>     "support"
>    ],
>    "pointerevents/pointerevent_support.js": [
> +   "f3e21bc91b635f59e14c40535ad52b5d9d9df46d",

You should rebase this on top of up-to-date mozilla-central -- all these unrelated changes conflict with (and are redundant as of) Bug 1410245, which made it to mozilla-central an hour ago or so.

(You're probably fine to just apply your patch, let parts of it fail, and ignore those not-applying parts, because I think they're all redundant now.)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8922102 [details] [diff] [review]
Tests

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

::: testing/web-platform/tests/intersection-observer/iframe-root.html
@@ +50,5 @@
> +  }, "Observer with an explicit root; target in a same-origin iframe.");
> +};
> +
> +function step0() {
> +    assert_equals(entries.length, 0, "No notifications.");

Two things:
 (1) Can you add a comment here (or a clearer message) to hint at *why* no notifications are expected?  (I think it's because the explicit root and the target are in different documents, based on comment 0 here)

 (2) Shouldn't we try scrolling the iframe's document (e.g. via iframe.contentDocument.scrollingElement.scrollTop = 250)* and make sure entries.length is still 0, for good measure?  (Because: the way things are set up right now, it looks like "target" isn't really inside the bounds of "root" -- it's out of view in the iframe that root encloses.  So I could imagine that we might not report any intersections, regardless of whether we have this cross-document logic implemented correctly.  So, seems like it'd be worth scrolling "target" into view and verifying that we still don't get any reports.)

*cribbed from iframe-no-root.html

Comment 4

[Daniel Holbert [:dholbert]]

Also: it looks like you're modifying two files that we imported from upstream.... e.g. this patch is renaming iframe-no-root-subframe.html to iframe-subframe.html, and I think (?) this is a file that was authored by Chromium folks here:
https://github.com/szager-chromium/web-platform-tests/commit/3d17daafba2bf3f9cf246a1688562a41235dfbf1#diff-d966c22f95f558fd557558ef498d02c7

I'm not familiar with web-platform-test lifecycle, but I'm hoping you are. :)  Just to be sure: are you sure this sort of tweak to imported tests is OK, process-wise?

(I'm willing to believe it is, since I think we sync our changes upstream, and your tweaks here seem minimal & reasonable to not really require review from the test author. Just want to be sure, though.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Also: it looks like you're modifying two files that we imported from
> upstream [...]
> are you sure this sort of tweak to imported tests is
> OK, process-wise?

I discussed this with jgraham yesterday, and he said this is totally fine, at least when the changes are guaranteed non-controversial (as is the case here).

I'm not sure tobytailor has access to his @mozilla.com Bugzilla account anymore, since he's left MoCo and doesn't have that email address anymore -- so I'll probably go ahead and address the review comments on his behalf sometime soon, and get this patch landed.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 7

[Daniel Holbert [:dholbert]]

I'm not realistically getting to resurrecting/completing these tests anytime soon; and in any case it looks like this testing gap might have been filled. We have a handful of cross-document intersection observer WPT tests these days:
https://searchfox.org/mozilla-central/search?q=iframe&path=testing%2Fweb-platform%2Ftests%2Fintersection-observer&case=false&regexp=false

e.g.:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/intersection-observer/cross-document-root.html
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/intersection-observer/resources/cross-origin-subframe.html

Hence, closing as WORSKFORME since the issue (adding test coverage) seems to have been addressed over time.