Add support for tooltips for metabox controls

[joedolson]

This PR wraps the existing buttons in markup for existing styles and scripts, but doesn't change the function signature to support adding all of the custom markup.

It may be desirable to add a modified path to inject this markup into existing patterns.

This could be done similar to wp_get_admin_notice(), which has a bunch of parsers for attributes, but I think this isn't appropriate here. Open to ideas, but I was thinking of something that accepted existing button markup, used the HTML API to inject the additional button markup, then accepted a string for tooltip text.

I don't think this is needed for toggletips, but for existing markup needing tooltips, it may be helpful.

Trac ticket: https://core.trac.wordpress.org/ticket/50921

Use of AI Tools

None

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joedolson, peterwilsoncc, afercia, jorbin, wildworks.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[peterwilsoncc]

In the dashboard, the move up tooltip is showing for the top widget and the move down button showing in the last position.

If the widget can't be moved in the direction (the arrow gets greyed out) I think it would be good to hide the tool tip too.

[joedolson]

That's an interesting case, and bears some considering. As a non-operable but focusable control, it should be different in some way, but I'm not sure that's the best option.

[t-hamano]

This could be done similar to wp_get_admin_notice(), which has a bunch of parsers for attributes, but I think this isn't appropriate here. Open to ideas, but I was thinking of something that accepted existing button markup, used the HTML API to inject the additional button markup, then accepted a string for tooltip text.

I think this is a good idea, but if we are to ship it, we should address it during the 7.1 Beta release cycle at the very least. Once a function is exposed, it becomes increasingly difficult to change its parameters. If we are to expose an API for tooltips in 7.1, I believe we need to consider all scenarios now and make the API robust.

[peterwilsoncc]

Even greyed out, I find it a little confusing to get the tool tip on the up arrow. Maybe that's a different bug and the arrow shouldn't be displayed?

A difference I am seeing is that unavailable up arrow has a hover effect on this branch, whereas there is none on trunk.

[joedolson]

I wouldn't expect to see it on trunk; this code isn't present on trunk.

In my opinion, sighted users should get more or less the same information that non-sighted users get. Non-sighted users get focus on the control, the full name of the control, and the information that it's disabled. I'm not sure whether it makes sense to just eliminate the tooltip for disabled controls - there could still be scenarios where it's necessary to know which control something is.

@afercia Any opinion here?

[afercia]

I'm currently on mobile and looking into the code isn't an option for me now. However I'd like to stress a couple things.

First: the mover buttons shouldn't get disabled with a disabled attribute. Instead, they should use aria-disabled and be noop-ed. Otherwise, when keyboard users reach the first or last position, they will face a focus loss. Gutenberg uses this pattern in many places, to avoid focus losses.

Using aria-disabled also helps discoverability for non-sighted users. And given these buttons would be focusable, I think they should show the tooltip. Again, it's the same pattern used in Gutenberg.

[joedolson]

@afercia The mover buttons don't get the disabled attribute; nothing has changed there, however, they do get aria-disabled, as their functionality is not relevant.

What you said essentially agrees with me; any control that is focusable should have a valid tooltip to explain it. The problem is in the question of how to indicate via the tooltip that it's a disabled control, and whether we need to do so.

I believe that Gutenberg does not have any indication in the tooltip that a control is not functional; they're the same in all cases. In that case, the most consistent option is to use the same expectation in core, which means removing the visual differences for non-functional controls and leaving it the same as other controls.

[afercia]

I agree we should use the same expectation and pattern used in Gutenberg.

[afercia]

Crafting an accessible name that fits the different needs is always a difficult choice.
Changing it from Move up to Move %s box up may be more descriptive for screen reader users but it makes things more difficult for voice command users.

The current implementation uses both an accessible name and an accessible description for that reason. For example:

• Name: "Move uo"
• Description: "Move Site Health Status box up"

As a voice command user, guessing the name from the visual appearance of the icon is way easier this way. As a screen reader user, I get additional info from the description.

Instead, for a voice command user, guessing that the acc name is Move Site Health Status box up is way more difficult and prone to errors.

Granted, if we name the button "Move up", there will be several buttons with the same name on the same page. However, as a voice command user I would prefer to issue a voice command Click move up (so that the software would show the number overlays) and them for example seven rather than having to guess and issue a command like Move WordPress Events and News box up.

Also, as a user, I'm not interested in learning it's a box. I just want to move it.

[joedolson]

We've tended to do a lot of creative button naming to keep buttons having unique names, which is a thing specifically not required in WCAG, and I'm not convinced the excess verbosity is worth it; but as a default kept the more verbose option prior to review.

For screen readers, I think that there is already good context - the buttons lie right after a heading that provides the name of the metabox. If we want to increase context, we could add role="region" to all metaboxes and copy the heading to an aria-label. so that the context is announced as soon as the widget is entered. This would also add metaboxes as navigable landmarks, which I think could be useful.

[afercia]

Can this string be moved to the default arg?
I think also the ID can be moved to the related arg?

[afercia]

Regarding the button, so far the wp-tooltip implementation assumes the element the tooltip applies to is always a button. This may not always be the case.

I'm planning to create a Trac ticket to consider tooltips for 'icon only' links. One of the most evident cases is about the 'List view' and 'Grid view' of the Media Library:

I think the tooltip implementation gives us the opportunity to reconsider those parts of the UI that we are most accustomed to and that at first glance we do not consider problematic but which in fact may be problematic for some users. Those two links are a perfect case. There may be more.

Would this be a good opportunity to change the tooltip helper so that it can accept a link markup?

[joedolson]

The popovertarget attribute isn't allowed anywhere but on button and input type="button", which makes extending this to links difficult. Do you have thoughts about how that might work?

[joedolson]

I'm not resolving this conversation, because the second part isn't addressed; but the first is.

[joedolson]

Related: https://core.trac.wordpress.org/ticket/65660

[peterwilsoncc]

I wouldn't expect to see it on trunk; this code isn't present on trunk.

Sorry, I was unclear.

The comment I made was about the hover effect showing on the up arrow (^) -- it changes colour on hover in the feature branch, not trunk -- rather than the tooltip itself. All good if it's intentional.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joedolson, peterwilsoncc, afercia, wildworks.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joedolson, peterwilsoncc, afercia, wildworks.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[joedolson]

Status of this PR:

• Extends the wp-tooltip API to support passing HTML directly into the function as an option. This makes it easier to add a tooltip to existing controls without needing to add extra arguments and handlers in the helper function to generate relevant attributes.
• Extends the wp-tooltip API to support generating tooltips on links.
• Restricts the API to only accept button or a elements. This helps enforce the popover contract and best practice accessibility by disallowing usage on non-conforming HTML.
• Enqueues the wp-tooltip.css in the global admin, but only enqueues wp-tooltip.js as a dependency for postbox, the only place it's currently in use on the back-end.

Would like to consider enqueuing it globally, as it's a light-weight script and the usage is intended to be extended.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (6)

src/wp-includes/general-template.php:494

• $args['id'] and $args['button'] are checked using truthiness and the (possibly custom) $button HTML is later concatenated into a sprintf() format string. This can mis-handle valid IDs like "0" and can also break output (or emit warnings) if custom button markup contains a literal "%" (e.g. inline styles like "width:100%"), because sprintf() will treat it as a format specifier. Consider using explicit empty-string checks and escaping percent signs in custom markup before running it through sprintf().

	$icon      = ( $args['icon'] ) ? trim( $args['icon'] ) : $defaults['icon'];
	$id        = ( $args['id'] ) ? $args['id'] : $defaults['id'];
	$button    = ( $args['button'] ) ? $args['button'] : $defaults['button'];
	$processed = false;
	$processor = new WP_HTML_Tag_Processor( $button );

src/wp-admin/includes/template.php:1375

• $box['id'] and the class string are output into HTML attributes without escaping. Meta box IDs/titles can come from plugins, so this increases XSS risk, especially now that this line is being modified anyway. Escaping the attribute values here would make the output robust.

					echo '<div id="' . $box['id'] . '" class="postbox ' . postbox_classes( $box['id'], $page ) . $hidden_class . '" ' . ' role="region" aria-label="' . esc_attr( wp_strip_all_tags( $box['title'] ) ) . '">' . "\n";

src/wp-admin/includes/template.php:1403

• The move-up button's aria-describedby currently points at the postbox container ID. That can cause screen readers to treat the entire metabox content as the description, which is likely unintended. It should point to the tooltip text element ("-text") or be removed.

						$move_up_button = '<button type="button" class="handle-order-higher" aria-describedby="' . $box['id'] . '">
							<span class="screen-reader-text">' . __( 'Move up' ) . '</span>
							<span class="order-higher-indicator" aria-hidden="true"></span>
						</button>';

src/wp-admin/includes/template.php:1413

• The move-down button's aria-describedby currently points at the postbox container ID, which can cause an overly-verbose/incorrect accessible description. It should point to the tooltip text element ("-text") or be removed.

						$move_down_button = '<button type="button" class="handle-order-lower" aria-describedby="' . $box['id'] . '">
							<span class="screen-reader-text">' . __( 'Move down' ) . '</span>
							<span class="order-lower-indicator" aria-hidden="true"></span>
						</button>';

src/wp-admin/includes/template.php:1423

• The show/hide button's aria-describedby currently points at the postbox container ID, which can result in an overly-verbose/incorrect accessible description. It should point to the tooltip text element ("-text") or be removed.

						$show_hide_button = '<button type="button" class="handlediv" aria-expanded="true" aria-describedby="' . $box['id'] . '">
							<span class="screen-reader-text">' . __( 'Show or hide panel' ) . '</span>
							<span class="toggle-indicator" aria-hidden="true"></span>
						</button>';

src/wp-admin/css/wp-tooltip.css:32

• The :has() guard only checks for disabled button elements. Since the tooltip toggle can now be an (wp_get_tooltip_helper and wp-tooltip.js), an aria-disabled="true" toggle link would still get hover/focus styling. Consider checking the .wp-tooltip__toggle element itself rather than only button[disabled]/button[aria-disabled].

.wp-tooltip:not(:has(button[aria-disabled="true"],button[disabled])) .wp-tooltip__toggle:hover,
.wp-tooltip:not(:has(button[aria-disabled="true"],button[disabled])) .wp-tooltip__toggle:focus,
.wp-tooltip .wp-tooltip__close:hover,
.wp-tooltip .wp-tooltip__close:focus {
	color: var(--wp-admin-theme-color, #3858e9);
}

.wp-tooltip:not(:has(button[aria-disabled="true"],button[disabled])) .wp-tooltip__toggle:focus,
.wp-tooltip .wp-tooltip__close:focus {

[aaronjorbin]

The only concern I see is with the integration on metaboxes. Currently, it has the aria-describedby pointing to the entire metabox.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-describedby says

Note: The aria-describedby content should only be a text string. If there are important underlying semantics in the content, consider using aria-details.

I will note I didn't test this with any AT, so this concern may not be valid.

[joedolson]

I did test that with AT, and it worked consistently... but I also agree that this is not really totally according to spec. It's possible it worked because of buggy implementation, where the presence of an aria-label was calculated as the accessible name, even though it's actually just a landmark name in this context, and doesn't replace the containing content.

If that's the case, I don't think we should depend on a potentially out-of-band behavior by AT. It wouldn't be difficult to add a comparable ID to the contained heading, and use that instead.

[aaronjorbin]

It wouldn't be difficult to add a comparable ID to the contained heading, and use that instead.

That works for me. An alternative idea would be to use aria-description and add a string along the lines of "Widget controls for $title metabox"

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[joedolson]

aria-description has very poor support, unfortunately - I just re-tested it for another PR related to visual revisions: WordPress/gutenberg#80440 (comment)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[aaronjorbin]

Looks Good to me!

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (8)

src/wp-includes/general-template.php:533

• $button is concatenated into the sprintf() format string. If callers pass custom button markup containing % (common in inline styles, URLs, etc.), sprintf() will treat it as a format directive, which can trigger warnings or corrupt output. Treat the button HTML as an argument to sprintf() (not part of the format string) and resolve any internal placeholders (%2$s, %3$s, %4$s) before formatting the wrapper markup.

		$markup = sprintf(
			'<span class="%1$s">
				' . $button . '
				<span popover="hint" id="%2$s" class="wp-tooltip__bubble" role="tooltip">' .
					'<span id="%2$s-text" class="wp-tooltip__text">%5$s</span>' .

src/wp-includes/general-template.php:552

• Same sprintf() format-string issue as the tooltip branch: concatenating $button into the format string means any % in custom markup can be interpreted by sprintf(). Resolve the intended placeholders on $button first, then pass the button HTML as an argument to sprintf() for the wrapper markup.

		$markup = sprintf(
			'<span class="%1$s">
				' . $button . '
				<span popover="auto" id="%2$s" class="wp-tooltip__bubble" role="dialog" aria-label="%3$s" tabindex="-1" autofocus>' .
					'<span id="%2$s-text" class="wp-tooltip__text">%5$s</span>' .

src/wp-includes/general-template.php:489

• $icon now falls back to the default icon when the caller passes an empty string, which removes the previously-supported ability to intentionally omit the icon by passing icon => ''. Since wp_parse_args() already applies the default, this can be simplified while preserving empty-string overrides.

	$icon      = ( $args['icon'] ) ? trim( $args['icon'] ) : $defaults['icon'];
	$id        = ( $args['id'] ) ? $args['id'] : $defaults['id'];
	$button    = ( $args['button'] ) ? $args['button'] : $defaults['button'];

src/wp-admin/includes/template.php:1375

• This adds a new $box['id'] output context in an HTML attribute. $box['id'] can be provided by plugins via add_meta_box(), so it should be escaped when printed to avoid attribute-injection issues.

					echo '<div id="' . $box['id'] . '" class="postbox ' . postbox_classes( $box['id'], $page ) . $hidden_class . '" ' . ' role="region" aria-label="' . esc_attr( wp_strip_all_tags( $box['title'] ) ) . '">' . "\n";

src/wp-admin/includes/template.php:1378

• The new heading id attribute is built from $box['id'] and output unescaped. Since meta box IDs are plugin-provided, escape before printing to avoid attribute-injection issues.

					echo '<h2 class="hndle" id="' . $box['id'] . '-title">';

src/wp-admin/includes/template.php:1400

• aria-describedby is built from $box['id'] and output unescaped inside HTML. Escape the meta box ID before concatenating into an attribute value.

						$move_up_button = '<button type="button" class="handle-order-higher" aria-describedby="' . $box['id'] . '-title">

src/wp-admin/includes/template.php:1410

• aria-describedby is built from $box['id'] and output unescaped inside HTML. Escape the meta box ID before concatenating into an attribute value.

						$move_down_button = '<button type="button" class="handle-order-lower" aria-describedby="' . $box['id'] . '-title">

src/wp-admin/includes/template.php:1420

• aria-describedby is built from $box['id'] and output unescaped inside HTML. Escape the meta box ID before concatenating into an attribute value.

						$show_hide_button = '<button type="button" class="handlediv" aria-expanded="true" aria-describedby="' . $box['id'] . '-title">

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 62816
GitHub commit: 54b41a8

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.