Opened 5 years ago
Closed 4 years ago
#49558 closed task (blessed) (fixed)
Remove noreferrer from wp_targeted_link_rel and other uses
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.6 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-unit-tests has-patch commit |
Focuses: | Cc: |
Description (last modified by )
When we added noopener noreferrer
in #37941, the noreferrer
bit was added specifically because at the time, Firefox didn't support noopener
. Since it does now and has for a while, see here, I think we should remove it, as it does have nasty side effects: it breaks cross-site analytics.
We should remove it everywhere, as links in the admin don't send a referrer anyway after [41741] and as such there's no security risk to removing it.
Attachments (3)
Change History (22)
#1
@
5 years ago
- Milestone changed from Awaiting Review to 5.5
- Owner set to SergeyBiryukov
- Status changed from new to accepted
#3
@
5 years ago
- Keywords needs-patch needs-unit-tests late added
Yes, I very much think this of benefit to the WP and its users.
However, the WP browser support policy still includes Edge 18 as it has over 1% usage at the time of writing.
Let's keep an eye on the Can I Use browser usage stats and gleefully commit this once the EdgeHTML Edge browser use drops below the WP support levels.
My hunch is that it will still make the 5.5 milestone, so I am keeping milestone unchanged.
#4
follow-up:
↓ 5
@
5 years ago
- Description modified (diff)
- Milestone changed from 5.5 to 5.6
It looks like Edge 18 still has 1.49% usage, so this cannot be addressed in time for 5.5, moving to 5.6.
#5
in reply to:
↑ 4
@
5 years ago
Replying to SergeyBiryukov:
It looks like Edge 18 still has 1.49% usage, so this cannot be addressed in time for 5.5, moving to 5.6.
I've been waiting for removing the tag and I think it should be fixed asap, not delaying it!
It is causing problems with affiliate tracking and making WordPress users lose earnings.
And in order to remove it, we need to switch each block with a link to HTML view and manually remove the tag.
#6
@
4 years ago
- Keywords has-unit-tests has-patch added; needs-patch needs-unit-tests late removed
Given Edge is less than 1% market share now (0.77% - September 1, 2020), let's get rid of this attribute value.
This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.
4 years ago
#8
@
4 years ago
- Keywords needs-refresh added
Failed to apply patch on trunk, one unit test file affected.
#9
@
4 years ago
- Keywords needs-refresh removed
Well done. The patch seems pretty solid, I have tested it and there were some failing unit tests that I fixed in the last patch
#10
@
4 years ago
- Keywords commit added
Thanks @Mista-Flo !
Patch looks good to me as well. Marking for commit
.
#12
follow-up:
↓ 14
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
I'm suddenly getting failures running npm run test:php
on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it? I've shared a bit of the output below.
There were 22 failures: 1) Tests_Targeted_Link_Rel::test_add_to_links_with_target_blank Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'<p>Links: <a href="/" target="_blank" rel="noopener">No rel</a></p>' +'<p>Links: <a href="/" target="_blank" rel="noopener noreferrer">No rel</a></p>' /var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:12 2) Tests_Targeted_Link_Rel::test_add_to_links_with_target_foo Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'<p>Links: <a href="/" target="foo" rel="noopener">No rel</a></p>' +'<p>Links: <a href="/" target="foo" rel="noopener noreferrer">No rel</a></p>' /var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:18 3) Tests_Targeted_Link_Rel::test_target_as_first_attribute Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'<p>Links: <a target="_blank" href="#" rel="noopener">No rel</a></p>' +'<p>Links: <a target="_blank" href="#" rel="noopener noreferrer">No rel</a></p>' /var/www/tests/phpunit/tests/formatting/WPTargetedLinkRel.php:24 4) Tests_Targeted_Link_Rel::test_add_to_existing_rel Failed asserting that two strings are identical. --- Expected +++ Actual
#13
@
4 years ago
Weird @antpb I just set my git up to date with trunk and unit tests from this file are all passing. Do you have a special configuration?
#14
in reply to:
↑ 12
;
follow-up:
↓ 15
@
4 years ago
- Type changed from enhancement to task (blessed)
Replying to antpb:
I'm suddenly getting failures running
npm run test:php
on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it?
The tests are passing on Travis, could you run npm run build
just to make sure the build
directory is updated?
#15
in reply to:
↑ 14
@
4 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Replying to SergeyBiryukov:
Replying to antpb:
I'm suddenly getting failures running
npm run test:php
on my local that is up to date with trunk. They seem similar to the changes from this ticket. @SergeyBiryukov is this patch possibly causing it?
The tests are passing on Travis, could you run
npm run build
just to make sure thebuild
directory is updated?
Thanks @SergeyBiryukov. My build was stale when I noticed this. And this is why it's important to stop when you're tired. 😆
#16
follow-ups:
↓ 18
↓ 19
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?
#17
@
4 years ago
@wpwebdevj yes. @SergeyBiryukov would this require opening a new ticket on the Gutenberg GitHub?
#18
in reply to:
↑ 16
@
4 years ago
Replying to wpwebdevj:
I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?
Hi, yes it definitely make sense I think, but the block editor project is handled by the Gutenberg team in its own Github repo, not in trac, so you need to open an issue on it, and link to this trac ticket for reference.
#19
in reply to:
↑ 16
@
4 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Replying to wpwebdevj:
I have tested version 5.6 and the block editor is still adding noreferrer to newly created links with target. Now that noreferrer has been removed from wp_targeted_link_rel, doesn't it make sense to also remove it from the block editor?
Thanks for catching that!
Looks like there is already a Gutenberg issue for this: https://github.com/WordPress/gutenberg/issues/26914, so I'm closing the ticket as fixed for the core part.
Also related: #47202.
Seconded.