Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions app/src/components/ui/button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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}
/>
)
Expand Down
3 changes: 3 additions & 0 deletions app/src/components/ui/combobox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
87 changes: 87 additions & 0 deletions app/tests/button-native.test.tsx
Original file line number Diff line number Diff line change
@@ -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();
});