Make WordPress Core

Opened 5 months ago

Closed 2 months ago

Last modified 2 weeks ago

#60685 closed defect (bug) (fixed)

Keyboard focus order mismatch in adminbar in front-end

Reported by: joedolson's profile joedolson Owned by: joedolson's profile joedolson
Milestone: 6.6 Priority: normal
Severity: normal Version: 3.3
Component: Toolbar Keywords: has-patch has-dev-note
Focuses: accessibility Cc:

Description

When viewing the Admin bar on the front-end, the search tool is added visually in the far right corner, after the profile menu. In the DOM, however, the search bar is located before the profile menu.

These should be reversed in the DOM so that keyboard order matches visual order.

See r19518.

Attachments (2)

60685.diff (1.4 KB) - added by sabernhardt 4 months ago.
removes float: right and increases priority number for the Search node
60685.9999.diff (1.4 KB) - added by sabernhardt 2 months ago.
sets Search menu priority at 9999

Download all attachments as: .zip

Change History (21)

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


5 months ago

#2 @joedolson
5 months ago

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

@sabernhardt
4 months ago

removes float: right and increases priority number for the Search node

#3 @sabernhardt
4 months ago

  • Keywords has-patch added; needs-patch removed

To keep the Search form to the right of other links without the float, the priority would need to change. I chose 20: more than recovery mode's 8, with more than one number in between for anyone who wants to have a link between the "Howdy" account links and the Search icon.

At least two of the plugins that add links to the secondary group have a priority of 999 (see directory search), so it may be appropriate to increase the priority more.

#4 @joedolson
3 months ago

I think the question here is what the intent is for plugins with a higher priority. Previously, with the float in place, it was impossible to add items to the right of the search without also customizing CSS. Now, everything will be ordered according to priority.

While some plugins may need to make changes, I'm not sure that's a significant concern. Nothing will actually break here as a result, a link may just appear in a different place than previously expected.

We could certainly increase the value to provide more space; though it's not strictly necessary - you can have multiple items in a single priority, after all.

Can you specify any of the plugins that have a priority of 999? I browsed those results a bit, but I didn't see them.

I think this is basically ready to commit; the only question is what the ultimate priority level should be. I'm inclined to think that 20 offers enough space, and that moving later doesn't really offer any significant advantages.

#5 @sabernhardt
3 months ago

Yeah, it is not so easy to search for 'parent' => 'top-secondary' and then to find where that specific node is added with the admin_bar_menu action.

For some additional priorities:

#6 @joedolson
3 months ago

Based on those values, with the patch as is, it would result in a change in the adminbar for all of those plugins.

I don't think there's anything we can do about the Polylang switch; we should just add that in a dev note and possible contact them directly with a heads up. It's a trivial change for them to make if needed.

But we can make this work for all of the other plugins, just by deciding that our aim is to have the search item as the last element of the adminbar, and giving it a priority of 9999, which would keep it at the end of the adminbar for the vast majority of cases.

#7 @joedolson
2 months ago

@sabernhardt I think it would be good to get this in sooner rather than later; what do you think about the proposal to just give it a very high priority?

#8 @sabernhardt
2 months ago

  • Keywords needs-dev-note added

I'm not sure about 9999, but that is probably a better choice than 20 (or a similarly low number). Maybe it's worth mentioning in the dev chat (agenda) first.

I tried a version-based priority to confirm that plugin authors could change it if they do not like where their links appear:

$special_priority = ( version_compare( $GLOBALS['wp_version'], '6.6-alpha', '>=' ) ) ? 2 : 100;

add_action( 'admin_bar_menu', 'wpadmin_toolbar_test_link', $special_priority );

function wpadmin_toolbar_test_link( $wp_admin_bar ) {
	$wp_admin_bar->add_node(
		array(
			'parent' => 'top-secondary',
			'id'     => 'mylink',
			'href'   => '#',
			'title'  => __( 'My Link' ),
		)
	);
}

@sabernhardt
2 months ago

sets Search menu priority at 9999

#9 @sabernhardt
2 months ago

I uploaded a patch to consider the 9999 priority.

#10 @joedolson
2 months ago

  • Keywords 2nd-opinion added

The value of 9999 feels quite arbitrary; but I think that any value here is ultimately pretty arbitrary. It's easy for extenders to get around, and in most cases will have the desired result.

We can run it by a dev chat, but it seems like a somewhat trivial issue to raise in a meeting. If we can just get a second opinion from another committer, I think that would be sufficient.

Pinging @audrasjb as the 6.6 tech lead.

#11 @audrasjb
2 months ago

  • Keywords 2nd-opinion removed

Thanks for the ping.
Honestly while this is indeed a arbitrarily high value, I think it makes sense in the context of something that shouldn't be overridden, except on rare contexts. And extenders can always override it if they want. To summary, I'm good with this value.

#12 @joedolson
2 months ago

  • Keywords commit added

Thanks, JB!

With that 2nd opinion, I think this is ready for commit.

#13 @joedolson
2 months ago

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

In 58215:

Toolbar: Accessibility: Fix keyboard focus order of search form.

Change the priority of the search form & remove float so that the visual position of the form matches it's positioning in the DOM.

This sets the priority of the search form to an arbitrarily high value of 9999 to ensure it will generally be last in the DOM.

Props joedolson, sabernhardt, audrasjb.
Fixes #60685.

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


5 weeks ago

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


5 weeks ago

#16 @joedolson
5 weeks ago

@sabernhardt Dev note for your review.

In [58215], the search input on the front end admin bar is added at a different priority. It was previously inserted at priority 4, then floated to appear at the end of the admin bar. It is now inserted at priority 9999, to load at the end of the admin bar without CSS manipulation.

Extenders placing admin bar nodes after the search or replacing core search should take the new priority into consideration.

### Assign different priority based on WordPress version

$priority = ( version_compare( $GLOBALS['wp_version'], '6.6', '>=' ) ) ? 4 : 9999;
add_action( 'admin_bar_menu', 'wpadmin_toolbar_test_link', $priority );
/**
 * Add a node to the WP admin toolbar.
 *
 * @param object $wp_admin_bar WP Admin Bar object.
 */
function wpadmin_toolbar_test_link( $wp_admin_bar ) {
	$wp_admin_bar->add_node(
		array(
			'parent' => 'top-secondary',
			'id'     => 'mylink',
			'href'   => '#',
			'title'  => __( 'My Link' ),
		)
	);
}

#17 @sabernhardt
5 weeks ago

  • Keywords commit removed

Thanks for writing the note. It's much easier for me to edit than to write.

A few small notes:

  • "Toolbar: Search has a much later priority" is one possible heading.
  • I recommend hyphenating "front-end admin bar", especially because the next sentence mentions the end of the admin bar (relating to its order).
  • To work in 6.6 alpha/beta/RC, I needed to change the version from 6.6 to 6.6-alpha. It may not make any difference for end users, but extenders may prefer that precision in the example.

#18 @pbiron
5 weeks ago

For posterity, here's a link to a comment I made on the Dev Note that covers the changes in this ticket

https://make.wordpress.org/core/2024/06/25/miscellaneous-developer-changes-in-wordpress-6-6/#comment-46841

#19 @sabernhardt
2 weeks ago

  • Keywords has-dev-note added; needs-dev-note removed

Follow-up: #61615

Note: See TracTickets for help on using tickets.