From 929a40985c205a05cc61087cad738ab12cee47a9 Mon Sep 17 00:00:00 2001
From: Peter Makowski <peter.makowski@canonical.com>
Date: Mon, 13 May 2024 14:08:46 +0000
Subject: [PATCH] feat: allow focusing disabled buttons and displaying tooltips
 (#1082)

Signed-off-by: Peter Makowski <me+git@petermakowski.io>
---
 src/components/Button/Button.stories.mdx      | 20 +++++++++++++++++++
 src/components/Button/Button.test.tsx         | 15 ++++----------
 src/components/Button/Button.tsx              |  6 ++----
 .../ContextualMenu/ContextualMenu.test.tsx    |  4 +++-
 .../TablePagination.test.tsx.snap             |  8 ++++----
 5 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/components/Button/Button.stories.mdx b/src/components/Button/Button.stories.mdx
index d934ad293..70a68bcae 100644
--- a/src/components/Button/Button.stories.mdx
+++ b/src/components/Button/Button.stories.mdx
@@ -1,6 +1,7 @@
 import { ArgsTable, Canvas, Meta, Story } from "@storybook/addon-docs";
 
 import Button, { ButtonAppearance } from "./Button";
+import Tooltip from "../Tooltip";
 
 <Meta
   title="Button"
@@ -231,3 +232,22 @@ Buttons are clickable elements used to perform an action, they can be used for b
     {Template.bind({})}
   </Story>
 </Canvas>
+
+<Canvas>
+  <Story
+    name="Disabled with tooltip"
+    args={{
+      children: "Disabled button with a tooltip",
+      disabled: true,
+    }}
+  >
+    {(args) => (
+      <div style={{ paddingTop: "3rem" }}>
+        <Tooltip message="This button is disabled">
+          <Button {...args} />
+        </Tooltip>
+      </div>
+    )}
+  </Story>
+</Canvas>
+
diff --git a/src/components/Button/Button.test.tsx b/src/components/Button/Button.test.tsx
index 8b61e4249..0861f6e01 100644
--- a/src/components/Button/Button.test.tsx
+++ b/src/components/Button/Button.test.tsx
@@ -37,21 +37,14 @@ describe("Button ", () => {
     const onClick = jest.fn();
     render(<Button disabled={true} onClick={onClick} />);
     const button = screen.getByRole("button");
-    expect(button).toBeDisabled();
-    expect(button).not.toHaveAttribute("aria-disabled");
-    expect(button).not.toHaveClass("is-disabled");
+    // The button should be disabled but not have the disabled attribute to allow for focus and displaying the tooltip
+    expect(button).not.toBeDisabled();
+    expect(button).toHaveAttribute("aria-disabled");
+    expect(button).toHaveClass("is-disabled");
     await userEvent.click(button);
     expect(onClick).not.toHaveBeenCalled();
   });
 
-  it("does not prevent default when disabling a button", async () => {
-    render(<Button disabled={true} onClick={jest.fn()} />);
-    const button = screen.getByRole("button");
-    const clickEvent = createEvent.click(button);
-    fireEvent(button, clickEvent);
-    expect(clickEvent.defaultPrevented).toBe(false);
-  });
-
   it("correctly disables a link", async () => {
     const onClick = jest.fn();
     render(
diff --git a/src/components/Button/Button.tsx b/src/components/Button/Button.tsx
index 6db834be2..64f2b5b29 100644
--- a/src/components/Button/Button.tsx
+++ b/src/components/Button/Button.tsx
@@ -88,21 +88,19 @@ const Button = <P,>({
     {
       "has-icon": hasIcon,
       "is-dense": dense,
-      "is-disabled": Component !== "button" && disabled,
+      "is-disabled": disabled,
       "is-inline": inline,
       "is-small": small,
     },
     className
   );
   const onClickDisabled = (e: MouseEvent) => e.preventDefault();
-  const disabledProp =
-    Component === "button" ? { disabled } : { "aria-disabled": disabled };
 
   return (
     <Component
       className={classes}
       onClick={disabled ? onClickDisabled : onClick}
-      {...disabledProp}
+      aria-disabled={disabled || undefined}
       {...buttonProps}
     >
       {children}
diff --git a/src/components/ContextualMenu/ContextualMenu.test.tsx b/src/components/ContextualMenu/ContextualMenu.test.tsx
index e8fe31271..97235ae4e 100644
--- a/src/components/ContextualMenu/ContextualMenu.test.tsx
+++ b/src/components/ContextualMenu/ContextualMenu.test.tsx
@@ -41,7 +41,9 @@ describe("ContextualMenu ", () => {
 
   it("can be disabled", () => {
     render(<ContextualMenu links={[]} toggleDisabled toggleLabel="Toggle" />);
-    expect(screen.getByRole("button", { name: "Toggle" })).toBeDisabled();
+    expect(screen.getByRole("button", { name: "Toggle" })).toHaveAttribute(
+      "aria-disabled"
+    );
   });
 
   it("can have a toggle button with just an icon", () => {
diff --git a/src/components/TablePagination/__snapshots__/TablePagination.test.tsx.snap b/src/components/TablePagination/__snapshots__/TablePagination.test.tsx.snap
index a42b77430..0fc40b997 100644
--- a/src/components/TablePagination/__snapshots__/TablePagination.test.tsx.snap
+++ b/src/components/TablePagination/__snapshots__/TablePagination.test.tsx.snap
@@ -12,9 +12,9 @@ exports[`<TablePagination /> renders table pagination and matches the snapshot 1
     Showing all 5 items
   </div>
   <button
+    aria-disabled="true"
     aria-label="Previous page"
-    class="p-button--base has-icon back"
-    disabled=""
+    class="p-button--base has-icon is-disabled back"
   >
     <i
       class="p-icon--chevron-down"
@@ -45,9 +45,9 @@ exports[`<TablePagination /> renders table pagination and matches the snapshot 1
   of 
   1
   <button
+    aria-disabled="true"
     aria-label="Next page"
-    class="p-button--base has-icon next"
-    disabled=""
+    class="p-button--base has-icon is-disabled next"
   >
     <i
       class="p-icon--chevron-down"