Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#61615 closed defect (bug) (fixed)

Toolbar: consider moving user menu to a higher priority (after most plugins)

Reported by: sabernhardt Owned by: hellofromTonya
Priority: normal Milestone: 6.6.1
Component: Toolbar Version: 6.6
Severity: normal Keywords: has-patch dev-reviewed commit fixed-major has-dev-note
Cc: Focuses:

Description

With [58215], the placement of admin bar items in the top-secondary group have a different visual order. The search form is moved to the end, but the user menu and recovery mode nodes from Core could be set a later priority so plugin links stay to the left of them.

@pbiron shared screenshots in a comment on the dev note. They show of a set of multiple secondary nodes in a 6.5.5 site and how they look in 6.6.

@joemcgill suggested increasing the priority of the user menu on Slack.

Attachments (1)

61615.diff (777 bytes ) - added by sabernhardt 2 years ago.
moves user menu and recovery mode nodes from priority levels 7 and 8 to 9991 and 9992

Download all attachments as: .zip

Change History (23)

@sabernhardt
2 years ago

moves user menu and recovery mode nodes from priority levels 7 and 8 to 9991 and 9992

#1 follow-up: @sabernhardt
2 years ago

I chose priorities close to the Search form's 9999 but wanted a few numbers for extenders to target a spot between them.

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


2 years ago

#3 @pbiron
2 years ago

thanx @sabernhardt. The patch looks good.

Having the nodes that core adds to top-secondary group more to the right (where they used to be, but now in DOM order) is good!

#4 @joedolson
2 years ago

  • Milestone Awaiting Review6.6.1
  • Owner set to joedolson
  • Status newaccepted

I'm going to go ahead and milestone this for 6.6.1. If it weren't so close to release, I'd want to get it in for 6.6, so it makes sense to me that this should be considered a bug.

#5 @pbiron
2 years ago

  • Keywords needs-patch added

#6 @pbiron
2 years ago

  • Keywords has-patch added; needs-patch removed

#7 in reply to: ↑ 1 @arthur791004
2 years ago

Is it possible to just reverse the order when rendering? Changing the order seems to be a breaking change and affects the existing behavior as well. For example, if a plugin adds the nodes to the admin bar in sequence, the order of those nodes will be changed after WP 6.6.

  • Before: | 3 | 2 | 1 | - due to the float: right
  • After: | 1 | 2 | 3 |

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


2 years ago

#9 @hellofromTonya
2 years ago

6.6.1 RC will likely happen late tomorrow, July 18th (no time is set yet).

During today's scrub, @joedolson noted to look at it later tonight for commit. On my review list for backporting to 6.6 branch.

#10 @joedolson
2 years ago

  • Keywords reviewing added

@arthur791004 I wouldn't consider a change in order a breaking change; there may be some special cases where the apparent order of items makes a significant difference, but mostly not. Reversing the order when adding nodes would result in a confusing and non-intuitive use of a common and standard WordPress API, so I think it's better that extenders update their code to accommodate the new order.

#11 @joedolson
2 years ago

  • Keywords commit added; reviewing removed

#12 @joedolson
2 years ago

  • Resolutionfixed
  • Status acceptedclosed

Commit action doesn't seem to have run.
=====

Toolbar: Move user and recovery menus to a higher priority.

Following [58215], admin bar items in the top-secondary group have a changed visual order. Increase the priority of the user and recovery menu items so nodes added with higher priorities will still be shown visually before the user and recovery menu items, as they were prior to 58215.
The items will appear in the reverse of the previous order, but the new order now matches their priority order, rather than being the opposite.

Props sabernhardt, joemcgill, pbiron, joedolson.
Fixes #61615.

Version 0, edited 2 years ago by joedolson (next)

#13 @joedolson
2 years ago

  • Keywords dev-feedback added; commit removed
  • Resolution fixed
  • Status closedreopened

Reopening for backport to 6.6.1.

#14 @hellofromTonya
2 years ago

  • Keywords dev-reviewed commit added; dev-feedback removed
  • Owner changed from joedolson to hellofromTonya
  • Status reopenedreviewing

[58748] LGTM for backport to the 6.6 branch for 6.6.1.

Doing backport commit now.

#15 @hellofromTonya
2 years ago

  • Resolutionfixed
  • Status reviewingclosed

In 58759:

Toolbar: Move user and recovery menus to a higher priority.

Following [58215], admin bar items in the top-secondary group have a changed visual order. Increase the priority of the user and recovery menu items so nodes added with higher priorities will still be shown visually before the user and recovery menu items, as they were prior to 58215.

The items will appear in the reverse of the previous order, but the new order now matches their priority order, rather than being the opposite.

Reviewed by hellofromTonya.
Merges [58748] to the 6.6 branch.

Props sabernhardt, joemcgill, pbiron, joedolson.
Fixes #61615.

#16 @hellofromTonya
2 years ago

  • Keywords fixed-major added

#17 @rabmalin
2 years ago

I have this in my theme:

function theme_slug_replace_howdy( $wp_admin_bar ) {
	$my_account = $wp_admin_bar->get_node( 'my-account' );
	$wp_admin_bar->add_node(
		array(
			'id'    => 'my-account',
			'title' => str_replace( 'Howdy,', 'Hello,', $my_account->title ),
		)
	);
}

add_action( 'admin_bar_menu', 'theme_slug_replace_howdy', 25 );

After 6.6.1 release I am getting following PHP warnings.

PHP Warning:  Attempt to read property "title" on null in /Users/nilambarsharma/Sites/staging/app/public/wp-content/themes/khai/functions.php on line 45
PHP Deprecated:  str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in /Users/nilambarsharma/Sites/staging/app/public/wp-content/themes/khai/functions.php on line 45

I noticed priority of this hook has been changed in the above commit. PHP notice is fixed when I keep priority 9999 for my custom hook in my theme. Just reporting here because I am not sure whether this issue should be fixed in core or in my theme.

#18 @narenin
2 years ago

Hi @rabmalin,

You are correct the same thing is already reported here.

https://core.trac.wordpress.org/ticket/61738

When these priorities changed we are getting null for the given node.

$wp_admin_bar->get_node( 'my-account' )


#19 @hellofromTonya
2 years ago

  • Keywords needs-dev-note added

As discussed in #61738, a dev note is needed to document this change and to provide guidance to extenders.

#20 @sabernhardt
2 years ago

My draft for the dev note seems ready for review.

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


2 years ago

#22 @hellofromTonya
2 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.