feat(a11y): add disabled label for a11y on buttons [AC-4127]#1341
feat(a11y): add disabled label for a11y on buttons [AC-4127]#1341Stefan3002 wants to merge 1 commit intocanonical:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for providing an assistive-technology-only “disabled reason” description for ActionButton when it is disabled, so screen readers can announce why an action is unavailable.
Changes:
- Introduces a new
disabledReasonLabelprop onActionButton. - Links the button to an off-screen description via
aria-describedbyusing a generated id (useId). - Conditionally renders an off-screen element containing the disabled reason text.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| aria-describedby={ | ||
| disabled && disabledReasonLabel | ||
| ? `disabled-reason-label-${buttonId}` | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
The new aria-describedby value will be overridden if callers pass their own aria-describedby via buttonProps (because {...buttonProps} comes last). Consider merging any existing buttonProps["aria-describedby"] with the disabled-reason id so both descriptions are preserved when disabledReasonLabel is provided.
| aria-describedby={ | ||
| disabled && disabledReasonLabel | ||
| ? `disabled-reason-label-${buttonId}` | ||
| : undefined | ||
| } |
There was a problem hiding this comment.
New disabledReasonLabel behavior isn’t covered by tests. Add coverage to verify (1) the off-screen description is rendered only when disabled + disabledReasonLabel are provided and (2) the button gets an aria-describedby pointing at that element.
|
Tests will be added after we agree this approach is viable and allright. |
e048970 to
71de6af
Compare
edlerd
left a comment
There was a problem hiding this comment.
We tend to not use the aria-describedby and a hidden div but a bit more simple: Add a title to the button with the reason. That makes the reason also discoverable to anyone on mouse over.
If you also use this approach, we probably don't need the changes proposed here. Wdyt @Stefan3002 ?
+1, I think it'll be better to use a |
|
@jmuzina and @edlerd thanks for taking the time to review this! From my research, I can see it is not enough to use title. It seems to be ignored in some cases by AT. Refs:
So, I think that if, "We tend to not use the aria-describedby and a hidden div", we should. At elast for this use case to begin with. What are your thoughts on this @edlerd and @jmuzina ? 🤗 Thanks again! |
|
I've referred this a11y best practices question to @paul-geoghegan for his thoughts! I would suggest to avoid using a Tooltip as it would complicate usage if the consumer already uses a tooltip on this button, and would introduce a dependency from Button to Tooltip. I would lean towards using an |
|
This is a great discussion! So yes using title can be problematic but mainly because it's an optional screen reader feature so not everyone will have it on. You should never have screen reader only information though. If you need to convay why something is disabled then you should be doing it for everyone which means you could just set that element as the describing element of the button. I have never seen this done anywhere else for that exact same reason. It just feels like we would be introducing a new prop that is pretty much doing what aria-describedby already does. |
|
@paul-geoghegan Hey there! I think we should only consider it for a11y reasons as a tooltip will clutter the UI and it might be unpredictable when the message is too long. What do you think about displaying it in a visually hidden way and linking it with describedby? CC: @jmuzina |
|
@Stefan3002 I just don't understand why we would want to tell assistive technology users something and not sighted users. If it was the other way around I would be saying we should present the info to the screen reader users so I think if it's important enough to be convayed to screen reader users then it's important enough to be convayed to everyone. |
|
Thanks for your insights @paul-geoghegan ! After thinking on this some more - Guaranteeing the disabled label is created may not be something we can solve in the core component implementation label, but rather something we expect from our consumers with documentation on best practice on how to use the native HTML props they already have. Could we remove Example: // Inside ActionButton.tsx
<button
{...buttonProps}
aria-describedby={buttonProps['aria-describedby']}
aria-disabled={isDisabled || undefined}
>
{children}
</button>// inside a consumer of ActionButton
<ActionButton
disabled={formDisabled}
aria-describedby={formDisabled ? disabledLabelId : undefined}
onClick={() => { /* Save logic */ }}
>
Save Changes
</ActionButton>
{formDisabled && (
<p
id={disabledLabelId}
role="status"
>
Some generic text that describes why the button is disabled....
</p>
)}This way, the label is presented the same to AT and visually, we don't need to introduce a nonstandard prop, and we keep the flexibility of allowing the consumer more control how the label is styled, what other aria attributes it gets, etc. I think it'll be more approachable anyway this way because it's closer to the HTML-native way to do this, what do we think? |
|
@jmuzina sorry, I re-read it and they are not tied together. I personally think this is probably the best option. That way if it's needed then it can easily be implemented by the end user. |
Done
QA
Pinging @canonical/react-library-maintainers for a review.
Storybook
To see rendered examples of all react-components, run:
QA in your project
from
react-componentsrun:Install the resulting tarball in your project with:
QA steps
Percy steps
Fixes
Fixes: #AC-4127