make disabled buttons focusable (#132)

src/components/Button/Button.module.css

@@ -26,7 +26,7 @@ limitations under the License.
   font: var(--cpd-font-body-md-semibold);
 }
 
-.button[disabled] {
+.button[aria-disabled="true"] {
   cursor: not-allowed;
   pointer-events: all !important;
 }
@@ -76,7 +76,7 @@ limitations under the License.
   background: var(--cpd-color-bg-action-primary-pressed);
 }
 
-.button[data-kind="primary"][disabled] {
+.button[data-kind="primary"][aria-disabled="true"] {
   color: var(--cpd-color-text-disabled);
   background: var(--cpd-color-bg-subtle-primary);
 }
@@ -100,7 +100,7 @@ limitations under the License.
   background: var(--cpd-color-bg-subtle-primary);
 }
 
-.button[data-kind="secondary"][disabled] {
+.button[data-kind="secondary"][aria-disabled="true"] {
   border-color: var(--cpd-color-border-interactive-secondary);
   color: var(--cpd-color-text-disabled);
   background: var(--cpd-color-bg-subtle-secondary);
@@ -124,7 +124,7 @@ limitations under the License.
   background: var(--cpd-color-bg-subtle-primary);
 }
 
-.button[data-kind="tertiary"][disabled] {
+.button[data-kind="tertiary"][aria-disabled="true"] {
   color: var(--cpd-color-text-disabled);
 }
 
@@ -147,7 +147,7 @@ limitations under the License.
   background: var(--cpd-color-bg-critical-subtle-hovered);
 }
 
-.button[data-kind="destructive"][disabled] {
+.button[data-kind="destructive"][aria-disabled="true"] {
   border-color: var(--cpd-color-border-interactive-secondary);
   color: var(--cpd-color-text-disabled);
   background: var(--cpd-color-bg-subtle-secondary);

src/components/Button/Button.stories.tsx

@@ -14,62 +14,125 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React from "react";
-import { Meta, StoryFn } from "@storybook/react";
+import { Meta } from "@storybook/react";
 
 import { Button as ButtonComponent } from "./Button";
 import VisibilityOnIcon from "@vector-im/compound-design-tokens/icons/visibility-on.svg";
 
-type Props = {
-  kind?: "primary" | "secondary" | "tertiary" | "destructive";
-  size?: "sm" | "lg";
-};
-
 export default {
   title: "Button",
   component: ButtonComponent,
   tags: ["autodocs"],
+  parameters: {
+    actions: { argTypesRegex: "^on.*" },
+  },
   argTypes: {
     size: {
       options: ["sm", "lg"],
       control: { type: "inline-radio" },
     },
+    kind: {
+      options: ["primary", "secondary", "tertiary", "destructive"],
+      control: { type: "inline-radio" },
+    },
+    disabled: {
+      type: "boolean",
+    },
+    as: {
+      options: ["a", "button"],
+      control: { type: "inline-radio" },
+    },
+    Icon: {
+      options: [undefined, "VisibilityOnIcon"],
+      mapping: {
+        VisibilityOnIcon: VisibilityOnIcon,
+        undefined: undefined,
+      },
+      control: { type: "inline-radio" },
+    },
+    onClick: { action: "onClick" },
   },
   args: {
-    size: "sm",
+    size: "lg",
+    as: undefined,
+    disabled: false,
+    children: "Click me!",
   },
 } as Meta<typeof ButtonComponent>;
 
-const Template: StoryFn<typeof ButtonComponent> = ({ kind, size }: Props) => (
-  <div style={{ display: "flex", gap: 8 }}>
-    <ButtonComponent kind={kind} size={size}>
-      Click me!
-    </ButtonComponent>
-    <ButtonComponent Icon={VisibilityOnIcon} kind={kind} size={size}>
-      Click me!
-    </ButtonComponent>
-    <ButtonComponent as="a" href="#" kind={kind} size={size}>
-      Click me!
-    </ButtonComponent>
-  </div>
-);
-
-export const Primary = Template.bind({});
-Primary.args = {
-  kind: "primary",
+export const Default = {
+  args: {
+    // test component defaults
+    kind: undefined,
+    size: undefined,
+  },
 };
 
-export const Secondary = Template.bind({});
-Secondary.args = {
-  kind: "secondary",
+export const Small = {
+  args: {
+    // test component defaults
+    kind: undefined,
+    size: "sm",
+  },
+};
+
+export const Primary = {
+  args: {
+    kind: "primary",
+  },
 };
 
-export const Tertiary = Template.bind({});
-Tertiary.args = {
-  kind: "tertiary",
+export const Secondary = {
+  args: {
+    kind: "secondary",
+  },
 };
 
-export const Destructive = Template.bind({});
-Destructive.args = {
-  kind: "destructive",
+export const Tertiary = {
+  args: {
+    kind: "tertiary",
+  },
+};
+
+export const Destructive = {
+  args: {
+    kind: "destructive",
+  },
+};
+
+export const WithIcon = {
+  args: {
+    ...Primary.args,
+    Icon: VisibilityOnIcon,
+  },
+};
+
+export const SmallWithIcon = {
+  args: {
+    ...Primary.args,
+    size: "sm",
+    Icon: VisibilityOnIcon,
+  },
+};
+
+export const Disabled = {
+  args: {
+    ...Primary.args,
+    disabled: true,
+  },
+};
+
+export const Link = {
+  args: {
+    ...Primary.args,
+    as: "a",
+    href: "https://example.org",
+  },
+};
+
+export const LinkDisabled = {
+  args: {
+    ...Link.args,
+    disabled: true,
+  },
 };

src/components/Button/Button.test.tsx

@@ -16,46 +16,120 @@ limitations under the License.
 
 import { vi, describe, it, expect } from "vitest";
 import React from "react";
-import { getByRole, render } from "@testing-library/react";
-import userEvent from "@testing-library/user-event";
-import ChatIcon from "@vector-im/compound-design-tokens/icons/chat.svg";
+import { fireEvent, screen, render } from "@testing-library/react";
 
 import { Button } from "./Button";
 
+import * as stories from "./Button.stories";
+import { composeStories } from "@storybook/react";
+
+const {
+  Default,
+  Small,
+  Primary,
+  Secondary,
+  Tertiary,
+  Destructive,
+  WithIcon,
+  SmallWithIcon,
+  Disabled,
+  Link,
+  LinkDisabled,
+} = composeStories(stories);
+
 describe("Button", () => {
-  it("renders", () => {
-    const { asFragment } = render(<Button kind="primary">Click me!</Button>);
-    expect(asFragment()).toMatchSnapshot();
+  it("renders a Default button", () => {
+    const { container } = render(<Default />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Small button", () => {
+    const { container } = render(<Small />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Primary button", () => {
+    const { container } = render(<Primary />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Secondary button", () => {
+    const { container } = render(<Secondary />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Tertiary button", () => {
+    const { container } = render(<Tertiary />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Destructive button", () => {
+    const { container } = render(<Destructive />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a WithIcon button", () => {
+    const { container } = render(<WithIcon />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a SmallWithIcon button", () => {
+    const { container } = render(<SmallWithIcon />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Disabled button", () => {
+    const { container } = render(<Disabled />);
+    expect(container).toMatchSnapshot();
+  });
+  it("renders a Link button", () => {
+    const { container } = render(<Link />);
+    expect(container).toMatchSnapshot();
   });
 
-  it("accepts an icon", () => {
-    const { asFragment } = render(
-      <Button kind="primary" Icon={ChatIcon}>
-        With icon
+  it("renders a LinkDisabled button", () => {
+    const { container } = render(<LinkDisabled />);
+    expect(container).toMatchSnapshot();
+  });
+
+  it("can be clicked", () => {
+    const onClick = vi.fn();
+    render(<Button onClick={onClick}>Test</Button>);
+
+    fireEvent.click(screen.getByText("Test"));
+
+    expect(onClick).toHaveBeenCalled();
+  });
+
+  it("cannot be clicked when disabled", () => {
+    const onClick = vi.fn();
+    render(
+      <Button onClick={onClick} disabled={true}>
+        Test
       </Button>,
     );
-    expect(asFragment()).toMatchSnapshot();
+
+    fireEvent.click(screen.getByText("Test"));
+
+    expect(onClick).not.toHaveBeenCalled();
   });
 
-  it("can be clicked", async () => {
-    const spy = vi.fn();
-    const { container } = render(<Button onClick={spy}>Click me!</Button>);
+  it("does not submit when disabled", () => {
+    const onSubmit = vi.fn();
+    render(
+      <Button onSubmit={onSubmit} disabled={true}>
+        Test
+      </Button>,
+    );
 
-    const user = userEvent.setup();
-    await user.click(getByRole(container, "button"));
+    fireEvent.click(screen.getByText("Test"));
 
-    expect(spy).toHaveBeenCalledTimes(1);
+    expect(onSubmit).not.toHaveBeenCalled();
   });
 
-  it("works as a link", () => {
-    const { container } = render(
-      <Button as="a" href="#anchor">
-        This is a link that looks like a button
+  it("can be focused when disabled", () => {
+    const onClick = vi.fn();
+    const onFocus = vi.fn();
+    render(
+      <Button onClick={onClick} onFocus={onFocus} disabled={true}>
+        Test
       </Button>,
     );
 
-    const anchor = getByRole(container, "link");
-    expect(anchor).toBeInTheDocument();
-    expect(anchor).toHaveAttribute("href", "#anchor");
+    fireEvent.focus(screen.getByText("Test"));
+
+    expect(onFocus).toHaveBeenCalled();
   });
 });

src/components/Button/Button.tsx

@@ -46,6 +46,12 @@ type ButtonOwnProps = PropsWithChildren<{
    * An icon to display within the button.
    */
   Icon?: ComponentType<React.SVGAttributes<SVGElement>>;
+  /**
+   * Note that disabled attribute is not added to buttons, so that disabled buttons are discoverable by keyboard.
+   * `aria-disabled` attribute is used to indicate button is disabled.
+   * Event handlers are not passed to disabled buttons (onClick, onSubmit).
+   */
+  disabled?: boolean;
 }>;
 
 type ButtonPropsFor<C extends React.ElementType> = ButtonOwnProps &
@@ -67,6 +73,7 @@ export const Button = forwardRef(function Button<
     children,
     className,
     Icon,
+    disabled,
     ...props
   }: ButtonPropsFor<C> & { as?: C },
   ref: ForwardedRef<C>,
@@ -76,9 +83,17 @@ export const Button = forwardRef(function Button<
     [styles["has-icon"]]: Icon,
   });
 
+  const { onClick, onSubmit, ...restProps } = props;
+  const eventHandlers = disabled
+    ? {}
+    : {
+        onClick,
+        onSubmit,
+      };
+
   return (
     <Component
-      {...props}
+      {...restProps}
       ref={ref as Ref<C>}
       className={classes}
       data-kind={kind}
@@ -87,6 +102,8 @@ export const Button = forwardRef(function Button<
       // We want them to behave like links but look like buttons
       role={as === "a" ? "link" : "button"}
       tabIndex={0}
+      {...eventHandlers}
+      aria-disabled={disabled}
     >
       {Icon && (
         <Icon