-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(a11y): disabled button behavior and color contrast #65
Conversation
083bae2
to
a972159
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much easier to review the changes if you review by commit.
@@ -115,4 +119,37 @@ export const AsADownloadLink: Story = { | |||
}, | |||
}; | |||
|
|||
const FormWithSubmitButton = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a story to demonstrate the usage of Button
s in a form.
@@ -133,6 +127,7 @@ const StylessButton = React.forwardRef<React.ElementRef<"button">, ButtonProps>( | |||
// Ref: https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled | |||
const handleClick = (event: React.MouseEvent<HTMLButtonElement>) => { | |||
if (disabled) { | |||
event.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one-liner to fix the submission issue.
// Type tests | ||
// ------------------------------ | ||
|
||
// @ts-expect-error - Button with `danger` variant cannot be disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the proposed change to the danger
and info
variant, we will get a TS error if disabled
is true
on these variants.
I'm writing tests for the changes using // @ts-expect-error
, which:
- Expects the provided code to throw an error, but suppresses the error
- Throws an error if the provided code doesn't have any type check errors
// Disabled state | ||
"aria-disabled:cursor-not-allowed", | ||
"aria-disabled:opacity-50", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the disabled styles to the primary variant, instead of leaving it in the base/default class list.
@@ -54,48 +51,45 @@ const computeClassNames = ({ | |||
"border-foreground-danger", | |||
"bg-background-danger", | |||
"text-foreground-danger", | |||
...(disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed disabled styles from the danger
variant.
); | ||
break; | ||
case "info": | ||
classNames.push( | ||
"border-foreground-info", | ||
"bg-background-info", | ||
"text-foreground-info", | ||
...(disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed disabled styles from the info
variant.
@@ -4,16 +4,34 @@ export type ButtonVariant = "primary" | "danger" | "info"; | |||
|
|||
export type ButtonSize = "small" | "medium" | "large"; | |||
|
|||
export interface ButtonProps | |||
interface BaseButtonProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorganized the props so that we can enforce stricter type checking.
As proposed, only the primary
variant can have disabled
being true
.
We could go with the current colors and discuss adding more in the redesign. |
Checklist:
Update index.md
)Issues and Context
There are two issues with the Button component, which were brought up in freeCodeCamp/freeCodeCamp#53824 (comment):
type="submit"
can still trigger form submission even if it's disabled(1) is indeed a bug and the fix is straight forward, so I only want to discuss (2).
So if we want to reflect the disabled state of an element, we can use either the HTML
disabled
oraria-disabled
attribute.With the
disabled
attribute, the element is still present in the DOM, but removed from the accessibility tree, meaning non-sighted users won't know the button is there nor its disabled state, and they also can't interact with it.With the
aria-disabled
attribute, the users can use the keyboard to focus on the button, and can be informed about the element and its disabled state. This is the behavior I decided to go with, as the UX is much better.However, we have been dimming the text and background color of the button is communicate the disabled state visually. This would be acceptable if the button isn't interactable, but since we want it to be interactable, we are failing the color contrast requirement.
Approaches
To address the color contrast requirement, the most straightforward fix would be improving the contrast. We are currently using the
opacity
attribute to dim the colors, but in order to make the button look nice and accessible, we would need to play with the color combinations and not just cranking up the opacity. We also don't have colors that could be used for the disabled state of thedanger
andinfo
variants, so I'd need some help here (from Ahmad) if we want to proceed with the color update.Alternatively, I think we should consider removing the disabled state and keeping buttons enabled at all time.
That would help resolve the color contrast and interactivity issues, while also improving the user experience. (I think we've been assuming the users would understand why the button is disabled and how to enable it, but that might not be obvious for some).
My Fix
I might have gone a little overboard with the changes.
My fix is a mix of both approaches:
primary
button to improve the contrastdisabled
value of thedanger
andinfo
to alwaysfalse
Rationale:
global.css
stylesheet and found that all buttons haveprimary
styles when they are disabled (notice thebutton
selector and the colors used in the code), so I assume we implicitly don't support disabled state indanger
andinfo
variants (otherwise they look very different from their enabled state).danger
orinfo
button that is used with thedisabled
prop (meaningdanger
andinfo
buttons are enabled at all time).The changes are to close down the feature that has no usages, and to move us closer to the second approach above (assuming everyone agrees with the approach).
Screenshots
Basically instead of dimming the text and the background colors, I dim the border color to communicate the disabled state.