fix(a11y): Keyboard-operable Cards

.changeset/fix-cui-308-clickable-cards.md

@@ -0,0 +1,5 @@
+---
+'@clickhouse/click-ui': patch
+---
+
+`CardPrimary` and `CardSecondary` with `onClick` are exposed as buttons and activate with Enter and Space, unless the card renders its own `infoText` button (`CardPrimary`) or `infoUrl` link (`CardSecondary`); then the card gets no button role and that control keeps Enter and Space. A card that is not a button is no longer a Tab stop, on all three cards; display-only cards keep the default key behavior. A disabled `CardPrimary`, `CardSecondary` or `CardHorizontal` no longer calls `onClick`. Do not put links or buttons inside the title, description or children of a clickable card.

eslint-suppressions.json

@@ -7,16 +7,6 @@
       "count": 1
     }
   },
-  "src/components/CardPrimary/CardPrimary.tsx": {
-    "jsx-a11y/no-noninteractive-tabindex": {
-      "count": 1
-    }
-  },
-  "src/components/CardSecondary/CardSecondary.tsx": {
-    "jsx-a11y/no-noninteractive-tabindex": {
-      "count": 1
-    }
-  },
   "src/components/Collapsible/Collapsible.tsx": {
     "jsx-a11y/click-events-have-key-events": {
       "count": 1

src/components/CardHorizontal/CardHorizontal.test.tsx

@@ -123,6 +123,7 @@ describe('CardHorizontal Component', () => {
     const { container } = renderCard({
       title: 'Test Card',
       disabled: true,
+      onClick: () => undefined,
     });
 
     const wrapper = container.firstChild;
@@ -133,12 +134,25 @@ describe('CardHorizontal Component', () => {
     const { container } = renderCard({
       title: 'Test Card',
       disabled: false,
+      onClick: () => undefined,
     });
 
     const wrapper = container.firstChild;
     expect(wrapper).toHaveAttribute('tabIndex', '0');
   });
 
+  it('should not be a Tab stop without a body action or with an inner button', () => {
+    const plain = renderCard({ title: 'Summary' }).container.firstChild;
+    expect(plain).not.toHaveAttribute('tabIndex');
+
+    const withButton = renderCard({
+      title: 'Test Card',
+      infoText: 'Continue',
+      onButtonClick: () => undefined,
+    }).container.firstChild;
+    expect(withButton).not.toHaveAttribute('tabIndex');
+  });
+
   it('should not call onClick when disabled', () => {
     const onClickMock = vitest.fn();
     const { container } = renderCard({
@@ -225,6 +239,22 @@ describe('CardHorizontal Component', () => {
     expect(onButtonClick).toHaveBeenCalledTimes(1);
   });
 
+  it('should not run onButtonClick on a body click when onClick is explicitly undefined', async () => {
+    const onButtonClick = vitest.fn();
+    const { container, getByRole } = renderCard({
+      title: 'Test Card',
+      infoText: 'Continue',
+      onClick: undefined,
+      onButtonClick,
+    });
+
+    fireEvent.click(container.firstChild as HTMLElement);
+    expect(onButtonClick).not.toHaveBeenCalled();
+
+    await userEvent.click(getByRole('button', { name: 'Continue' }));
+    expect(onButtonClick).toHaveBeenCalledTimes(1);
+  });
+
   it('should open infoUrl only once when the inner button is clicked', async () => {
     const windowOpenSpy = vitest.spyOn(window, 'open').mockImplementation(() => null);
     const { getByRole } = renderCard({
@@ -450,7 +480,11 @@ describe('CardHorizontal Component', () => {
 
     it("should call the consumer's onKeyDown", async () => {
       const onKeyDown = vitest.fn();
-      const { container } = renderCard({ title: 'Test Card', onKeyDown });
+      const { container } = renderCard({
+        title: 'Test Card',
+        onClick: () => undefined,
+        onKeyDown,
+      });
 
       (container.firstChild as HTMLElement).focus();
       await userEvent.keyboard('{Enter}');
@@ -497,5 +531,14 @@ describe('CardHorizontal Component', () => {
 
       expect(screen.queryByRole('button')).not.toBeInTheDocument();
     });
+
+    it('should not run a consumer onClick on a disabled card', () => {
+      const onClick = vitest.fn();
+      const { container } = renderCard({ title: 'Test Card', disabled: true, onClick });
+
+      fireEvent.click(container.firstChild as HTMLElement);
+
+      expect(onClick).not.toHaveBeenCalled();
+    });
   });
 });

src/components/CardHorizontal/CardHorizontal.tsx

@@ -3,6 +3,7 @@ import { Button } from '@/components/Button';
 import { Container } from '@/components/Container';
 import { Icon } from '@/components/Icon';
 import { cn, cva } from '@/lib/cva';
+import { clickOnActivationKey } from '@/utils/keyboard';
 import { openInNewTab } from '@/utils/url';
 import { CardHorizontalProps } from './CardHorizontal.types';
 import styles from './CardHorizontal.module.css';
@@ -82,53 +83,52 @@ export const CardHorizontal = ({
   onButtonClick,
   onKeyDown,
   className,
-  ...props
+  ...rest
 }: CardHorizontalProps) => {
-  const handleClick = (e: React.MouseEvent<HTMLElement>) => {
-    if (disabled) {
-      e.preventDefault();
-      return;
-    }
-
+  const { onClick, ...props } = rest;
+  // An explicit onClick={undefined} turns the card body off; consumers use it to keep only the inner button.
+  const hasOnClickProp = 'onClick' in rest;
+  const fireAction = (e: React.MouseEvent<HTMLElement>) => {
     if (typeof onButtonClick === 'function') {
       onButtonClick(e);
     }
     if (infoUrl) {
       openInNewTab(infoUrl);
     }
   };
+  const handleClick = (e: React.MouseEvent<HTMLDivElement>) => {
+    if (disabled) {
+      e.preventDefault();
+      return;
+    }
+    if (hasOnClickProp) {
+      onClick?.(e);
+      return;
+    }
+    fireAction(e);
+  };
   const handleButtonClick = (e: React.MouseEvent<HTMLElement>) => {
     e.stopPropagation();
-    handleClick(e);
+    if (disabled) {
+      e.preventDefault();
+      return;
+    }
+    fireAction(e);
   };
-  const isActionable =
-    typeof props.onClick === 'function' ||
-    typeof onButtonClick === 'function' ||
-    Boolean(infoUrl);
+  const isActionable = hasOnClickProp
+    ? typeof onClick === 'function'
+    : typeof onButtonClick === 'function' || Boolean(infoUrl);
   // With an inner info button, that button is the keyboard action, so the card leaves Enter and Space alone.
   const isButton = isActionable && !infoText;
   const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
-    if (typeof onKeyDown === 'function') {
-      onKeyDown(e);
-    }
-    // Keys from nested controls bubble up here and must not activate the card.
-    const shouldSkipActivation =
-      !isButton || e.defaultPrevented || disabled || e.target !== e.currentTarget;
-    if (shouldSkipActivation) {
-      return;
-    }
-    if (e.key === 'Enter' || e.key === ' ') {
-      e.preventDefault();
-      // A held key repeats keydown, so only the first press activates the card.
-      if (!e.repeat) {
-        // Runs whichever onClick is attached.
-        e.currentTarget.click();
-      }
+    onKeyDown?.(e);
+    if (isButton && !disabled) {
+      clickOnActivationKey(e);
     }
   };
   return (
     <div
-      tabIndex={disabled ? -1 : 0}
+      tabIndex={isButton ? (disabled ? -1 : 0) : undefined}
       aria-disabled={disabled}
       role={isButton ? 'button' : undefined}
       onClick={handleClick}

src/components/CardPrimary/CardPrimary.stories.tsx

@@ -1,4 +1,5 @@
 import { Meta, StoryObj } from '@storybook/react-vite';
+import { expect, fn, userEvent, within } from 'storybook/test';
 import { CardPrimary } from '@/components/CardPrimary';
 
 const meta: Meta<typeof CardPrimary> = {
@@ -101,3 +102,20 @@ export const WithoutButton: Story = {
     alignContent: 'center',
   },
 };
+
+export const Keyboard: Story = {
+  args: {
+    ...WithoutButton.args,
+    onClick: fn(),
+  },
+  play: async ({ canvasElement, args }) => {
+    const card = within(canvasElement).getByRole('button', { name: /Card title/ });
+
+    await userEvent.tab();
+    await expect(card).toHaveFocus();
+
+    await userEvent.keyboard('{Enter}');
+    await userEvent.keyboard(' ');
+    await expect(args.onClick).toHaveBeenCalledTimes(2);
+  },
+};

src/components/CardPrimary/CardPrimary.test.tsx

@@ -1,4 +1,4 @@
-import { screen } from '@testing-library/react';
+import { fireEvent, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { renderCUI } from '@/utils/test-utils';
 import { CardPrimary, CardPrimaryProps } from '@/components/CardPrimary';
@@ -170,4 +170,113 @@ describe('CardPrimary Component', () => {
       expect(warnSpy.mock.calls[0][0]).toContain('javascript:alert(1)');
     });
   });
+
+  describe('keyboard', () => {
+    const renderCard = (props: CardPrimaryProps) => renderCUI(<CardPrimary {...props} />);
+    const getCard = (container: HTMLElement) =>
+      container.querySelector('[aria-disabled]') as HTMLElement;
+
+    it.each(['{Enter}', ' '])(
+      'should call onClick on "%s" and expose a button',
+      async key => {
+        const onClick = vitest.fn();
+        const { container } = renderCard({ title: 'Test Card', onClick });
+
+        const card = screen.getByRole('button', { name: /Test Card/ });
+        expect(card).toBe(getCard(container));
+        card.focus();
+        await userEvent.keyboard(key);
+
+        expect(onClick).toHaveBeenCalledTimes(1);
+      }
+    );
+
+    it('should activate only once while Space is held and keep the page from scrolling', () => {
+      const onClick = vitest.fn();
+      const { container } = renderCard({ title: 'Test Card', onClick });
+
+      const card = getCard(container);
+      expect(fireEvent.keyDown(card, { key: ' ' })).toBe(false);
+      expect(fireEvent.keyDown(card, { key: ' ', repeat: true })).toBe(false);
+
+      expect(onClick).toHaveBeenCalledTimes(1);
+    });
+
+    it('should leave other keys and keys from controls inside the card alone', () => {
+      const onClick = vitest.fn();
+      const { container } = renderCard({
+        title: 'Test Card',
+        onClick,
+        children: <input aria-label="Name" />,
+      });
+
+      expect(fireEvent.keyDown(getCard(container), { key: 'Tab' })).toBe(true);
+      expect(fireEvent.keyDown(screen.getByRole('textbox'), { key: ' ' })).toBe(true);
+      expect(onClick).not.toHaveBeenCalled();
+    });
+
+    it('should run the consumer onKeyDown and still activate', () => {
+      const onClick = vitest.fn();
+      const onKeyDown = vitest.fn();
+      const { container } = renderCard({ title: 'Test Card', onClick, onKeyDown });
+
+      fireEvent.keyDown(getCard(container), { key: 'Enter' });
+
+      expect(onKeyDown).toHaveBeenCalledTimes(1);
+      expect(onClick).toHaveBeenCalledTimes(1);
+    });
+
+    it.each([
+      ['infoText only', { infoText: 'Go' }],
+      ['infoUrl without infoText', { infoUrl: 'https://example.com' }],
+      ['onButtonClick without infoText', { onButtonClick: vitest.fn() }],
+    ])('should be the only button when it has %s', (_name, extra) => {
+      const onClick = vitest.fn();
+      renderCard({ title: 'Test Card', onClick, ...extra });
+
+      fireEvent.keyDown(screen.getByRole('button', { name: /Test Card/ }), {
+        key: 'Enter',
+      });
+
+      expect(onClick).toHaveBeenCalledTimes(1);
+    });
+
+    it('should not run a consumer onClick on a disabled card', () => {
+      const onClick = vitest.fn();
+      const { container } = renderCard({ title: 'Test Card', onClick, disabled: true });
+
+      fireEvent.click(getCard(container));
+
+      expect(onClick).not.toHaveBeenCalled();
+    });
+
+    it('should leave the keys to the inner button and not be a button itself', () => {
+      const onClick = vitest.fn();
+      const { container } = renderCard({
+        title: 'Test Card',
+        infoText: 'Go',
+        onButtonClick: vitest.fn(),
+        onClick,
+      });
+
+      const card = getCard(container);
+      expect(card).not.toHaveAttribute('role');
+      expect(fireEvent.keyDown(card, { key: 'Enter' })).toBe(true);
+      expect(onClick).not.toHaveBeenCalled();
+    });
+
+    it('should keep the default key behavior on a display-only or disabled card', () => {
+      const onClick = vitest.fn();
+      const plain = getCard(renderCard({ title: 'Summary' }).container);
+      expect(plain).not.toHaveAttribute('role');
+      expect(plain).not.toHaveAttribute('tabIndex');
+      expect(fireEvent.keyDown(plain, { key: ' ' })).toBe(true);
+
+      const disabled = getCard(
+        renderCard({ title: 'Test Card', onClick, disabled: true }).container
+      );
+      fireEvent.keyDown(disabled, { key: 'Enter' });
+      expect(onClick).not.toHaveBeenCalled();
+    });
+  });
 });

src/components/CardPrimary/CardPrimary.tsx

@@ -5,6 +5,7 @@ import { Button } from '@/components/Button';
 import { Icon } from '@/components/Icon';
 import { Spacer } from '@/components/Spacer';
 import { cn, cva } from '@/lib/cva';
+import { clickOnActivationKey } from '@/utils/keyboard';
 import { openInNewTab } from '@/utils/url';
 import { CardPrimaryProps } from './CardPrimary.types';
 import styles from './CardPrimary.module.css';
@@ -95,6 +96,8 @@ const Card = ({
   onButtonClick,
   isSelected,
   children,
+  onClick,
+  onKeyDown,
   className,
   ...props
 }: CardPrimaryProps) => {
@@ -106,12 +109,31 @@ const Card = ({
       openInNewTab(infoUrl);
     }
   };
+  const handleCardClick = (e: React.MouseEvent<HTMLDivElement>) => {
+    if (disabled) {
+      e.preventDefault();
+      return;
+    }
+    onClick?.(e);
+  };
 
-  const Component = !!infoUrl || typeof onButtonClick === 'function' ? Button : 'div';
+  const hasInnerButton = !!infoUrl || typeof onButtonClick === 'function';
+  const Component = hasInnerButton ? Button : 'div';
+  // With an inner info button, that button is the keyboard action, so the card leaves Enter and Space alone.
+  const isButton = typeof onClick === 'function' && !(infoText && hasInnerButton);
+  const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
+    onKeyDown?.(e);
+    if (isButton && !disabled) {
+      clickOnActivationKey(e);
+    }
+  };
   return (
     <div
       aria-disabled={disabled}
-      tabIndex={0}
+      tabIndex={isButton ? 0 : undefined}
+      role={isButton ? 'button' : undefined}
+      onClick={handleCardClick}
+      onKeyDown={handleKeyDown}
       {...props}
       className={cn(
         wrapperVariants({ size, align: alignContent, hasShadow, isSelected }),