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

feat(button): allow tooltips on disabled buttons via disabledFocusable #14646

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Sep 13, 2023

Closes #8509
Related internal slack thread

This adds a new disabledFocusable prop to Button. It gives disabled styling to the button while allowing it to remain focusable.

This aims to solve a longstanding problem where users have no way to understand why disabled buttons (particularly IconButtons) are disabled.

I'm opening this as a draft for now to get feedback from the design team, IBMa and the community. I hope we can reach a consensus to merge this, but it's still possible this exploration could fail for a number of reasons.

Changelog

New

  • add disabledFocusable to button
  • add associated styling via new classes
  • add test coverage

Testing / Reviewing

  • Review the design elements, ensure all button variants render appropriately when given disabledFocusable.
    • Is the existing focus ring acceptable design?
  • The onClick event should not fire when given disabledFocusable

Open questions

These are sourced from conversations in Slack, GitHub, and Carbon team discussions.

  • Does the aria spec allow for this?
  • Can disabledFocusable be used on a button that is a link (has an href)?
  • If it can receive focus, do the disabled styles need to be updated to pass color contrast?
  • Is it confusing to have in the design system some disabled items be focusable and some aren't?
  • Would the recommendation be to make all disabled components use aria-disabled or something like disabledFocusable?
  • Is there any precedent for this elsewhere?

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit d0e9dd4
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6501d39fec1fbe0008905fbb
😎 Deploy Preview https://deploy-preview-14646--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Sep 13, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit d0e9dd4
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6501d39f35b8df0008628ad3
😎 Deploy Preview https://deploy-preview-14646--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones
Copy link
Member Author

Here's my thoughts on the open questions

Does the aria spec allow for this?

Yes

Unless otherwise stated, authors MAY use aria-* attributes in place of their HTML equivalents on HTML elements where the aria-* semantics would be expected. For example, authors MAY specify aria-disabled=true on a button element, while also implementing the necessary scripting to functionally disable the button, rather than the use disabled attribute.

Can disabledFocusable be used on a button that is a link (has an href)?

Nope, it's not allowed per the spec, and the implementation accounts for this.

If it can receive focus, do the disabled styles need to be updated to pass color contrast?

I don't think so.

The disabled styling definitely should be applied:

In addition to setting the aria-disabled attribute, authors SHOULD change the appearance (grayed out, etc.) to indicate that the item has been disabled.

Beyond this, I think adding aria-disabled and disabling click events for the button would qualify the button as "inactive" despite being focusable. Having focus doesn't mean the control is operable, so I think there would be no requirement for that to meet color contrast.

https://www.w3.org/WAI/WCAG21/Understanding/non-text-contrast.html

User Interface Components that are not available for user interaction (e.g., a disabled control in HTML) are not required to meet contrast requirements. An inactive user interface component is visible but not currently operable. An example would be a submit button at the bottom of a form that is visible but cannot be activated until all the required fields in the form are completed.

Is it confusing to have in the design system some disabled items be focusable and some aren't?

To me it feels less confusing than a disabled IconButton not having a way to convey why it's disabled.

Would the recommendation be to make all disabled components use aria-disabled or something like disabledFocusable?

No, disabled still has it's place. disabledFocusable would only be cases where the button needs to remain in the focus order (for a tooltip to be displayed, for instance).

There's some interesting points in this article though that suggest using disabled results in less inclusive buttons overall.

Is there any precedent for this elsewhere?

Yes, Fluent UI has options for both disabled and disabledFocusable: https://react.fluentui.dev/?path=/docs/components-button-button--default

I struggled for quite a while to find a good name for this prop (visuallyDisabled, partiallyDisabled, ariaDisabled, etc) and funnily enough landed on disabledFocusable before I looked around to see if other design systems had implemented something like this and saw FluentUI using the same name!

@tw15egan
Copy link
Collaborator

Should it read out that it is disabled? Or is that expected to be added in the description text of why it is disabled? Adding aria-disabled gets it to announce Dimmed, but not sure if that is enough

@tay1orjones
Copy link
Member Author

I'm looking into why the pagination previous buttons are no longer getting disabled styling...

image

@tay1orjones
Copy link
Member Author

After a lot of consideration and review, we're not going to land this for a variety of reasons.

I know this isn't the outcome many folks wanted, but our team is committed to evaluating the root issue and providing more robust guidance around disabled elements. This work will be aimed at extending the disabled states pattern already present on the website. This and more is outlined in the issue I opened today:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip doesn't show when button is disabled
2 participants