Button: Never apply aria-disabled to anchor

[mirka]

Fixes #62281

What?

Prevents link (anchor) buttons from ever having an aria-disabled attribute.

Why?

<Button href="foo"> automatically renders an anchor element, whereas <Button href="foo" disabled> will remain a button element, because anchor elements cannot be disabled. Neither disabled nor aria-disabled are valid attributes on an anchor.

However, there was a bug where <Button href="foo" disabled accessibleWhenDisabled> rendered an anchor with an aria-disabled attribute.

Testing Instructions

See the Storybook for Button and see what happens when you render <Button href="foo" disabled accessibleWhenDisabled>.

✅ New unit test should also pass.

[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: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>

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

[github-actions]

Flaky tests detected in f1ddeaa.
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/9879092432
📝 Reported issues:

• [Flaky Test] Navigates from items list to preview via TAB, and vice versa #62434 in /test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js

[DaniGuardiola]

Makes sense to me and work as intended!

Random idea for discussion, tangential to this PR, but do you think it'd be possible to use an Ariakit button underneath? We might be able to delegate many of these behaviors (like accessibleWhileDisabled), plus get a few potential safeguards or a11y features out of the box and reduce our maintenance area. Not sure if there'd be any significant trade-offs.

[DaniGuardiola]

Makes sense to me and work as intended!

Random idea for discussion, tangential to this PR, but do you think it'd be possible to use an Ariakit button underneath? We might be able to delegate many of these behaviors (like accessibleWhileDisabled), plus get a few potential safeguards or a11y features out of the box and reduce our maintenance area. Not sure if there'd be any significant trade-offs.

[DaniGuardiola]

Github decided to duplicate my review 😆

[tyxla]

Makes sense. Thanks for making this behavior clearer through a unit test 👍

[ciampo]

We might be able to delegate many of these behaviors (like accessibleWhileDisabled), plus get a few potential safeguards or a11y features out of the box and reduce our maintenance area. Not sure if there'd be any significant trade-offs.

That is an alluring idea, although Button is such a complex and ubiquitous component that we'd need to do extensive testing, both automated (behavioral and visual) and human, before making such a change. I wonder if it just makes more sense to think about a v2 of Button instead — although likely neither of those projects would be high priority.

[mirka]

Basically agree with @ciampo. But my instinct is that Ariakit is not going to make a big difference here, because most of the complexity of Button is in the prop bloat and the styles 😅