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.

Sign up for GitHub

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

Jump to bottom

fix(a11y): disabled button behavior and color contrast #65

Merged
merged 3 commits into from
Apr 22, 2024
Merged

fix(a11y): disabled button behavior and color contrast #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12c5ba3
fix: disabled submit button triggers form submission
huyenltnguyen Apr 10, 2024
a972159
fix: color contrast of disabled state
huyenltnguyen Apr 10, 2024
339ade3
Merge branch 'main' of https://github.com/freeCodeCamp/ui into fix/di…
huyenltnguyen Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/button/button.stories.tsx
View file
Open in desktop
Copy link
Member Author

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.

Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import React, { useState } from "react";
import { Meta, StoryObj } from "@storybook/react";

import { FormControl } from "../form-control";
import { FormGroup } from "../form-group";
import { ControlLabel } from "../control-label";
import { Button } from ".";

const story = {
Expand Down Expand Up @@ -115,4 +119,37 @@ export const AsADownloadLink: Story = {
},
};

const FormWithSubmitButton = () => {
Copy link
Member Author

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 Buttons in a form.

const [username, setUsername] = useState("");

const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
setUsername(event.target.value);
};

const handleSubmit = () => {
alert("Submitted");
};

return (
<form onSubmit={handleSubmit}>
<FormGroup controlId="username">
<ControlLabel>Username</ControlLabel>
<FormControl
componentClass="input"
type="text"
onChange={handleChange}
/>
</FormGroup>

<Button type="submit" disabled={!username}>
Submit
</Button>
</form>
);
};

export const AsASubmitButton: Story = {
render: FormWithSubmitButton,
};

export default story;
36 changes: 36 additions & 0 deletions src/button/button.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,28 @@ describe("<Button />", () => {
expect(onClick).not.toHaveBeenCalled();
});

it("should not trigger form submission if the button has `submit` type and is disabled", async () => {
const handleSubmit = jest.fn();

render(
<form onSubmit={handleSubmit}>
<label>
Username
<input type="text" />
</label>

<Button type="submit" disabled>
Submit
</Button>
</form>,
);

const button = screen.getByRole("button", { name: "Submit" });
await userEvent.click(button);

expect(handleSubmit).not.toHaveBeenCalled();
});

it("should render an anchor element if the `href` prop is defined", () => {
render(<Button href="https://www.freecodecamp.org">freeCodeCamp</Button>);

Expand Down Expand Up @@ -115,3 +137,17 @@ describe("<Button />", () => {
expect(onClick).toHaveBeenCalledTimes(1);
});
});

// ------------------------------
// Type tests
// ------------------------------

// @ts-expect-error - Button with `danger` variant cannot be disabled
Copy link
Member Author

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

<Button variant="danger" disabled>
Button text
</Button>;

// @ts-expect-error - Button with `info` variant cannot be disabled
<Button variant="info" disabled>
Button text
</Button>;
58 changes: 29 additions & 29 deletions src/button/button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ const defaultClassNames = [
"active:before:border-transparent",
"active:before:bg-gray-900",
"active:before:opacity-20",
// Disabled state
"aria-disabled:cursor-not-allowed",
"aria-disabled:opacity-50",
Comment on lines -21 to -23
Copy link
Member Author

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.

// Focus state
"focus:outline-none", // Hide the default browser outline
"focus-visible:ring",
Expand Down Expand Up @@ -54,48 +51,45 @@ const computeClassNames = ({
"border-foreground-danger",
"bg-background-danger",
"text-foreground-danger",
...(disabled
Copy link
Member Author

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.

? ["active:before:hidden"]
: [
"hover:bg-foreground-danger",
"hover:text-background-danger",
// This hover rule is redundant for the component library,
// but is needed to override the border color set in client's `global.css`.
// We can remove it once we have completely removed the CSS overrides in client.
"hover:border-foreground-danger",
"dark:hover:bg-background-danger",
"dark:hover:text-foreground-danger",
]),
"hover:bg-foreground-danger",
"hover:text-background-danger",
// This hover rule is redundant for the component library,
// but is needed to override the border color set in client's `global.css`.
// We can remove it once we have completely removed the CSS overrides in client.
"hover:border-foreground-danger",
"dark:hover:bg-background-danger",
"dark:hover:text-foreground-danger",
);
break;
case "info":
classNames.push(
"border-foreground-info",
"bg-background-info",
"text-foreground-info",
...(disabled
Copy link
Member Author

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.

? ["active:before:hidden"]
: [
"hover:bg-foreground-info",
"hover:text-background-info",
// This hover rule is redundant for the component library,
// but is needed to override the border color set in client's `global.css`.
// We can remove it once we have completely removed the CSS overrides in client.
"hover:border-foreground-info",
"dark:hover:bg-background-info",
"dark:hover:text-foreground-info",
]),
"hover:bg-foreground-info",
"hover:text-background-info",
// This hover rule is redundant for the component library,
// but is needed to override the border color set in client's `global.css`.
// We can remove it once we have completely removed the CSS overrides in client.
"hover:border-foreground-info",
"dark:hover:bg-background-info",
"dark:hover:text-foreground-info",
);
break;
// default variant is 'primary'
default:
classNames.push(
"border-foreground-secondary",
"bg-background-quaternary",
"text-foreground-secondary",
...(disabled
? ["active:before:hidden"]
? [
"active:before:hidden",
"border-gray-450",
"aria-disabled:cursor-not-allowed",
"aria-disabled:opacity-80",
]
: [
"border-foreground-secondary",
"hover:bg-foreground-primary",
"hover:text-background-primary",
// This hover rule is redundant for the component library,
Expand Down Expand Up @@ -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();
Copy link
Member Author

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.

return;
}

Expand Down Expand Up @@ -197,6 +192,11 @@ export const HeadlessButton = React.forwardRef<
);
} else {
return (
// @ts-expect-error - Type check error is expected.
// `disabled` can either be `boolean | undefined` or `false | undefined` depending on the union member.
// TypeScript can't infer the actual union member (that ties to the `variant`),
// so it complains about the `disabled` type being incompatible.
// Ref: https://github.com/Microsoft/TypeScript/issues/30518
<StylessButton
className={className}
onClick={onClick}
Expand Down
24 changes: 21 additions & 3 deletions src/button/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,34 @@ export type ButtonVariant = "primary" | "danger" | "info";

export type ButtonSize = "small" | "medium" | "large";

export interface ButtonProps
interface BaseButtonProps
Copy link
Member Author

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.

extends React.ButtonHTMLAttributes<HTMLButtonElement | HTMLAnchorElement> {
children: React.ReactNode;
variant?: ButtonVariant;
size?: ButtonSize;
onClick?: MouseEventHandler<HTMLButtonElement | HTMLAnchorElement>;
type?: "submit" | "button";
disabled?: boolean;
block?: boolean;
href?: string;
download?: string;
target?: React.HTMLAttributeAnchorTarget;
}

interface PrimaryButtonProps extends BaseButtonProps {
variant?: "primary";
disabled?: boolean;
}

interface InfoButtonProps extends BaseButtonProps {
variant: "info";
disabled?: false;
}

interface DangerButtonProps extends BaseButtonProps {
variant: "danger";
disabled?: false;
}

export type ButtonProps =
| PrimaryButtonProps
| InfoButtonProps
| DangerButtonProps;