Open Bug 1411775 Opened 4 years ago Updated 4 years ago

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

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

ASSIGNED

People

(Reporter: tschneider, Assigned: tschneider, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attached patch TestsSplinter Review
Attachment #8922102 - Flags: review?(dholbert)
Assignee: nobody → tschneider
Status: NEW → ASSIGNED
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 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
Flags: needinfo?(tschneider)
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.)
(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.
Flags: needinfo?(tschneider)
Flags: needinfo?(dholbert)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.