Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#43290 closed defect (bug) (fixed)

Add rel="noopener" to target="_blank" links by default in menu walker

Reported by: audrasjb's profile audrasjb Owned by: pento's profile pento
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.1
Component: Menus Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

WordPress core previously introduced rel="noopener" attribute to target="_blank" links –which is fine– but I think menu items should also add this attribute to target-blank links.

I know there is already an optional "Link Relationship (XFN)" text input but 1) It's optional and tricky for unexperimented users ; 2) I think we should add rel="noopener" attribute by default when "Open link in a new tab" option is checked and "Link Relationship (XFN)" input is empty.

Related: #43280 , #37941

Attachments (3)

43290.diff (762 bytes) - added by audrasjb 7 years ago.
adds default noopener value to menu items with target blank attribute
43290.2.diff (825 bytes) - added by audrasjb 7 years ago.
Adds both noopener and noreferrer values and refreshes the patch against trunk
43290.3.diff (2.6 KB) - added by welcher 6 years ago.
Adds some unit tests

Download all attachments as: .zip

Change History (22)

@audrasjb
7 years ago

adds default noopener value to menu items with target blank attribute

#1 @audrasjb
7 years ago

  • Keywords has-patch added

#2 @welcher
7 years ago

@audrasjb thanks for the ticket and the patch.

My only thought here is that we should also be adding noreferrer as well to keep our approach consistent with #37941

#3 @welcher
7 years ago

  • Owner set to audrasjb
  • Status changed from new to assigned

#4 @welcher
7 years ago

  • Milestone changed from Awaiting Review to 5.0

@audrasjb
7 years ago

Adds both noopener and noreferrer values and refreshes the patch against trunk

#5 @audrasjb
7 years ago

You're alright @welcher
43290.2.diff adds both values now and is refreshed against trunk.

Cheers,
Jb

Last edited 7 years ago by audrasjb (previous) (diff)

#6 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#7 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

Patch needs review and a decision.

#8 @welcher
6 years ago

  • Owner changed from audrasjb to welcher

Patch looks good to me.

@pento is it too late to get this into 5.1?

It would make sense to get this and #43280 into core at the same time.

Last edited 6 years ago by welcher (previous) (diff)

This ticket was mentioned in Slack in #core by welcher. View the logs.


6 years ago

#10 @pento
6 years ago

  • Keywords needs-unit-tests added

Thanks for the ping, @welcher!

As this is an enhancement, (though a small one), we're already past the deadline for getting it committed. I like the idea, I think it's reasonable to commit it for 5.2.

In the mean time, it'd be nice to have some unit tests for this behaviour.

@welcher
6 years ago

Adds some unit tests

#11 @welcher
6 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#12 @audrasjb
6 years ago

  • Keywords commit added

43290.3.diff looks great on my side. Thanks for the unit-tests!

#13 follow-up: @afercia
6 years ago

As mentioned on #46421, I'd suggest to investigate on the actual usefulness of noreferrer:

#14 in reply to: ↑ 13 @audrasjb
6 years ago

Replying to afercia:

As mentioned on #46421, I'd suggest to investigate on the actual usefulness of noreferrer:

As mentionned on #43280 I think both of the rel attributes should be added, in order to keep some consistency with https://developer.wordpress.org/reference/functions/wp_targeted_link_rel/

If we want to remove noreferrer, I believe we should remove it from that function first ;)

Last edited 6 years ago by audrasjb (previous) (diff)

#15 @audrasjb
6 years ago

  • Type changed from enhancement to defect (bug)

Changing ticket type to Defect/bug. See https://core.trac.wordpress.org/ticket/43280#comment:18 for context.

#16 @welcher
6 years ago

@afercia I would lean towards keeping them both in place.

Both for consistency as @audrasjb points out, and also as these menus are shown on the front end nd I don't believe the patch in [41741] addresses that use case.

#17 @audrasjb
6 years ago

I think we are good to land in 5.2 here :-)

#18 @pento
6 years ago

  • Owner changed from welcher to pento

#19 @pento
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 45141:

Menus: Add rel="noopener" to target="_blank" links by default in menus.

This expands upon rel="noopener" being previously added to links in the content.

Props audrasjb, welcher.
Fixes #43290.

Note: See TracTickets for help on using tickets.