Admin notices accessibility

[joedolson]

This PR is an alternative to #11923.

This PR only attempts to handle notices that are passed through wp_get_admin_notices(), with the intention that all core admin notices should do this. This simplifies the code by not attempting to capture all legacy variants that a notice could conceivably appear in.

It doesn't capture JS produced notices, but those already need to be handled separately.

It doesn't attempt to render the notices in a different location in PHP, keeping the existing DOM relocation, but wrapping the notices that are moved in an aside to provide better navigation.

This doesn't include a conditional skip link to navigate to those, although that could be added.

It also doesn't add ARIA roles into the PHP produced notices - these should not be there; ARIA is only necessary for empty notices where information will be inserted or modified with JS.

This should be considered a draft, although reviews are welcome. It is not in any way ready for merge.

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

Use of AI Tools

AI assistance: Yes
Tool(s): GitHub Copilot
Model(s): Claude Sonnet
Used for: Initial code skeleton; final implementation was reviewed and significantly modified by me.
-->

---

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.

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

[joedolson]

@pattonwebz @afercia This is a rough draft of some alternative implementation for notices. Not thoroughly tested at this phase, but feel free to hammer at it.

[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.

[afercia]

I left some comments.

[afercia]

I'd consider to wrap this in a named function (with a docblock), for clarity. Then call the function at the bottom of the wrapping IIFE where other functions are called e.g. setPinMenu();.

[joedolson]

Seems reasonable. This code is quite old, and I don't see any evidence that there was a reason for it to be run where it is.

[afercia]

I understand this class and method have been designed for a generic implementation but why it is building an object with the markup, message, and args? To my understanding, they are never used. If I'm not missing anything, the $notices array is only used to count the notices.
It so, there is no need to get the markup/message/args. We could also consider to not use the wp_admin_notice_markup filter and use the wp_admin_notice action hook instead.

[joedolson]

I was hypothesizing that this could also be used to build out a list of notices that could be triggered, e.g. from the admin bar, to be able to browse notices. That's not something that's critical to implement in this PR, but I was experimenting with the implementation.

The wp_admin_notice action isn't viable, because it doesn't catch anything that uses a echo wp_get_admin_notice() or $message = wp_get_admin_notice() pattern.

[afercia]

Some more thoughts:

So far, this PR only works for notices that are printed via wp_admin_notice() / wp_get_admin_notice().

Other notices will not be collected. For example all the ones in the Settings pages that are rendered via settings_errors(). #13669 would help with this because it refactors settings_errors() to use wp_admin_notice() internally. It can be merged into this PR for testing purposes and tested on the Settings pages.

There are other hooks that are used to render notices. As far as I can tell, they don't pass arguments, they are just hooks:

network_admin_notices
user_admin_notices
admin_notices
all_admin_notices (appears to not be used in Core, unless I'm missing something)

As such, the notice can only be collected if the callback actually uses wp_admin_notice() internally. For example these are in various places in Core and will need to be checked:

add_action( 'admin_notices', 'default_password_nag' ); -> for exampe, this one uses `wp_admin_notice()` internally.
add_action( 'admin_notices', 'new_user_email_admin_notice' );
add_action( 'admin_notices', 'update_nag', 3 );
add_action( 'admin_notices', 'deactivated_plugins_notice', 5 );
add_action( 'admin_notices', 'paused_plugins_notice', 5 );
add_action( 'admin_notices', 'paused_themes_notice', 5 );
add_action( 'admin_notices', 'maintenance_nag', 10 );
add_action( 'admin_notices', 'wp_recovery_mode_nag', 1 );
add_action( 'admin_notices', array( 'WP_Privacy_Policy_Content', 'notice' ) );
add_action( 'admin_notices', array( 'WP_Privacy_Policy_Content', 'policy_text_changed_notice' ) );
add_action( 'user_admin_notices', 'new_user_email_admin_notice' );
add_action( 'network_admin_notices', 'new_user_email_admin_notice' );
add_action( 'admin_notices', 'site_admin_notice' );
add_action( 'network_admin_notices', 'site_admin_notice' );
add_action( 'network_admin_notices', 'update_nag', 3 );
add_action( 'network_admin_notices', 'maintenance_nag', 10 );

Also the bundled themes use these hooks, for example, twenty twemty one:
add_action( 'admin_notices', 'twenty_twenty_one_upgrade_notice' );

Extenders may not use wp_admin_notice() at all.

Suggest for testing: test multiple notices on the same page.
For example, add here one more notice:

add_settings_error( 'general', 'settings_testnotice', __( 'Notice test.' ), 'warning' );

Test saving on one of the Settings pages:

This PR	With #13669 merged

[afercia]

After merging #13669 I tested a few more cases. All seem to be collected and counted correctly.

A notice rendered directly in the middle of a page e.g. in the Reading Settings page:

wp_admin_notice(
	'My notice at the bottom of the page',
	array(
		'type'               => 'warning',
		'additional_classes' => array( 'inline' ),
	)
);

Two other notices hooked into hooks that fire late on a page:

add_action(
	'in_admin_footer',
	function () {
		wp_admin_notice(
			'My notice in the in_admin_footer hook',
			array(
				'type'               => 'warning',
				'additional_classes' => array( 'inline' ),
			)
		);
	},
	1
);

add_action(
	'admin_footer-options-reading.php',
	function () {
		wp_admin_notice(
			'My notice in the admin_footer-options-reading.php hook',
			array(
				'type'               => 'warning',
				'additional_classes' => array( 'inline' ),
			)
		);
	},
	1
);

Other findings reported by @pattonwebz especially the 4th one on https://core.trac.wordpress.org/ticket/50486#comment:55 need to be tested more in depth.

[joedolson]

This intentionally doesn't capture any notices that aren't passed through wp_get_admin_notice(). The intent is that all WordPress core notices should be updated to pass through the filter, so that we're internally consistent, and that these accessibility benefits would be for people using WordPress API paths to generate notices. Those have been available since version 6.4 now, and all plugins should be able to migrate to them.

In my opinion, directly printing a notice is _doing_it_wrong(), and we shouldn't need to support it. There are far too many possible ways that can be done to be reasonable to try and catch.

[afercia]

The wp_admin_notice action isn't viable, because it doesn't catch anything that uses a echo wp_get_admin_notice() or $message = wp_get_admin_notice() pattern.

Yes. It won't catch anything echoed when using wp_get_admin_notice() directly.

However, we can try to add a new hook inside wp_get_admin_notice(). Then, try to detect whether the notice is actuallly echoed later and try to dedupe potential duplicates.

[joedolson]

I'm not sure I see the value of a new hook. Can you specify how you see a duplicate happening with this?

All calls to wp_admin_notice() call wp_get_admin_notice(), but no call to the getter should also call the printing function. The only case I'm thinking of where this would have a duplicate would be if a notice was created, never printed, then created a second time to be printed. In that case, the problem is that the caller is doing things wrong, and should be fixed.

I can appreciate wanting to try to catch duplicates, I'm just not seeing how what you propose would do that.

[afercia]

I'm not sure I see the value of a new hook. Can you specify how you see a duplicate happening with this?

Nevermind, I think I understand a little better now.

[afercia]

One more thing:
to my understanding, the id in wp_get_admin_notice() isn't a real unique ID. It may be empty or whatever it is passed to.

For future extensibility, should we consider to make wp_get_admin_notice() generate a unique ID and add it to:

• a new argument passed to the wp_admin_notice_markup filter (or in the existing $args)
• a data-* attribute in the notice markup

This way, any notice generated by wp_get_admin_notice / wp_admin_notice would carry a unique identifier. That would be useful for any future extension e.g. a notification center.

[joedolson]

Updates add a unique ID to all generated notices and key the array to those IDs; I think the future potential for a notification center, or even just making that an easily plugged extension, is definitely worthwhile.

I don't think that the issue about renderers is very significant; when we implemented wp_admin_notices(), we were pretty thorough in catching the output that was created. I believe that the only significant outliers are JS generated notices and the settings API errors, that you've already got a separate PR for.

[afercia]

I left some minor comments.

[afercia]

One more thing worth considering. It's not strictly related to this PR and can be considered in a follow-up.

As far as I can tell, the 4 admin notice types 'error', 'success', 'warning', 'info' are not 'enforced' anywhere. They are only mentioned in the inline documentation.

I would consider adding a const in the most appropriate class (the collector?) to have a canonical source for the types so that it can be referenced when needed e.g.:

const NOTICE_TYPES = array( 'error', 'success', 'warning', 'info' );

then when in other parts of the codebase there is the need to get the Core notice types, this would be all that is needed

ClassName::NOTICE_TYPES;

[joedolson]

I can see some argument for having a canonical reference of notice types, but I agree that it's beyond the scope of this PR - it doesn't directly impact the accessibility of notices, just the stability of usage.

[afercia]

LGTM

[peterwilsoncc]

While updating the tests yesterday to account for the new ID related attributes, I noticed that the data set an ID that is not a string does not include the id or data-id attributes. The invalid value is simply dropped.

If the argument passed is incorrect, I think the default attributes should be used in its place.

https://github.com/joedolson/wordpress-develop/blob/129d943956923e14fa42cea0aca4da23bc26aa9e/tests/phpunit/tests/functions/wpGetAdminNotice.php#L188-L194

[github-actions]

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

SVN changeset: 64282
GitHub commit: 1c277e1

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

[t-hamano]

Let's push an additional commit to fix this to the correct version.

[joedolson]

I could have sworn I'd gotten all of those... apparently not!

[Mamaduka]

I'm seeing title prefixes on the Dashboard and when editing a post, even though no notices are visible.

Screenshots

[t-hamano]

@Mamaduka #14223