New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for blocked iframe navigation #12290

Merged
merged 3 commits into from Aug 15, 2018

Conversation

Projects
None yet
4 participants
@jugglinmike
Contributor

jugglinmike commented Aug 3, 2018

Step 2 of "process the iframe attributes" [1] reads:

  1. If there exists an ancestor browsing context whose active
    document's URL, ignoring fragments, is equal to url, then return.

Add tests for this behavior, verifying the absence of navigation through
the synchronously-fired beforeunload event.

[1] https://html.spec.whatwg.org/multipage/iframe-embed-object.html#otherwise-steps-for-iframe-or-frame-elements

Add test for blocked iframe navigation
Step 2 of "process the iframe attributes" [1] reads:

> 2. If there exists an ancestor browsing context whose active
>    document's URL, ignoring fragments, is equal to url, then return.

Add tests for this behavior, verifying the absence of navigation through
the synchronously-fired `beforeunload` event.

[1] https://html.spec.whatwg.org/multipage/iframe-embed-object.html#otherwise-steps-for-iframe-or-frame-elements
function when(target, eventName) {
return new Promise(function(resolve, reject) {
target.addEventListener(eventName, function handle() {
target.removeEventListener(eventName, handle);

This comment has been minimized.

@bzbarsky

bzbarsky Aug 3, 2018

Contributor

Any reason not to just use { once: true } in the addEventListener call? I think pretty much everyone supports that now.

This comment has been minimized.

@jugglinmike

jugglinmike Aug 3, 2018

Contributor

I tend to be pretty conservative for usage of platform features. I'm happy to do this here since removing the handler is more of a courtesy--a Promise can only be resolved once, after all.

var subjectNavigation = when(iframes[0].contentWindow, 'beforeunload');
var controlNavigation = when(iframes[1].contentWindow, 'beforeunload');
iframes[0].src = location.pathname;

This comment has been minimized.

@bzbarsky

bzbarsky Aug 3, 2018

Contributor

location.href might be clearer as to what's going on, I think.

This comment has been minimized.

@jugglinmike

jugglinmike Aug 3, 2018

Contributor

I chose pathname for parity with the root-relative "control" URL. Clarity is more important, though, so I'll go with your preference.

return Promise.all([subjectNavigation, controlNavigation]);
});
}, 'different path name');

This comment has been minimized.

@bzbarsky

bzbarsky Aug 3, 2018

Contributor

This test doesn't have any test assertions. Is it just testing that loads generally work? If so, it's worth documenting that explicitly. But it's not very clear to me what this is trying to test.

This comment has been minimized.

@jugglinmike

jugglinmike Aug 3, 2018

Contributor

All the other tests assert the same result, making me suspicious of test bugs. This test gives a little more confidence in the correctness of the utility functions. I'll add a comment explaining this.

iframes[0].src = location.pathname;
iframes[1].src = '/common/blank.html?4';
return Promise.race([

This comment has been minimized.

@bzbarsky

bzbarsky Aug 3, 2018

Contributor

So I'm a little confused. If the beforeunload event fires sync (before the src setter returns, right?) then why does this need the promise bits?

It seems like this setup could be much simpler and just check that beforeunload fires during the src set for the different-url case but not the same-url case...

This comment has been minimized.

@jugglinmike

jugglinmike Aug 3, 2018

Contributor

This strategy is a layover from my earlier attempts to hook into the "load" event. A simplification like that would be nice, but Chrome is firing the event asynchronously, and I can't tell if that's allowed or not.

I believe the text that starts this off is:

whenever an iframe element with a non-null nested browsing context but with no srcdoc attribute specified has its src attribute set, changed, or removed, the user agent must process the iframe attributes.

That text doesn't seem to require synchronous behavior, so my initial claim about synchronicity may have been incorrect. Then again, since it's not in an algorithm, I'm not sure if it's normative. The description of the src attribute doesn't mention this directly, and I'm not sure where else the relevant normative text would go. Do you have any thoughts on this?

This comment has been minimized.

@zcorpan

zcorpan Aug 4, 2018

Contributor

It's normative because it has the word "must". It doesn't say to queue a task or anything, therefore it's synchronous.

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Aug 14, 2018

@bzbarsky now that gh-12343 has landed, WPT has coverage for the synchronicity of the beforeunload event. I'm inclined to keep these tests "loose" in that regard (despite the complexity) so that the behavior can be tested and fixed independently. (For what it's worth, all the WPT tests involving beforeunload currently support an incorrectly-asynchronous dispatch.) What do you think?

@bzbarsky

This comment has been minimized.

Contributor

bzbarsky commented Aug 14, 2018

Sounds ok to me, but maybe add a comment in the test explaining why the complexity is there.

@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Aug 14, 2018

Sounds good, @bzbarsky. It looks like I forgot to push up the commit that implemented your feedback. This branch is all up-to-date now.

@bzbarsky

Looks great!

@bzbarsky bzbarsky merged commit 6af1174 into web-platform-tests:master Aug 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jugglinmike

This comment has been minimized.

Contributor

jugglinmike commented Aug 15, 2018

Thanks, @bzbarsky!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment