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

Crbug 1060691 name noopener #5779

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ParisMeuleman
Copy link
Contributor

@ParisMeuleman ParisMeuleman commented Aug 4, 2020

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …

(See WHATWG Working Mode: Changes for more details.)


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 8:00 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Parsing MDN data...
Parsing...



If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@annevk
Copy link
Member

annevk commented Aug 4, 2020

Ah, I was thinking this might not be web compatible, but this is a twist on the agreement reached at #1826 (comment). We would still reuse a named browsing context if it exists, but if we create something new, we would not use the name. I like it. Please also test this with noreferrer as we don't want that to diverge from noopener.

(You'll also need to join the googlers organization or make your membership thereof public.)

@ParisMeuleman
Copy link
Contributor Author

ParisMeuleman commented Aug 5, 2020

Thanks Anne for having a look! I didn't expect that now since it is in WIP.

Ah, I was thinking this might not be web compatible, but this is a twist on the agreement reached at #1826 (comment). We would still reuse a named browsing context if it exists, but if we create something new, we would not use the name. I like it. Please also test this with noreferrer as we don't want that to diverge from noopener.

Thanks for the pointers on the previous issue! I agree that noreferrer should not diverge from noopener if possible, if we agree on a behavior, I'll make sure to have both noopener and noreferrer. I did not fully realize the potential impact highlighted there, i.e. with non-top-level docs. Indeed I'd have expected noopener and noreferrer to imply a change of top level BCG, but I get why noreferrer is (was) useful there.

  • I think that this 'twist' should not impact frames.

  • I reckon however that It will have an impact when targeting an existing popup:

    • IIUC, currently the popup is reused if it is initially opened without noopener/referrer, and the name is not cleared. It is reusable multiple times.
      e.g. The following snippet would open only one popup:
      open("/a", "A");
      open("/b","A","noopener");
      open("/c","A","noopener");
      I believe the behavior is different if we don't have a frame with the target's name i.e. the following opens two separate popups:
      open("/d","B","noopener");
      open("/e","B","noopener");
    • With the change suggested in this PR, the first reuse will occur and then the others ones would open a new window. In the example above that would result in a first popup with /b, and an empty name, and a second popup with /c and an empty name.

IMO that seems an acceptable change in behavior.
IMO2 that's not perfect, I'd think it would be better if open("/b","A","noopener"); behaved as if the target was _blank, but achieving this would not be web compatible as discussed in #1826.
I also like that COOP would not induce a different behavior here anymore (beyond forcing the noopener).

(You'll also need to join the googlers organization or make your membership thereof public.)

Good point, I got removed since I wasn't interacting with the GH org's repos.

@annevk
Copy link
Member

annevk commented Aug 5, 2020

I don't see how your behavior for /b and /c would happen as step 7 of "The rules for choosing a browsing context" would end up setting chosen and step 8 would never run.

@ParisMeuleman
Copy link
Contributor Author

I don't see how your behavior for /b and /c would happen as step 7 of "The rules for choosing a browsing context" would end up setting chosen and step 8 would never run.

Indeed my bad, sorry about the confusion...
I believe the impact becomes quite minimal here and I'll push this forward for review.

Gets me wondering if this is enough in the COOP case though:

In the presence of a cross-origin opener policy, nested documents that are cross-origin with their top-level browsing context's active document always set noopener to true.

This statement might not be true if we end up reusing a BCG right? Could this become a problem?
Am I missing something in step 7 that would prevent this behavior? I reckon we'd have familiar-with, but then I guess it depends on what related enough really means?
(I'll open an issue if we agree there's one there)

@annevk
Copy link
Member

annevk commented Aug 6, 2020

You reuse a top-level browsing context, but to be able to reuse it one would first have to be able to create one and I don't see how that would work if COOP is in effect. (Edit: except if both use same-origin as policy, but that would also mean that upon navigating elsewhere it would get replaced.)

@ParisMeuleman
Copy link
Contributor Author

ParisMeuleman commented Aug 6, 2020

You reuse a top-level browsing context, but to be able to reuse it one would first have to be able to create one and I don't see how that would work if COOP is in effect. (Edit: except if both use same-origin as policy, but that would also mean that upon navigating elsewhere it would get replaced.)

Yep, I had the edit case in mind. But I guess the situation is different for a new top-level browsing context because the origin inherited by the initial empty document would be the iframe's.

Base automatically changed from master to main January 15, 2021 07:57
@annevk
Copy link
Member

annevk commented Apr 28, 2021

@ParisMeuleman are you still pursuing this? I think it would be a good change to make still. (If name doesn't exist within the browsing context group, then don't create a named browsing context.)

@ParisMeuleman
Copy link
Contributor Author

Yep I still think this change makes sense. Sorry I lost track of that PR. I'll have a fresh look at it and open that PR proper in the coming days.

@ParisMeuleman
Copy link
Contributor Author

@annevk, unfortunately I will go on leave for quite a bit (months) very soon, I'll reassign the chromium bug to someone in our team to follow this up.
Note that I also had the Chromium implementation drafted: https://chromium-review.googlesource.com/c/chromium/src/+/2317229

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

Successfully merging this pull request may close these issues.

2 participants