Let a button drawn as a link answer the keyboard like a button (#303)

Six controls in the app are buttons that navigate: "New skill", "New
agent", the sidebar's new-channel control, the two empty-state returns,
and PageShell's back button, which is five routes in every state each of
them has. All six draw a router Link through `render`.

Base UI defaults `nativeButton` to true, so every one of them was told to
expect a native <button> and found an anchor, and said so at render. The
warning was the visible half. The rest was that Base UI put type="button"
on an anchor, where it means nothing, and withheld the role="button" and
Space-to-activate handling it applies to a non-button — so these read as
links to a screen reader and ignored the Space key.

Replacing the element is exactly the case where the default is wrong, so
the default now follows `render`, once in the shared Button rather than
at each call site. Passing `render` is not proof the result is a
non-button, only that we can no longer assume it is one, so the one call
site that draws a real <button> through `render` says so: the combobox
trigger, which otherwise trips the inverse warning.

SidebarMenuButton, Item and the sidebar's other wrappers call useRender
directly rather than useButton, so they have no nativeButton and never
warned; DropdownMenuItem already defaults it to false. None of them
change here.

Author: jerelvelarde

app/src/components/ui/button.tsx

@@ -44,12 +44,32 @@ function Button({
   className,
   variant = "default",
   size = "default",
+  render,
+  /*
+   * DIVERGES FROM UPSTREAM SHADCN. Base UI defaults `nativeButton` to `true`, which is right only
+   * while the element really is a `<button>`. Six call sites here draw a router `Link` through
+   * `render` instead — "New skill", "New agent", the sidebar's new-channel control, the two
+   * empty-state returns, and `PageShell`'s back button, which is five routes in every state each of
+   * them has — and every one of them warned at render that it had been told to expect a native
+   * button and found an anchor. It was not only noise: Base UI was putting `type="button"` on an
+   * anchor, which means nothing there, and withholding the `role="button"` and Space-to-activate
+   * handling a non-button needs in order to behave like one.
+   *
+   * Replacing the element is exactly the case where the default is wrong, so the default follows
+   * `render`, once here rather than at every call site. Passing `render` is not proof the result is
+   * a non-button, only that we can no longer assume it is one, so a call site drawing a real
+   * `<button>` through `render` passes `nativeButton` back explicitly — `combobox.tsx` is the one
+   * that does.
+   */
+  nativeButton = render === undefined,
   ...props
 }: ButtonPrimitive.Props & VariantProps<typeof buttonVariants>) {
   return (
     <ButtonPrimitive
       data-slot="button"
       className={cn(buttonVariants({ variant, size, className }))}
+      nativeButton={nativeButton}
+      render={render}
       {...props}
     />
   )

app/src/components/ui/combobox.tsx

@@ -72,6 +72,9 @@ function ComboboxInput({
             size="icon-xs"
             variant="ghost"
             render={<ComboboxTrigger />}
+            /* `ComboboxTrigger` draws a real `<button>`, which `Button` cannot tell from the one
+             * call site here that draws a link. See the note in `button.tsx`. */
+            nativeButton
             data-slot="input-group-button"
             className="group-has-data-[slot=combobox-clear]/input-group:hidden data-pressed:bg-transparent"
             disabled={disabled}

app/tests/button-native.test.tsx

@@ -0,0 +1,87 @@
+import { afterAll, afterEach, beforeAll, expect, spyOn, test } from "bun:test";
+import { GlobalRegistrator } from "@happy-dom/global-registrator";
+import { cleanup, render } from "@testing-library/react";
+import type { ReactElement } from "react";
+import { Button } from "@/components/ui/button";
+
+beforeAll(() => GlobalRegistrator.register());
+afterEach(cleanup);
+afterAll(() => GlobalRegistrator.unregister());
+
+/**
+ * Base UI reports a button drawn as the wrong element through `console.error`, from an effect, so
+ * the only way to assert the app renders quietly is to collect what a render logs.
+ *
+ * The DOM assertions below carry the real weight. Base UI remembers every message it has already
+ * printed for the life of the process, so an empty log is evidence only as long as nothing earlier
+ * said the same thing. `role` and `type` on the rendered element say what `nativeButton` resolved to
+ * no matter what else has run.
+ */
+function drawing(element: ReactElement) {
+  const logged: string[] = [];
+  const spy = spyOn(console, "error").mockImplementation(
+    (...args: unknown[]) => {
+      logged.push(args.map(String).join(" "));
+    },
+  );
+
+  try {
+    const { container } = render(element);
+    return {
+      complaints: logged.filter((message) => message.startsWith("Base UI:")),
+      element: container.firstElementChild as HTMLElement,
+    };
+  } finally {
+    spy.mockRestore();
+  }
+}
+
+test("a button with no `render` is still a native button", () => {
+  const { complaints, element } = drawing(<Button>Save</Button>);
+
+  expect(complaints).toEqual([]);
+  expect(element.tagName).toBe("BUTTON");
+  expect(element.getAttribute("type")).toBe("button");
+  expect(element.getAttribute("role")).toBeNull();
+});
+
+test("a button drawn as a link takes button semantics rather than `type`", () => {
+  const { complaints, element } = drawing(
+    <Button render={<a href="/settings" />}>Settings</Button>,
+  );
+
+  expect(complaints).toEqual([]);
+  expect(element.tagName).toBe("A");
+  expect(element.getAttribute("role")).toBe("button");
+  expect(element.getAttribute("type")).toBeNull();
+});
+
+/**
+ * The shape `PageShell`'s back button and the sidebar's links use: a function, because a router
+ * `Link` takes its own props alongside the ones Base UI merges in.
+ */
+test("a button drawn as a link through the function form does the same", () => {
+  const { complaints, element } = drawing(
+    <Button render={(props) => <a href="/agents" {...props} />}>Agents</Button>,
+  );
+
+  expect(complaints).toEqual([]);
+  expect(element.tagName).toBe("A");
+  expect(element.getAttribute("role")).toBe("button");
+});
+
+/**
+ * `render` that draws a real button is the case the default cannot see, so a call site says so —
+ * `combobox.tsx` is the one that does.
+ */
+test("a call site drawing a real button can say so", () => {
+  const { complaints, element } = drawing(
+    <Button nativeButton render={<button type="button" />}>
+      Open
+    </Button>,
+  );
+
+  expect(complaints).toEqual([]);
+  expect(element.tagName).toBe("BUTTON");
+  expect(element.getAttribute("role")).toBeNull();
+});