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

Set aria-disabled instead of disabled on Button / SubmitButton #325

Closed
chawes13 opened this issue Mar 27, 2019 · 6 comments · Fixed by #481
Closed

Set aria-disabled instead of disabled on Button / SubmitButton #325

chawes13 opened this issue Mar 27, 2019 · 6 comments · Fixed by #481
Assignees
Labels
a11y Accessibility
Milestone

Comments

@chawes13
Copy link
Contributor

Disabling a button can be confusing for users as it generally doesn't provide information as to why it is disabled. Additionally, it removes the button from the tab order so users of assistive technologies may miss it altogether, depending on their style of navigation and the implementation of the AT.

Instead of setting disabled, we could set aria-disabled="true", which would allow the button to be focused and would alert screen readers that it was disabled. Styling could target this attribute instead of disabled. Additionally, we could force a noop when invalid is true OR we could force developers to handle the errors and tell users why the form failed.

References:

@chawes13 chawes13 added the idea label Mar 27, 2019
@dpikt
Copy link
Contributor

dpikt commented Mar 28, 2019

I'm down with this. In generally, we disable buttons much more often then we have to- potentially we could fix this issue simply in the way we render these buttons.

For instance, right now we often write:
<SubmitButton {...{ pristine, invalid, submitting }}
which disables the button when the form is pristine or invalid.
But by writing:
<SubmitButton {...{ submitting }}
we would avoid the disabling behavior. This has the advantage of allowing the form to submit, which shows sync errors. Also, disabling the form when pristine doesn't seem to have much upside to me- at best, we avoid an extraneous API call but the UX is kinda confusing.

@dpikt
Copy link
Contributor

dpikt commented Mar 28, 2019

What do you think about making this change to usage? Any potential downsides you see?

@chawes13
Copy link
Contributor Author

I definitely agree with your comment about the pristine disabling behavior. I like the idea of changing this usage, but I could see a downside in terms of enforcing it without updating the component.

I'm in favor of removing the behavior to set disabled based on these props, instead forcing a developer to set disabled manually when they truly want to disable something. However, this would likely be considered a "breaking change".

@dpikt
Copy link
Contributor

dpikt commented Mar 28, 2019

Yeah that's most definitely a breaking change. I feel like I could go either way on it. The behavior is arcane enough that if we removed it from any boilerplate (form component snippet) it probably wouldn't persist in any new apps moving forward. I guess the question is whether we want to retrofit our old apps- but that could also occur without a library change.

If we go for it, we should make sure to get any important minor upgrades in before it as well.

@chawes13
Copy link
Contributor Author

Can we mark this as a topic for discussion at the next priority meeting?

@dpikt
Copy link
Contributor

dpikt commented Apr 2, 2019

@chawes13 sounds good!

@dpikt dpikt added a11y Accessibility and removed idea labels Apr 11, 2019
@dpikt dpikt added this to the v4 milestone Apr 19, 2019
@chawes13 chawes13 linked a pull request Sep 16, 2021 that will close this issue
5 tasks
@chawes13 chawes13 self-assigned this Sep 16, 2021
@chawes13 chawes13 mentioned this issue Jun 23, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants