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

Link: Allow explicit rel="opener" #1428

Open
wants to merge 2 commits into
base: master
from

Conversation

@sstur
Copy link

sstur commented Sep 10, 2019

We shouldn't blindly set noopener on all links because there's a valid use case to open a new tab that can communicate back using postMessage()

Also, this cleans up the string concatenation so that we don't get extraneous leading space or duplicate values.

@sstur sstur changed the title Links: Allow explicit rel="opener" Link: Allow explicit rel="opener" Sep 10, 2019
Copy link
Owner

necolas left a comment

We shouldn't blindly set noopener on all links because there's a valid use case to open a new tab that can communicate back using postMessage()

It's not done "blindly", and if you have use cases you should explain them.

if (component === 'a' && domProps.target === '_blank' && domProps.rel !== 'opener') {
const existingRel = domProps.rel ? domProps.rel.split(' ') : [];
// Ensure that we don't end up with duplicates.
const newRel = new Set([...existingRel, 'noopener', 'noreferrer']);

This comment has been minimized.

Copy link
@necolas

necolas Sep 10, 2019

Owner

Creating a Set, array, then flattening it is unnecessary overhead on a hot path

This comment has been minimized.

Copy link
@sstur

sstur Sep 10, 2019

Author

OK, I guess I thought this was the simplest way to prevent the duplicates which I saw in my DOM when I was using this. I don't know that it's a bug necessarily to have duplicates but I'm not sure that it's well defined in the spec what should happen if multiple conflicting or duplicates are in the rel attribute. I'll revert this change for a more performant one.

@sstur

This comment has been minimized.

Copy link
Author

sstur commented Sep 10, 2019

OK, sorry. What I meant by "blindly" was without letting the user opt-out. I might have used a poor word choice.

A simple use case would be opening a tab on the same domain that we would need to communicate back to the original tab.

Another case might be an oAuth flow where you open a tab to ask the user to go login using Google/GH/etc then that tab lands back on a same-origin page that sends a postMessage to window.opener.

I think noopener is a super wise default but we should probably be able to opt out because this essentially introduced a breaking change when I upgraded RNW and one that seems not to be able to be worked around.

if (relList.indexOf('noreferrer') === -1) {
relList.push('noreferrer');
}
domProps.rel = relList.join(' ');

This comment has been minimized.

Copy link
@sstur

sstur Sep 10, 2019

Author

@necolas, how's this? We have only one array allocation now. Want me to keep it in pure string land?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.