-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 a 'noopener' link attribute #290
Conversation
Getting something like this into HTML might make it possible for Secure Contexts to distinguish between popups that could be secure contexts and popups that couldn't. WDYT, @annevk? |
I think we should add some text that explains why you want to use this instead of |
Simplified the patch, and added some text to Basically, we just tacked completely unrelated functionality onto |
"Charlie Reis" needs to be added to the acknowledgements section too. |
Can we also make sure a caniuse entry and web platform tests are in progress? Do we have interest from more than one implementer? |
@rlbmoz, @dveditz: How do y'all feel about implementing this in Firefox? :) |
We need to credit @sc0ttbeardsley in Acknowledgements since he proposed this in https://lists.w3.org/Archives/Public/public-whatwg-archive/2015May/0046.html. Per http://logs.glob.uno/?c=mozilla%23content&s=5+Nov+2015&e=5+Nov+2015#c338410 at least some from Mozilla are on board with this feature. |
<p>This keyword also <a href="#noopener">causes the <code data-x="dom-opener">opener</code> | ||
attribute to remain null</a> if the hyperlink <span data-x="creating a new browsing context">creates</span> a new <span>browsing context</span>.</p> | ||
<p class="note">For historical reasons, the <code data-x="rel-noreferrer">noreferrer</code> | ||
attribute implies the behavior associated with the <code data-x="rel-noopener">noopener</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not the attributes you're looking for. (Keywords.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
I added the |
Thanks for the tag @annevk. I have been chatting with Steve Workman at Mozilla (sorry, don't know his github) and his team was going to look into at least throwing a warning when window.opener is modified across origins (I think IE does this already). @mikewest Here is the original bug I filed: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28821 (I suspect this PR should resolve that). Also it looks like http://crbug.com/69267 is related to http://crbug.com/168988 I just closed my PR on the wrong repo. Regarding the proposal, I had thought about using "noopener" keyword but I think "newcontext" might be a bit more explicit. Ultimately we don't only want window.opener to be nullified, right? I think there is a desire to also do all of the other things (it might be hard to explicitly list everything though) associated with a new browsing context. |
Note to self: file a bug against Gecko once this is merged. |
<p class="note">For historical reasons, the <code data-x="rel-noreferrer">noreferrer</code> | ||
keyword implies the behavior associated with the <code data-x="rel-noopener">noopener</code> | ||
keyword when present on a hyperlink that <span | ||
data-x="creating a new browsing context">creates</span> a new <span>browsing context</span>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are allowed to break inside an attribute value when it has spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This document has seriously strange style rules.
@sc0ttbeardsley what other things are you thinking of that are not related to the browsing context having an opener browsing context (and therefore being auxiliary)? |
@annevk the original chromium bug mentioned running in a new "process". We should avoid getting so explicit that we dive into browser implementation variations on what constitutes a new browsing context. For instance, do all browsers equate new processes with new browsing contexts? |
Either way it's a new browsing context. When the new browsing context has no opener it can run (in theory) in a distinct process since there's no synchronous script access to it. |
Refactored the patch a bit. WDYT, @annevk? @sc0ttbeardsley: Thanks for the links to the chromium bugs. I've added some labels there. |
<td><code data-x="rel-noopener">noopener</code></td> | ||
<td><em>not allowed</em></td> | ||
<td><span data-x="hyperlink annotation">Annotation</span></td> | ||
<td>Requires that any <span>browsing context</span> created by following the hyperlink must not have a <span>opener browsing context</span>.</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
I think with those nits this looks good. After this lands #313 will still need to be fixed to fully define the behavior added here accurately, correct? |
@@ -20968,6 +20968,11 @@ interface <dfn>HTMLHyperlinkElementUtils</dfn> { | |||
|
|||
</li> | |||
|
|||
<li><p>If <var>subject</var>'s <a href="#linkTypes">link types</a> include the <code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm. No? "types" is plural, so it doesn't "include". At least, "types includes" sounds strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Misread.
Squished. I didn't change "includes", but I will if you feel strongly about it. |
Based on [Charlie Reis' proposal][1], this patch adds a mechanism by which web developers can open a new window without granting that window access to the opener browsing context. This is possible today via 'rel=noopener target=_blank', but that is both obscure and poorly explanatory. This patch adds a 'noopener' keyword to both hyperlinks and 'window.open' which clarifies the behavior and makes it available without side effects. [1]: https://wiki.whatwg.org/wiki/Links_to_Unrelated_Browsing_Contexts
Ah, unfortunately this was not rebased so it did not merge completely cleanly. 2992ea9 is the commit. |
Thank you though for your efforts! |
webkit bug: https://bugs.webkit.org/show_bug.cgi?id=155166 |
Based on Charlie Reis' proposal at 1, this patch adds a mechanism
by which web developers can open a new window without granting that
window access to the opener browsing context. This is possible today
via , but that is both obscure and
poorly explanatory. The attribute clarifies the behavior.