Using target _blank is not safe

Hi @guardiola86, I am reviewing the #2 patch. Thank you

Hi @guardiola86, Getting this when I apply the #2 patch.

Lenovo@LAPTOP-PDE747K8 MINGW64 /c/xampp/htdocs/drupal-9/web/modules/contrib/link_attributes-3299750 (3299750-using-target-blank)
$ git apply -v using-target-blank-is-not-safe-3299750-2.patch
Checking patch src/Plugin/Field/FieldWidget/LinkWithAttributesWidget.php...
Applied patch src/Plugin/Field/FieldWidget/LinkWithAttributesWidget.php cleanly.

+++ b/src/Plugin/Field/FieldWidget/LinkWithAttributesWidget.php
@@ -151,6 +151,10 @@ class LinkWithAttributesWidget extends LinkWidget implements ContainerFactoryPlu
+      if ($values[$delta]['options']['attributes']['target'] == '_blank') {
+        $values[$delta]['options']['attributes']['rel'] = 'noopener';

We should check there's not already a value set in case someone wants to override it manually

Interesting learning! I didn't know this yet. Cool!

Agree with #6 and this should be done as MR, please.

@guardiola86 @anoopsingh92 would you do that?

I have done @Anybody, Please review the Merge request !6
Thanks

@anoopsingh92 thank you! Still needs the additional check from #6.

Added an empty check, please review.

Thanks @Grevil, looks good! I think to be 100% safe we should have a simple test here for both values to be present, if target="_blank" is selected.

If that goes green, RTBC from my side. Code LGTM already!

@Anybody, I agree! Furthermore, we should allow testing inside issues, as the first patch applied here failed.

Done, please review, tests should be green now.

Testing seems to work with patch files here manually, not with MR's. Let's try! :)

Code LGTM and tests pass, thank you @Grevil! RTBC! Let's see what @larowlan says.

This is looking good, can we add an extra test case for when target is blank, but a value of rel is already set?

Thanks!

@larowlan totally makes sense! @Grevil could you add that one please? Shouldn't be too hard anymore, I hope.

Done, please review!

Nice work @Grevil, this proves that the value is "nofollow" and even with target="_blank" is not "noopener"!
The comment is also important here. Thanks!

FYI: https://developer.chrome.com/docs/lighthouse/best-practices/external-anc...

As of Chromium version 88, anchors with target="_blank" automatically get noopener behavior by default. Explicit specification of rel="noopener" helps protect users of legacy browsers including Edge Legacy and Internet Explorer.

So changing this to minor. @larowlan should decide if we should do this anyway or keep it as it was before and let the browsers do their work.

Perhaps @guardiola86 could check this for the major browsers: Firefox, Safari, Edge?

@larowlan as of #22 I'd vote to set this won't fix and leave it to the browsers.
If you'd like to add this anyway, it's RTBC'd and fine, I think.

Yes, if we can confirm Firefox is the same, I'd prefer to close this as is and avoid the extra complexity

Yes, I can confirm, that Firefox also has this feature implemented in their 79 release! See https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/79#html

Setting target="_blank" on <a> and <area> elements implicitly provides the same behavior as also setting rel="noopener" [...]

I guess I will set this "RTBC" to confirm that this can be closed?

Thanks @Grevil, so I'll set it won't fix as of #24

Thanks!