ToolbarButton: Always keep focusable when disabled

[mirka]

Fixes #57701

What?

Keeps disabled ToolbarButtons focusable in all instances.

Why?

Toolbar/menu items are one of the UI elements that are generally recommended to keep focusable when disabled.

Testing Instructions

Play around with the Storybook code for the ToolbarButtons in the Toolbar story. Passing disabled should keep the button focusable.

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

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: jeryj <jeryj@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

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

[mirka]

I don't foresee a need for it, but technically a consumer can override this by passing accessibleWhenDisabled={ false }. This is currently undocumented on our end, but we could document it if it ever becomes a need.

[github-actions]

Flaky tests detected in a5b11eb.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9792483814
📝 Reported issues:

• [Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

[ciampo]

🚀

[tyxla]

LGTM 👍

Noting that in 406ef77 which I added to #63107 I'm suggesting to use the new stable prop, but this won't be necessary if this PR lands first - instead we'll have to remove the experimental prop usage.

[mirka]

Noting that in 406ef77 which I added to #63107 I'm suggesting to use the new stable prop, but this won't be necessary if this PR lands first - instead we'll have to remove the experimental prop usage.

Good catch, thanks! I guess I'll do a cleanup & normalization pass in a separate PR. I've seen some aria-disabled and accessibleWhenDisabled usage here and there that should be cleaned up too.

[talldan]

I noticed that since this PR was merged there seem to be some styling changes in the block toolbar.

Before:

After:

It's possible (very likely) the block toolbar styles are implemented incorrectly, so it might not be the fault of this PR, but thought I'd mention it.

[mirka]

@talldan Is there a special repro condition for that? This is what I'm seeing at current trunk 8b470e0

Default

All locked

[talldan]

Oh, sorry, for some reason I didn't include any 🤦

If you edit any page in the site editor, and try selecting a template part block, the toolbar shows what it does in my comment.

[mirka]

Ah ok, got it. That color is easy to fix, but when we take consistency and accessibility into consideration, I'm not sure if/how we should fix it 🤔 For example, it is in fact visibly grayed out in the "All locked" case I posted.

I do think the button needs to remain focusable, but if so we need to visually show that it's disabled. Maybe something like this?

CleanShot.2024-07-09.at.19.42.31.mp4

But all in all, I'm leaning toward predictability/consistency, which is to visually gray out as usual when a button is disabled. It's potentially confusing to even a sighted mouse user if an enabled-looking button is unresponsive. What do you think?