fix(a11y): make settings rows keyboard-accessible (#1430)

src/components/settings/SeedPhraseDialog.tsx

@@ -1,7 +1,6 @@
 import { useState, useEffect, useMemo, type ComponentProps } from 'react'
 import { getseedOptions } from '@joinmarket-webui/joinmarket-ng-api-ts/@tanstack/react-query'
 import { useQuery, useQueryClient } from '@tanstack/react-query'
-import { cx } from 'class-variance-authority'
 import { AlertTriangleIcon, ClockIcon } from 'lucide-react'
 import { useTranslation } from 'react-i18next'
 import { Alert, AlertDescription, AlertTitle } from '@/components/ui/alert'
@@ -17,7 +16,7 @@ import {
 import { Label } from '@/components/ui/label'
 import { useApiClient } from '@/hooks/useApiClient'
 import { getErrorReason } from '@/lib/errorReason'
-import type { WalletFileName } from '@/lib/utils'
+import { cn, type WalletFileName } from '@/lib/utils'
 import type { Milliseconds, WithRequiredProperty } from '@/types/global'
 import { SeedPhraseGrid } from '../ui/jam/SeedPhraseGrid'
 import { Spinner } from '../ui/spinner'
@@ -189,13 +188,13 @@ export const SeedPhraseDialog = ({
             <DialogFooter>
               <div className="flex w-full items-center justify-between">
                 <div
-                  className={cx('text-muted-foreground flex items-center gap-1 text-sm', {
+                  className={cn('text-muted-foreground flex items-center gap-1 text-sm', {
                     'text-destructive! animate-pulse': secondsLeft <= 10,
                   })}
                 >
                   <ClockIcon className="h-4 w-4" />
                   <span
-                    className={cx('mt-0.5 font-mono', {
+                    className={cn('mt-0.5 font-mono', {
                       hidden: secondsLeft < 1,
                     })}
                   >

src/components/settings/SettingsItem.test.tsx

@@ -0,0 +1,183 @@
+import { render, screen } from '@testing-library/react'
+import user from '@testing-library/user-event'
+import { KeyRoundIcon } from 'lucide-react'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { SettingItem, SettingsLink, SettingSwitch } from './SettingsItem'
+
+const mocks = vi.hoisted(() => ({
+  navigate: vi.fn(),
+  open: vi.fn(),
+}))
+
+vi.mock('react-router-dom', () => ({
+  useNavigate: () => mocks.navigate,
+}))
+
+vi.mock('@/components/ui/switch', () => ({
+  Switch: ({
+    checked,
+    disabled,
+    onCheckedChange,
+  }: {
+    checked?: boolean
+    disabled?: boolean
+    onCheckedChange?: (checked: boolean) => void
+  }) => (
+    <button type="button" disabled={disabled} onClick={() => onCheckedChange?.(!checked)}>
+      switch:{String(checked)}
+    </button>
+  ),
+}))
+
+describe('SettingItem', () => {
+  beforeEach(() => {
+    mocks.navigate.mockReset()
+    mocks.open.mockReset()
+  })
+
+  it('renders title and icon', () => {
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" />)
+
+    expect(screen.getByText('settings.show_seed')).toBeInTheDocument()
+  })
+
+  it('exposes an actionable row as a button', () => {
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={vi.fn()} />)
+
+    expect(screen.getByRole('button', { name: 'settings.show_seed' })).toBeInTheDocument()
+  })
+
+  it('does not render a button without an action', () => {
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" />)
+
+    expect(screen.queryByRole('button')).not.toBeInTheDocument()
+  })
+
+  it('is reachable by keyboard', async () => {
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={vi.fn()} />)
+
+    await user.tab()
+
+    expect(screen.getByRole('button', { name: 'settings.show_seed' })).toHaveFocus()
+  })
+
+  it('triggers the action on Enter', async () => {
+    const action = vi.fn()
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={action} />)
+
+    await user.tab()
+    await user.keyboard('{Enter}')
+
+    expect(action).toHaveBeenCalledOnce()
+  })
+
+  it('triggers the action on Space', async () => {
+    const action = vi.fn()
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={action} />)
+
+    await user.tab()
+    await user.keyboard(' ')
+
+    expect(action).toHaveBeenCalledOnce()
+  })
+
+  it('triggers the action on click', async () => {
+    const action = vi.fn()
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={action} />)
+
+    await user.click(screen.getByRole('button', { name: 'settings.show_seed' }))
+
+    expect(action).toHaveBeenCalledOnce()
+  })
+
+  it('does not trigger the action while disabled', async () => {
+    const action = vi.fn()
+    render(<SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={action} disabled={true} />)
+
+    const item = screen.getByRole('button', { name: 'settings.show_seed' })
+    expect(item).toBeDisabled()
+
+    await user.click(item)
+    await user.tab()
+    await user.keyboard('{Enter}')
+
+    expect(action).not.toHaveBeenCalled()
+  })
+
+  it('stays a wrapper when children provide their own control', () => {
+    render(
+      <SettingItem icon={KeyRoundIcon} title="settings.show_seed" action={vi.fn()} hasInteractiveChild={true}>
+        <button type="button">child-control</button>
+      </SettingItem>,
+    )
+
+    // no button is nested inside another button
+    expect(screen.queryByRole('button', { name: 'settings.show_seed' })).not.toBeInTheDocument()
+    expect(screen.getByRole('button', { name: 'child-control' })).toBeInTheDocument()
+  })
+})
+
+describe('SettingsLink', () => {
+  beforeEach(() => {
+    mocks.navigate.mockReset()
+  })
+
+  it('navigates internally on Enter', async () => {
+    render(<SettingsLink icon={KeyRoundIcon} title="settings.rescan_chain" to="/rescan" />)
+
+    await user.tab()
+    await user.keyboard('{Enter}')
+
+    expect(mocks.navigate).toHaveBeenCalledWith('/rescan')
+  })
+
+  it('opens external links in a new tab on Enter', async () => {
+    vi.stubGlobal('open', mocks.open)
+    render(<SettingsLink icon={KeyRoundIcon} title="settings.documentation" to="https://example.org" external={true} />)
+
+    await user.tab()
+    await user.keyboard('{Enter}')
+
+    expect(mocks.open).toHaveBeenCalledWith('https://example.org', '_blank', 'noreferrer,noopener')
+    expect(mocks.navigate).not.toHaveBeenCalled()
+  })
+})
+
+describe('SettingSwitch', () => {
+  it('is keyboard operable via the row when no toggle is displayed', async () => {
+    const onCheckedChange = vi.fn()
+    render(
+      <SettingSwitch
+        icon={KeyRoundIcon}
+        title="settings.hide_balance"
+        checked={false}
+        onCheckedChange={onCheckedChange}
+        displayToggle={false}
+      />,
+    )
+
+    await user.tab()
+    await user.keyboard('{Enter}')
+
+    expect(onCheckedChange).toHaveBeenCalledWith(true)
+  })
+
+  it('is keyboard operable via the toggle when one is displayed', async () => {
+    const onCheckedChange = vi.fn()
+    render(
+      <SettingSwitch
+        icon={KeyRoundIcon}
+        title="settings.use_address_chunking_enabled"
+        checked={false}
+        onCheckedChange={onCheckedChange}
+        displayToggle={true}
+      />,
+    )
+
+    await user.tab()
+    expect(screen.getByRole('button', { name: 'switch:false' })).toHaveFocus()
+
+    await user.keyboard('{Enter}')
+    expect(onCheckedChange).toHaveBeenCalledWith(true)
+  })
+})

src/components/settings/SettingsItem.tsx

@@ -1,9 +1,9 @@
 import type { PropsWithChildren, ReactNode } from 'react'
-import { cx } from 'class-variance-authority'
 import { ExternalLinkIcon } from 'lucide-react'
 import type { LucideIcon } from 'lucide-react'
 import { useNavigate } from 'react-router-dom'
 import { Switch } from '@/components/ui/switch'
+import { cn } from '@/lib/utils'
 
 type SettingsItemProps = PropsWithChildren<{
   icon?: LucideIcon
@@ -11,6 +11,12 @@ type SettingsItemProps = PropsWithChildren<{
   title: string
   disabled?: boolean
   action?: () => void | Promise<void>
+  /**
+   * Set when {@link children} already renders its own focusable control (e.g. a `Switch`).
+   * The row then stays a plain wrapper instead of becoming a button, so a button is never
+   * nested inside a button - keyboard users operate the child control directly.
+   */
+  hasInteractiveChild?: boolean
 }>
 
 export const SettingItem = ({
@@ -19,16 +25,22 @@ export const SettingItem = ({
   title,
   action,
   disabled = false,
+  hasInteractiveChild = false,
   children,
 }: SettingsItemProps) => {
+  const rowClassName = cn('flex min-w-0 items-center justify-between gap-2 py-2', {
+    'hover:bg-muted/50 cursor-pointer rounded-md px-2': !disabled,
+    'cursor-not-allowed opacity-60': disabled,
+  })
+  // The row bleeds into the card padding so the hover background spans the full width.
+  // A block `div` with `width: auto` gets this from the negative margin alone, but a
+  // `button` does not stretch to its parent, and once `w-full` pins the width the
+  // negative margin no longer widens it. Keeping the bleed on a wrapper makes `w-full`
+  // resolve against the widened box, so both variants end up the same size.
+  const bleedClassName = !disabled ? '-mx-2' : undefined
+
   const content = (
-    <div
+    <>
-      className={cx('flex min-w-0 items-center justify-between gap-2 py-2', {
-        'hover:bg-muted/50 -mx-2 cursor-pointer rounded-md px-2': !disabled,
-        'cursor-not-allowed opacity-60': disabled,
-      })}
-      onClick={!disabled && action ? () => void action() : undefined}
-    >
       <div className="flex min-w-0 items-center gap-2">
         <div className="bg-muted/50 flex h-7 w-7 shrink-0 items-center justify-center rounded-lg border">
           {Icon && <Icon className="text-muted-foreground h-4 w-4 align-middle" />}
@@ -39,13 +51,34 @@ export const SettingItem = ({
         </div>
       </div>
       {children !== undefined && <div className="shrink-0">{children}</div>}
-    </div>
+    </>
   )
 
-  return content
+  // A row that is itself the control must be a real button: the browser then provides
+  // focus, Enter/Space activation and the correct role for free.
+  if (action && !hasInteractiveChild) {
+    return (
+      <div className={bleedClassName}>
+        <button
+          type="button"
+          className={cn(rowClassName, 'w-full appearance-none border-0 bg-transparent text-left')}
+          onClick={() => void action()}
+          disabled={disabled}
+        >
+          {content}
+        </button>
+      </div>
+    )
+  }
+
+  return (
+    <div className={cn(bleedClassName, rowClassName)} onClick={!disabled && action ? () => void action() : undefined}>
+      {content}
+    </div>
+  )
 }
 
-type SettingsLinkProps = Omit<SettingsItemProps, 'action' | 'children'> & {
+type SettingsLinkProps = Omit<SettingsItemProps, 'action' | 'children' | 'hasInteractiveChild'> & {
   to: string
   external?: boolean
 }
@@ -69,14 +102,17 @@ export const SettingsLink = ({ to, external = false, ...props }: SettingsLinkPro
   )
 }
 
-type SettingsSwitchProps = Omit<SettingsItemProps, 'children' | 'action'> & {
+type SettingsSwitchProps = Omit<SettingsItemProps, 'children' | 'action' | 'hasInteractiveChild'> & {
   checked?: boolean
   onCheckedChange?: (checked: boolean) => void
   displayToggle?: boolean
 }
 export const SettingSwitch = ({ checked, onCheckedChange, displayToggle = true, ...props }: SettingsSwitchProps) => {
   return (
-    <SettingItem {...props} action={() => onCheckedChange?.(!checked)}>
+    // With a visible toggle the `Switch` is the focusable control and the row stays a
+    // wrapper. Without one the row itself has to be the button, or the setting cannot be
+    // reached by keyboard at all.
+    <SettingItem {...props} action={() => onCheckedChange?.(!checked)} hasInteractiveChild={displayToggle}>
       {displayToggle && <Switch checked={checked} onCheckedChange={onCheckedChange} disabled={props.disabled} />}
     </SettingItem>
   )