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

How should noopener/noreferrer affect link targeting? #1826

Closed
bzbarsky opened this issue Sep 27, 2016 · 48 comments
Closed

How should noopener/noreferrer affect link targeting? #1826

bzbarsky opened this issue Sep 27, 2016 · 48 comments
Labels
interop Implementations are not interoperable with each other topic: frames/navigables/browsing contexts topic: navigation

Comments

@bzbarsky
Copy link
Contributor

Consider this testcase:

<a rel="noreferrer" href="" target="foo">Click me, then click me again.</a>

Should clicking the link twice lead to one new window being opened or two? In Safari and Firefox and Edge it's one. In Chrome it's two.

What about the noopener case? The theory is it's just like the noreferrer case but sends a referrer.... But note that there is desire to be able to open noopener things in a totally new process, which does not play very nicely with being able to then target them by name. Presumably this is the root of the Chrome behavior here?

/cc @domenic @mikewest @annevk

@annevk
Copy link
Member

annevk commented Sep 28, 2016

I like the Chrome behavior... The less we share those names the better. (To define that better we still need a lot of refactoring I'm afraid.)

@mikewest
Copy link
Member

I'm surprised that Chrome opens two windows for rel="noreferrer" target="foo" (as I thought we only used that as a process-hopping heuristic for target="_blank", but that might have changed to "any new window"). I'm not surprised by the rel="noopener" case, although the discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=651579 suggests that I should be.

I think we should change step 4 of https://html.spec.whatwg.org/#dom-open to check the noopener behavior and push things into a new window regardless of targeting rules. I'll send a PR that we can argue about.

I wouldn't be sad if we did the same for noreferrer.

@annevk
Copy link
Member

annevk commented Sep 30, 2016

I think we should keep noreferrer and noopener in sync where possible. Do you have compatibility concerns with that?

@mikewest
Copy link
Member

Nope. It's apparently less work for me to do things that way, in fact.

mikewest added a commit that referenced this issue Sep 30, 2016
WIP: All of this would benefit from more substantial refactoring, but
let's decide that this is the right way to approach things first.

#1826
mikewest added a commit that referenced this issue Oct 5, 2016
WIP: All of this would benefit from more substantial refactoring, but
let's decide that this is the right way to approach things first.

#1826
@bzbarsky
Copy link
Contributor Author

I landed the Gecko changes based on what we are planning to do here, plus web platform tests. It sure would be nice to update the spec to match the tests. ;)

@bzbarsky
Copy link
Contributor Author

@mikewest @annevk Any chance of actually getting the spec updated to what we all agreed on and getting Chrome fixed?

@annevk
Copy link
Member

annevk commented Apr 23, 2017

I can't realistically commit to a timeframe for that. As far as I can tell the way navigation/history/loading/etc. works is rather broken and refactoring it to match implementations (or at least what they should aspire too) is a hard problem for me. I tried a few times and got stuck on edge cases each time. I want to try again, but there's a lot of other things to work on too, so...

@bzbarsky
Copy link
Contributor Author

I mean, I can write the spec update I want. The real questions are:

  1. Will that get merged?
  2. Will Chrome actually bother to implement?

Because if the answer to either one is "no", then I should just back out the changes to noreferrer behavior I made, and try to figure out whether noreferrer and noopener should actually behave the same wrt link targeting or not, with some lovely reverse-engineering.

// cc @domenic

@annevk
Copy link
Member

annevk commented Apr 24, 2017

cc @RByers for the interop question. We'd take a PR that clarifies this if there's agreement (and tests).

@annevk
Copy link
Member

annevk commented May 4, 2017

This is basically a duplicate of #313, no? I still can't commit to a timeframe, but I do have a much better handle on this now and am making some modest progress towards fixing this situation. Help with tests and such appreciated.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 4, 2017

This is basically a duplicate of #313, no?

No, because #313 is about how "familiar with" behaves, whereas this bug is about the noopener/noreferrer case, which applies even in same-origin situations and hence is not affected by the "familiar with" restrictions.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 4, 2017

After some thought, and given total lack of response from @mikewest or @RByers, I am just going to undo the main incompatible change to noreferrer behavior we made in Gecko.

In terms of spec text, the Gecko behavior after my change will basically be:

  1. In the noopener case, skip step 4 of https://html.spec.whatwg.org/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
  2. In the noreferrer case, perform step 4 as usual.
  3. In both cases, when creating a new browsing context in step 5 create it in a separate unit of related browsing contexts, such that it can't be targeted by name.

So if you do window.open('', 'foo') and then do <a rel="noreferrer" target="foo"> the link will target the already-open window.

@annevk
Copy link
Member

annevk commented May 4, 2017

@bzbarsky why, compat? Making noreferrer/noopener the same seems much better.

@mikewest
Copy link
Member

mikewest commented May 4, 2017

Sorry I haven't been paying attention to window targeting. I'll try to be more responsive on the bugs Anne's been pinging today. :)

I think I would be happier if noreferrer and noopener had the same effect on targeting. Is the difference to Chrome's behavior causing compat issues for Firefox? If y'all are getting complaints, it's something I can try to prioritize changing (though, I suppose if y'all are getting a lot of complaints, it might be tougher to change than I'd have expected).

@RByers
Copy link

RByers commented May 4, 2017

Sorry I missed this also. I don't have context in this space to speak to the details, but unsurprisingly I support any effort that'll increase interop - even if it means taking a little compat risk in chromium. I'll chat with Mike offline a bit.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 4, 2017

why, compat?

Yes. See https://bugzilla.mozilla.org/show_bug.cgi?id=1358469

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 4, 2017

Specifically, everyone has for years been shipping behavior for noreferrer that is interoperable when the named target already exists. When it doesn't exist, interop is horrible, of course. But the "target already exists" case is interoperable inasmuch as any named targeting is interoperable, and is the only way to do no-referrer loads until everyone implements referrer policy. And people are using it.

@mikewest
Copy link
Member

mikewest commented May 5, 2017

@bzbarsky: Skimming through this again, I think html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html is the test you added way back when we were discussing this; https://codereview.chromium.org/2864613003 should bring Blink into line with Gecko's behavior on that test once it's cleaned up and landed.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented May 5, 2017

I think html/semantics/links/links-created-by-a-and-area-elements/htmlanchorelement_noopener.html is the test you added

Yes.

https://codereview.chromium.org/2864613003 should bring Blink into line with Gecko's behavior

Could you please summarize what behavior is actually being implemented there, for both noopener and noreferrer? The tests here are very much not exhaustive (e.g. doesn't test noreferrer), so agreement on them clearly doesn't mean very much in practice.

Again, I am backing out some of the changes I made to Gecko's noreferrer behavior to allow it to target an existing named thing (but the thing it creates will still not be targetable). See https://bugzilla.mozilla.org/show_bug.cgi?id=1358469 and the change I just landed there, which includes wpt changes.

@annevk
Copy link
Member

annevk commented Feb 5, 2018

Currently in Chrome/Safari TP these links

<a href=https://example.com/ target=hi>start</a>
<a href=https://example.com/ target=hi rel=noreferrer>noreferrer</a>
<a href=https://example.com/ target=hi rel=noopener>noopener</a>

all end up in a new browsing context, until "start" is clicked at which point they all reuse the same.

In Firefox "noopener" is always new. "noreferrer" opens a new one that cannot be reused, unless "start" was clicked first in which case "noreferrer" behaves the same as "start".

In Edge if you start with "noreferrer" everything will then open a new browsing context. If you start with "start" or "noopener" the browsing context will always be reused. (Bonus points for doing something unexpected I suppose.)

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Feb 5, 2018

Does Edge even support noopener? Because I thought part of the point of noopener was to explicitly create a new unit of related browsing contexts...

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Feb 5, 2018

https://caniuse.com/#search=noopener says Edge in fact does not support it.

@annevk
Copy link
Member

annevk commented Feb 5, 2018

Sure, but that's not what's unexpected. What's unexpected is, e.g., clicking "noreferrer" and then clicking "start" twice gives you three new browsing contexts.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Feb 5, 2018

Oh. Yeah, that's odd. ;)

alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This feature has some outstanding issues and feature requests; see whatwg#323
and whatwg#1826. But this editorial cleanup creates a more solid foundation
for future work.
@annevk
Copy link
Member

annevk commented Jan 23, 2019

@cdumez @mikewest ping.

I'll note that the HTML Standard currently lets name targeting win over creating a new browsing context (matches Chrome/Safari), but that this is easily fixed (the name matching step would need to also check noopener being false).

@annevk
Copy link
Member

annevk commented Jan 24, 2019

Given the compatibility issue in #1826 (comment) I guess we should go with the Chrome/Safari status quo. Reuse existing window if it exists, create a new one (that cannot be targeted if noopener/noreferrer is set) otherwise.

However, note that for window.open() only Safari has that behavior. There Chrome and Firefox do open a new window for noopener and not target an existing frame or some such. That seems less than ideal as it would mean more diverging code paths.

@bzbarsky
Copy link
Contributor Author

I would really prefer we did the more-secure thing for noopener, especially since it seems to be web-compatible enough (Firefox is shipping it, after all). That would also avoid the problem of diverging codepaths, for links and window.open, I would hope.

But that would obviously need buy-in from Chrome and Safari.

@cdumez
Copy link

cdumez commented Jan 24, 2019

Btw, I believe I made changes in this area so it’d be good to confirm Safari behavior using a recent STP. I believe link targeting no longer works when noopener/noreferer is specified.

@annevk
Copy link
Member

annevk commented Jan 24, 2019

@cdumez in release 73 name targeting happens first: https://morning-tarp-1.glitch.me/.

@cdumez
Copy link

cdumez commented Jan 24, 2019

If Blink people are on board with trying this, I'd be willing to give it a try in WebKit too. I personally like Firefox's behavior better here but there are some risks associated with changing our behavior (https://bugzilla.mozilla.org/show_bug.cgi?id=1358469#c0).

@cdumez
Copy link

cdumez commented Jan 24, 2019

Wait, I take back what I said. I just tried Firefox and link targeting fails with noopener but works with noreferrer. The inconsistency is definitely not nice here, especially considering that noreferer is supposed to be a superset of noopener. I am guessing Firefox did this for compat reasons?

@cdumez
Copy link

cdumez commented Jan 24, 2019

Given the compatibility issue in #1826 (comment) I guess we should go with the Chrome/Safari status quo. Reuse existing window if it exists, create a new one (that cannot be targeted if noopener/noreferrer is set) otherwise.

I think I agree with this now. At least the behavior is consistent between noopener and noreferer. The inconsistency in Firefox and Chrome bothers me.

However, note that for window.open() only Safari has that behavior. There Chrome and Firefox do open a new window for noopener and not target an existing frame or some such. That seems less than ideal as it would mean more diverging code paths.

That's the part I thought I fixed recently in STP.

@bzbarsky
Copy link
Contributor Author

I am guessing Firefox did this for compat reasons?

Yes.

At least the behavior is consistent between noopener and noreferer.

Is that more important than sane behavior for noopener? With this behavior, if you do a noopener link maybe you'll actually get the severing you want, and maybe you won't, depending.

@annevk
Copy link
Member

annevk commented Jan 25, 2019

@bzbarsky we could advice folks to also use _blank. It doesn't really make sense to set the name of a browsing context you're not going to control anyway.

@bzbarsky
Copy link
Contributor Author

It depends. You might control it on the other side. That is, you may be opening a page you control with noopener (and its subframes might target it by name, etc) so that if the user later navigates to another tab there the new thing won't be able to reach back to you.

I mean, the way I see it we end up with a suboptimal inconsistency somewhere here. We just have to pick where...

@annevk
Copy link
Member

annevk commented Jan 28, 2019

That case seems more unlikely. Since if the user navigated there directly (or from search results or some such) it wouldn't have a name and things would fall apart. I agree that none of this is ideal though.

@annevk
Copy link
Member

annevk commented Jan 29, 2019

The way I think we should resolve this given all the information given thus far:

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Feb 3, 2019

I've lost track of what the "what the current specification accepts" behavior is. Is there a summary somewhere?

@annevk
Copy link
Member

annevk commented Feb 4, 2019

If the target attribute contains a name (i.e., not _self or _blank or some such), we'll try to find a browsing context with that name and use it. If that fails, we'll create a new top-level browsing context, using noopener/noreferrer to guide the decision as to whether it gets an opener.

@natechapin
Copy link

I've been trying clean up chromium's browsing context selection in the last month or so and stumbled across this issue. Is the current spec text sufficiently agreed upon that I should standardize chromium's behavior on it (i.e., noopener/noreferrer shouldn't ever affect browsing context selection by name)?

@annevk
Copy link
Member

annevk commented Apr 17, 2019

I think so, especially given that we cannot change it for noreferrer it makes sense to not do it for noopener either, to keep noreferrer as a superset.

@bzbarsky
Copy link
Contributor Author

I don't think "shouldn't ever affect" is quite correct, because the things actually opened via noopener/noreferrer, when it opens new things, can't be targeted later.

Anyway, I still think basically ignoring noopener on the load that we want to target for targeting purposes is very sad, but if that's what everyone else wants to implement...

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 5, 2019
…s for choosing a browsing con…, a=testonly

Automatic update from web-platform-tests
HTML: correct noopener tests

In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
--

wpt-commits: 0c79d0d99c720f679fa089374971778e461e6662
wpt-pr: 16451
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Jun 6, 2019
…s for choosing a browsing con…, a=testonly

Automatic update from web-platform-tests
HTML: correct noopener tests

In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
--

wpt-commits: 0c79d0d99c720f679fa089374971778e461e6662
wpt-pr: 16451
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…s for choosing a browsing con…, a=testonly

Automatic update from web-platform-tests
HTML: correct noopener tests

In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
--

wpt-commits: 0c79d0d99c720f679fa089374971778e461e6662
wpt-pr: 16451

UltraBlame original commit: f2b4dc0d863dcdd1768145f1fa83462cf9a9271c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…s for choosing a browsing con…, a=testonly

Automatic update from web-platform-tests
HTML: correct noopener tests

In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
--

wpt-commits: 0c79d0d99c720f679fa089374971778e461e6662
wpt-pr: 16451

UltraBlame original commit: f2b4dc0d863dcdd1768145f1fa83462cf9a9271c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…s for choosing a browsing con…, a=testonly

Automatic update from web-platform-tests
HTML: correct noopener tests

In particular, if a name is given use that first and potentially reuse a window.

Closes whatwg/html#1826.
--

wpt-commits: 0c79d0d99c720f679fa089374971778e461e6662
wpt-pr: 16451

UltraBlame original commit: f2b4dc0d863dcdd1768145f1fa83462cf9a9271c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: frames/navigables/browsing contexts topic: navigation
Development

No branches or pull requests

8 participants
@mikewest @cdumez @RByers @bzbarsky @annevk @geoffreygaren @natechapin and others