Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button component: link buttons should not use aria-disabled #62281

Closed
afercia opened this issue Jun 4, 2024 · 2 comments · Fixed by #63376
Closed

Button component: link buttons should not use aria-disabled #62281

afercia opened this issue Jun 4, 2024 · 2 comments · Fixed by #63376
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jun 4, 2024

Description

Discovered while checking #62080

The Button component accepts a href prop which is supposed to be an URL. When set, the Button renders an <a> element: a link with a proper href attribute.

However, when the Button component receives also the disabled and ;__experimentalIsFocusable props both set to true, the rendered link gets an aria-disabled="true" attribute.

That's not correct for two reasons:

  • It's invalid HTML: aria-disabled-true is invalid on an <a> element.
  • Conceptually incorrect: links can't be 'disabled'. Either they are links or not.

Weirdly enough, when setting __experimentalIsFocusable to false and keeping the href prop, the Button is rendered as a disabled <button> element.

Rendered invalid HTML example:

<a href="https://wordpress.org" aria-disabled="true" class="components-button">Code is poetry</a>

It is worth mentioning that links can't 'really' be disabled. Their real nature is to not be disabled. The defautl action can be prevented via JavaScript to make a click event do nothing but that's less than ideal. It's against the nature of a link.That's the reason why an aria-disabled="true" or disabled attribute on <a> elements are invalid HTML.

Step-by-step reproduction instructions

Screenshots, screen recording, code snippet

aria-disabled="true" invalid HTML

Screenshot 2024-06-04 at 15 02 56

disabled invalid HTML

Screenshot 2024-06-04 at 15 18 44

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jun 4, 2024
@afercia
Copy link
Contributor Author

afercia commented Jun 4, 2024

Note: I think this problem was discussed when first implementing the Button component but never fully addressed, nor I could find a specific issue about it.

@mirka
Copy link
Member

mirka commented Jun 4, 2024

I noticed this issue as well when testing edge cases in the TypeScript types for Button. For what it's worth, TypeScript will error on cases where a disabled is used together with href. But we should definitely fix the runtime behavior.

{ /* @ts-expect-error - although the runtime behavior will allow this to be an anchor, this is probably a mistake. */ }
<Button disabled __experimentalIsFocusable href="foo" />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
2 participants