Skip to content
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

Update noopener tests to match spec rules for choosing a browsing con… #16451

Merged
merged 2 commits into from
May 11, 2019
Merged

Conversation

natechapin
Copy link
Contributor

Per whatwg/html#1826, a few tests are out of date with https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name

html/browsers/the-window-object/window-open-noopener.html?indexed and
html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html
both assert that specifying noopener will prevent reuse of an existing named browser context, but the spec does not include this special case.

@annevk
Copy link
Member

annevk commented Apr 23, 2019

These changes make me realize there's something we did not discuss. Whether opener should be cleared or not. Currently the specification does not require that (look at the "window open steps", it never sets the opener browsing context to null). It probably makes sense, even though it could have been leaked already via another popup or so, but that would also require a further specification change.

@natechapin
Copy link
Contributor Author

These changes make me realize there's something we did not discuss. Whether opener should be cleared or not. Currently the specification does not require that (look at the "window open steps", it never sets the opener browsing context to null). It probably makes sense, even though it could have been leaked already via another popup or so, but that would also require a further specification change.

I don't feel strongly. FWIW, chromium doesn't currently clear the opener, but if the spec changes, I'm happy to update it.

@annevk
Copy link
Member

annevk commented May 7, 2019

Okay, but these test changes make it look like you were assuming such a spec change, or am I missing something? In particular they suggest that you should reuse but also not have an opener, whereas the specification says you should reuse and have an opener. I'd suggest we test the spec as-is first and then do a change separately, if desired.

@natechapin
Copy link
Contributor Author

Okay, but these test changes make it look like you were assuming such a spec change, or am I missing something? In particular they suggest that you should reuse but also not have an opener, whereas the specification says you should reuse and have an opener. I'd suggest we test the spec as-is first and then do a change separately, if desired.

That was poor naming on my part, sorry for the confusion. I've renamed "shouldHaveOpener" in window-open-nooopener.html to "shouldReturnWindow", which better described what the spec expects. When shouldReturnWindow is false because of noopener, the test case communicates with the target browsing context via the BroadcastChannel to ensure that the opener was not cleared even though the window.open() call returned null.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks great!

@annevk
Copy link
Member

annevk commented May 10, 2019

So from whatwg/html#1826 (comment) the main thing remaining here is implementation bugs I suppose?

@natechapin
Copy link
Contributor Author

So from whatwg/html#1826 (comment) the main thing remaining here is implementation bugs I suppose?

https://bugs.chromium.org/p/chromium/issues/detail?id=961816
https://bugzilla.mozilla.org/show_bug.cgi?id=1550840

@annevk annevk merged commit 0c79d0d into web-platform-tests:master May 11, 2019
@natechapin natechapin deleted the noopener branch May 13, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants