mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site): stop click propagation so popover opens inside clickable rows (#26875)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. <img width="628" height="353" alt="image" src="https://github.com/user-attachments/assets/16fbf58f-282a-4629-b0de-45160117adb6" /> Linear: [DES-22051](https://linear.app/codercom/issue/DES-22051/adjust-workspace-list-icon-and-agent-link-affordance) ## Problem [#23374](https://github.com/coder/coder/pull/23374) swapped the underlying `Tooltip` for a `Popover` under the `HelpTooltip` → `HelpPopover` rename, turning the trigger from a hover surface into a click surface. On the workspaces list, `WorkspaceOutdatedTooltip` is rendered inside a `<TableRow>` wired up by `useClickableTableRow`, whose `onClick` navigates to the workspace page. Other interactive children in the same row already stop propagation (checkbox, actions cell, agent badge). The outdated tooltip didn't, so clicking the info icon bubbled up to the row's `onClick` and navigated away before the popover could open. ## Fix Stop click and keydown propagation on both trigger variants (`HelpPopoverTrigger asChild` span and `HelpPopoverIconTrigger`) in `WorkspaceOutdatedTooltip`. The popover still opens because Radix composes its own click handler on top of the user-provided one via `composeEventHandlers`; `stopPropagation()` does not set `defaultPrevented`, so Radix's toggle still runs. ## Regression coverage Added an `InsideClickableRow` story that mounts the tooltip inside a `useClickableTableRow` row with a tracked `onRowClick`, clicks the trigger, asserts the popover dialog opens, and asserts `onRowClick` was not called. Verified locally that the new story fails on `main` (1 call to `onRowClick`) and passes with this fix. <details> <summary>Decision log</summary> - Fix lives on the component (rather than at the `WorkspacesTable.tsx` call site) so every consumer is safe by default. The popover is interactive in a way the parent shouldn't have to know about, and the existing call site in `TaskPage.tsx` is unaffected because its parent has no click handler. - `onKeyDown` propagation is also stopped so the trigger keeps working when activated via keyboard, and to satisfy `lint/a11y/useKeyWithClickEvents` on the `<span>` trigger variant. - Considered pushing the guard down into the `HelpPopoverIconTrigger` / `HelpPopover` primitives so every consumer is covered — left as a separate follow-up since today only `WorkspaceOutdatedTooltip` is embedded in a `useClickableTableRow` row, and a blanket change would need a broader audit. </details>
This commit is contained in:
+68
-1
@@ -1,5 +1,13 @@
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { expect, screen, userEvent, waitFor, within } from "storybook/test";
|
||||
import type { ComponentProps } from "react";
|
||||
import { expect, fn, screen, userEvent, waitFor, within } from "storybook/test";
|
||||
import {
|
||||
Table,
|
||||
TableBody,
|
||||
TableCell,
|
||||
TableRow,
|
||||
} from "#/components/Table/Table";
|
||||
import { useClickableTableRow } from "#/hooks/useClickableTableRow";
|
||||
import {
|
||||
MockTemplate,
|
||||
MockTemplateVersion,
|
||||
@@ -48,3 +56,62 @@ const Example: Story = {
|
||||
};
|
||||
|
||||
export { Example as WorkspaceOutdatedTooltip };
|
||||
|
||||
// Regression coverage for the `useClickableTableRow` usage on the workspaces
|
||||
// list. The trigger must stop click + keyboard propagation so the popover
|
||||
// opens instead of the parent row's onClick swallowing the activation and
|
||||
// navigating away.
|
||||
type ClickableRowArgs = ComponentProps<typeof WorkspaceOutdatedTooltip> & {
|
||||
onRowClick: () => void;
|
||||
};
|
||||
|
||||
export const InsideClickableRow: StoryObj<ClickableRowArgs> = {
|
||||
args: {
|
||||
onRowClick: fn(),
|
||||
},
|
||||
decorators: [
|
||||
(Story, { args }) => {
|
||||
const clickableProps = useClickableTableRow({
|
||||
onClick: args.onRowClick,
|
||||
});
|
||||
return (
|
||||
<Table>
|
||||
<TableBody>
|
||||
<TableRow {...clickableProps}>
|
||||
<TableCell>
|
||||
<Story />
|
||||
</TableCell>
|
||||
</TableRow>
|
||||
</TableBody>
|
||||
</Table>
|
||||
);
|
||||
},
|
||||
],
|
||||
play: async ({ args, canvasElement, step }) => {
|
||||
const body = within(canvasElement.ownerDocument.body);
|
||||
|
||||
await step("mouse click opens the popover", async () => {
|
||||
await userEvent.click(body.getByRole("button", { name: "More info" }));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByRole("dialog")).toHaveTextContent(
|
||||
MockTemplateVersion.message,
|
||||
),
|
||||
);
|
||||
await userEvent.keyboard("{Escape}");
|
||||
});
|
||||
|
||||
await step("keyboard activation via Space opens the popover", async () => {
|
||||
body.getByRole("button", { name: "More info" }).focus();
|
||||
await userEvent.keyboard(" ");
|
||||
await waitFor(() =>
|
||||
expect(screen.getByRole("dialog")).toHaveTextContent(
|
||||
MockTemplateVersion.message,
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
await step("the row's onClick was never called", async () => {
|
||||
expect(args.onRowClick).not.toHaveBeenCalled();
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { useTheme } from "@emotion/react";
|
||||
import Link from "@mui/material/Link";
|
||||
import { InfoIcon, RotateCcwIcon } from "lucide-react";
|
||||
import { CircleAlertIcon, RotateCcwIcon } from "lucide-react";
|
||||
import { type FC, type ReactNode, useState } from "react";
|
||||
import { useQuery } from "react-query";
|
||||
import { toast } from "sonner";
|
||||
@@ -35,27 +35,36 @@ export const WorkspaceOutdatedTooltip: FC<WorkspaceOutdatedTooltipProps> = ({
|
||||
}) => {
|
||||
const [isOpen, setIsOpen] = useState(false);
|
||||
|
||||
// Stop activation from bubbling to a parent `useClickableTableRow` row,
|
||||
// which navigates on click, Enter (onKeyDown), and Space (onKeyUp). Radix
|
||||
// composes its own click handler, so the popover still opens.
|
||||
const stopPropagation = (event: React.SyntheticEvent) => {
|
||||
event.stopPropagation();
|
||||
};
|
||||
|
||||
return (
|
||||
<HelpPopover open={isOpen} onOpenChange={setIsOpen}>
|
||||
{children ? (
|
||||
<HelpPopoverTrigger asChild>
|
||||
<span className="flex items-center gap-1.5 cursor-help">
|
||||
<InfoIcon
|
||||
css={(theme) => ({
|
||||
color: theme.roles.notice.outline,
|
||||
})}
|
||||
size={14}
|
||||
/>
|
||||
<span
|
||||
className="flex items-center gap-1.5 cursor-help"
|
||||
onClick={stopPropagation}
|
||||
onKeyDown={stopPropagation}
|
||||
onKeyUp={stopPropagation}
|
||||
>
|
||||
<CircleAlertIcon className="text-content-secondary" size={14} />
|
||||
<span>{children}</span>
|
||||
</span>
|
||||
</HelpPopoverTrigger>
|
||||
) : (
|
||||
<HelpPopoverIconTrigger size="small" hoverEffect={false}>
|
||||
<InfoIcon
|
||||
css={(theme) => ({
|
||||
color: theme.roles.notice.outline,
|
||||
})}
|
||||
/>
|
||||
<HelpPopoverIconTrigger
|
||||
size="small"
|
||||
hoverEffect={false}
|
||||
onClick={stopPropagation}
|
||||
onKeyDown={stopPropagation}
|
||||
onKeyUp={stopPropagation}
|
||||
>
|
||||
<CircleAlertIcon className="text-content-secondary" />
|
||||
<span className="sr-only">Outdated info</span>
|
||||
</HelpPopoverIconTrigger>
|
||||
)}
|
||||
|
||||
Reference in New Issue
Block a user