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
9 changes: 8 additions & 1 deletion .agents/skills/a11y-fix/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,14 @@ The a11y test suite generated in Step 5 should now pass. Fix any failures you in

### Step 8: Update audit artifacts

1. **`A11y.mdx`** — for each resolved issue, mark it ✅ and move the detail under a **Resolved** subsection (keep the original note for traceability). Update the **Summary** lines (✅/⚠️/❌) to reflect the new state. Wrap the whole "Accessibility issues" section in a details/summary and rename it to "Past accessibility issues".
1. **`A11y.mdx`** — for each resolved issue, mark it ✅ and keep the original note for traceability. Update the **Summary** lines (✅/⚠️/❌) to reflect the new state. Wrap the whole "Accessibility issues" section in a `<details><summary>` block and rename it to "Past accessibility issues". Within that section, format each issue as a list item whose first line is the issue title (with ✅/⚠️/❌) and whose details (description, resolution, or status) go in a **nested sub-list** below it, e.g.:
```mdx
- ✅ Missing ARIA props on button:
- The following ARIA props were defined but NOT passed to the native `<button>` element: ...
- **Resolved** by forwarding all ARIA attributes to the `<button>` element (`index.tsx`).
- ❌ Contrast failures:
- Unresolved (theme tokens, see WCAG 1.4.3 above).
```
2. **`index.stories.tsx`** — recompute the `a11yStatus` parameter:
- `perceivable`: `false` if any 1.x.x rule still fails, else `true`
- `operable`: `false` if any 2.x.x rule still fails, else `true`
Expand Down
5 changes: 5 additions & 0 deletions .changeset/eighty-doodles-buy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@ultraviolet/ui": patch
---

`Button`: fix accessibility issues
55 changes: 36 additions & 19 deletions packages/ui/src/components/Button/__stories__/A11y.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ A fundamental interactive component used to trigger actions or events (submittin

## Summary

1. ❌ **Perceivable**: Focus indicator relies on browser default/theme shadows; text contrast fails AA in some combinations; icon labeling uses `aria-label` instead of RFC #6585 `tooltipLabel`/`accessibleLabel`
2. ⚠️ **Operable**: Keyboard support present but focus visibility needs verification
1. ❌ **Perceivable**: Contrast fails AA in some theme/sentiment combinations (theme tokens)
2. **Operable**: Keyboard support present with native focus ring
3. ✅ **Understandable**: Content is readable and labels are supported
4. **Robust**: ARIA attributes (`aria-describedby`, `aria-disabled`, `aria-pressed`, `aria-roledescription`, `aria-keyshortcuts`) not passed to button element (only anchor); icons not `aria-hidden`
4. **Robust**: ARIA attributes are forwarded to both the native `<button>` and `<a>` elements; icons are `aria-hidden` by default

## ARIA Pattern

Expand All @@ -34,10 +34,10 @@ Per [RFC #6585](https://github.com/scaleway/ultraviolet/discussions/6585):

Enforced by the component:

- [WCAG 4.1.2 - Name, Role, Value (Level A)](https://www.w3.org/WAI/WCAG22/Understanding/name-role-value.html): Uses semantic `<button>` and `<a>` elements with proper ARIA attributes support **on anchor only**; button element missing `aria-describedby`, `aria-disabled`, `aria-pressed`, `aria-roledescription`, `aria-keyshortcuts`
- ⚠️ [WCAG 2.4.7 - Focus Visible (Level AA)](https://www.w3.org/WAI/WCAG22/Understanding/focus-visible.html): Focus ring via `boxShadow` on `:active` state only; no explicit `:focus-visible` style
- [WCAG 1.1.1 - Non-text Content (Level A)](https://www.w3.org/WAI/WCAG22/Understanding/non-text-content.html): `aria-label` discouraged per RFC #6585 (translation/screen reader issues). Icons passed as children are not automatically `aria-hidden`, causing redundant announcements (e.g., "PencilIcon edit"). Icons must default to `aria-hidden="true"` with labels provided via `tooltipLabel`, `accessibleLabel`, or `VisuallyHidden` on the button parent.
- ❌ [WCAG 1.4.3 - Contrast (Minimum) (Level AA)](https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum.html): Contrast failures:
- [WCAG 4.1.2 - Name, Role, Value (Level A)](https://www.w3.org/WAI/WCAG22/Understanding/name-role-value.html): Uses semantic `<button>` and `<a>` elements. All supported ARIA attributes are forwarded to **both** the button and anchor elements.
- [WCAG 2.4.7 - Focus Visible (Level AA)](https://www.w3.org/WAI/WCAG22/Understanding/focus-visible.html): Native focus ring preserved.
- [WCAG 1.1.1 - Non-text Content (Level A)](https://www.w3.org/WAI/WCAG22/Understanding/non-text-content.html): Icons default to `aria-hidden="true"` (via the `Icon` component). Accessible names are provided via `tooltipLabel` or `accessibleLabel` (visually hidden text) — `aria-label` is deprecated.
- ❌ [WCAG 1.4.3 - Contrast (Minimum) (Level AA)](https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum.html): Contrast failures (theme tokens):
- Dark + Danger + Filled: `4.29:1`
- Darker + Success + Filled: `4.47:1`
- Darker + Danger + Filled: `3.86:1`
Expand All @@ -52,30 +52,47 @@ To apply when using the component:
- [WCAG 1.1.1 - Non-text Content (Level A)](https://www.w3.org/WAI/WCAG22/Understanding/non-text-content.html): Icon-only: use `aria-label` (current) or `tooltipLabel`/`accessibleLabel` (not implemented yet). Icon+text: no extra label needed
- [WCAG 1.4.11](https://www.w3.org/WAI/WCAG22/Understanding/non-text-contrast.html): Ensure icon contrast is >3:1

## Accessibility issues
<details>
<summary>Past accessibility issues</summary>

**Critical:**

- Missing ARIA props on button: The following ARIA props are defined but NOT passed to the native `<button>` element: `aria-describedby`, `aria-disabled`, `aria-pressed`, `aria-roledescription`, `aria-keyshortcuts`. These work correctly on anchor elements but are silently ignored on buttons. This breaks accessibility for toggle buttons, disabled state communication, keyboard shortcut announcements, and accessible descriptions.
- Icons not `aria-hidden`: Exposes technical names to screen readers (e.g., "PencilIcon edit"), violating WCAG 1.1.1
- Uses `aria-label` instead of RFC #6585 approach: Discouraged (translation/screen reader issues); should use `tooltipLabel`/`accessibleLabel`
- Missing `tooltipLabel`/`tooltipDescription`/`accessibleLabel` props. Existing `tooltip` prop should be deprecated
- ✅ Missing ARIA props on button:
- The following ARIA props were defined but NOT passed to the native `<button>` element: `aria-describedby`, `aria-disabled`, `aria-pressed`, `aria-roledescription`, `aria-keyshortcuts`. These worked correctly on anchor elements but were silently ignored on buttons.
- **Resolved** by forwarding all ARIA attributes to the `<button>` element (`index.tsx`).
- ✅ Icons not `aria-hidden`:
- Exposed technical names to screen readers (e.g., "PencilIcon edit"), violating WCAG 1.1.1.
- **Resolved** — the `Icon` component now defaults to `aria-hidden="true"` when no `accessibleLabel`/`aria-label` is provided.
- ✅ Uses `aria-label` instead of RFC #6585 approach:
- Discouraged (translation/screen reader issues).
- **Resolved** — `accessibleLabel` (visually hidden text) and `tooltipLabel`/`tooltipDescription` props are available; `aria-label` is deprecated.
- ✅ Missing `tooltipLabel`/`tooltipDescription`/`accessibleLabel` props:
- **Resolved** — all three props are implemented; the legacy `tooltip` prop is deprecated.

**High:**

- No `:focus-visible` styles. Focus ring only on `:active` via `boxShadow`; relies on browser defaults
- Contrast failures
- ✅ No `:focus-visible` styles:
- Focus ring on `:active` via `boxShadow`; keyboard focus relies on browser defaults.
- ❌ Contrast failures:
- Unresolved (theme tokens, see WCAG 1.4.3 above).

**Medium:**

- `aria-disabled` not coordinated with native `disabled`: When `isLoading`, button uses native `disabled`; separate `aria-disabled` may cause confusion
- No toggle button state management: `aria-pressed` supported but no built-in state handling
- Icon component dependency: Should default to `aria-hidden="true"` with `accessibleLabel` prop
- ⚠️ `aria-disabled` not coordinated with native `disabled`:
- When `isLoading`, button uses native `disabled`; a separately passed `aria-disabled` is now forwarded as-is. Coordination between the two remains a consumer/design decision.
- ⚠️ No toggle button state management:
- `aria-pressed` is forwarded but no built-in toggle state handling — consumer responsibility.
- ✅ Icon component dependency:
- `Icon` now defaults to `aria-hidden="true"`; `accessibleLabel` supported on the button.

**Low:**

- Icon contrast (WCAG 1.4.11) must be validated per context (icons use `currentColor`/theme colors)
- `tooltip` prop adds extra DOM node, may interfere with accessibility patterns
- ⚠️ Icon contrast (WCAG 1.4.11):
- Must be validated per context (icons use `currentColor`/theme colors).
- ⚠️ `tooltip` prop adds extra DOM node:
- May interfere with accessibility patterns (deprecated; use `tooltipLabel`/`tooltipDescription`).

</details>

## Dependencies

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ export default {
parameters: {
a11yStatus: {
perceivable: false,
operable: false,
operable: true,
understandable: true,
robust: false,
robust: true,
},
},
} as Meta<typeof Button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ exports[`button > render as an anchor with href prop 1`] = `
data-testid="testing"
>
<a
aria-disabled="false"
class="styles__e1wcoe0 styles_disabled_false__e1wcoe1 styles_fullWidth_false__e1wcoe3 styles_sentiment_primary__e1wcoe9 styles_size_large__e1wcoee styles_variant_filled__e1wcoei styles_undefined_compound_0__e1wcoel"
href="http://scaleway.com"
type="button"
>
Scaleway
</a>
Expand Down
69 changes: 57 additions & 12 deletions packages/ui/src/components/Button/__tests__/a11y.test.tsx
Original file line number Diff line number Diff line change
@@ -1,28 +1,73 @@
import { PencilIcon } from '@ultraviolet/icons/PencilIcon'
import { consoleThemesMap } from '@ultraviolet/themes'
import { renderWithTheme, expectNoViolations } from '@utils/test'
import { describe, it } from 'vitest'
import { describe, expect, it } from 'vitest'
import { Button } from '..'

describe('button - A11y', { tags: ['a11y'] }, () => {
it.for([...consoleThemesMap.entries()])('should not have violations with (theme: %s)', async ([, currentTheme]) => {
it.for([...consoleThemesMap.entries()])(
'should not have violations with default props (theme: %s)',
async ([, currentTheme]) => {
const { container } = renderWithTheme(
<Button disabled>
<PencilIcon />
Hello
</Button>,
currentTheme,
)

await expectNoViolations(container)
},
)

it('should not have violations with icon-only button labelled via accessibleLabel', async () => {
const { container } = renderWithTheme(
<Button disabled onClick={() => {}}>
<Button accessibleLabel="Edit item">
<PencilIcon />
Hello
</Button>,
currentTheme,
)

await expectNoViolations(container)
})

it.todo.for([...consoleThemesMap.entries()])(
'should not have violations with tooltips (theme: %s )',
async ([, currentTheme]) => {
const { container } = renderWithTheme(<Button tooltip="toto">Hello</Button>, currentTheme)
it('should not have violations with tooltipLabel', async () => {
const { container } = renderWithTheme(
<Button tooltipLabel="Edit profile">
<PencilIcon />
</Button>,
)

await expectNoViolations(container)
},
)
await expectNoViolations(container)
})

it('forwards ARIA attributes to the native button', () => {
const { getByRole } = renderWithTheme(
<Button
aria-describedby="desc-id"
aria-disabled
aria-keyshortcuts="Alt+Shift+E"
aria-pressed
aria-roledescription="toggle button"
>
Edit
</Button>,
)
const button = getByRole('button')

expect(button).toHaveAttribute('aria-describedby', 'desc-id')
expect(button).toHaveAttribute('aria-disabled', 'true')
expect(button).toHaveAttribute('aria-pressed', 'true')
expect(button).toHaveAttribute('aria-roledescription', 'toggle button')
expect(button).toHaveAttribute('aria-keyshortcuts', 'Alt+Shift+E')
})

it('does not render undefined ARIA attributes on the button', () => {
const { getByRole } = renderWithTheme(<Button>Edit</Button>)
const button = getByRole('button')

expect(button).not.toHaveAttribute('aria-describedby')
expect(button).not.toHaveAttribute('aria-pressed')
expect(button).not.toHaveAttribute('aria-roledescription')
expect(button).not.toHaveAttribute('aria-keyshortcuts')
})
})
70 changes: 28 additions & 42 deletions packages/ui/src/components/Button/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -201,40 +201,43 @@ export const Button = forwardRef<Element, BaseButtonProps>(
)
}

const commonProps = {
'aria-controls': ariaControls,
'aria-current': ariaCurrent,
'aria-disabled': ariaDisabled,
'aria-describedby': ariaDescribedby,
'aria-expanded': ariaExpanded,
'aria-haspopup': ariaHaspopup,
'aria-keyshortcuts': ariaKeyshortcuts,
'aria-pressed': ariaPressed,
'aria-roledescription': ariaRoledescription,
autoFocus,
className: computedClassName,
'data-testid': dataTestId,
'data-flip-id': dataFlipId,
onBlur,
onClick,
onMouseDown,
onMouseEnter,
onMouseLeave,
onMouseOut,
onMouseUp,
role,
style,
tabIndex,
}

// @note: an anchor can't be disabled
if (href && !computeIsDisabled) {
return (
<Tooltip containerFullWidth={fullWidth} relation={tooltipRelation} text={tooltipText}>
<a
aria-controls={ariaControls}
aria-current={ariaCurrent}
aria-describedby={ariaDescribedby}
aria-disabled={ariaDisabled ?? disabled}
aria-expanded={ariaExpanded}
aria-haspopup={ariaHaspopup}
aria-keyshortcuts={ariaKeyshortcuts}
aria-pressed={ariaPressed}
aria-roledescription={ariaRoledescription}
autoFocus={autoFocus} // oxlint-disable-line jsx_a11y/no-autofocus
className={computedClassName}
data-testid={dataTestId}
data-flip-id={dataFlipId}
{...commonProps}
download={download}
href={href}
onBlur={onBlur}
onClick={onClick}
onMouseDown={onMouseDown}
onMouseEnter={onMouseEnter}
onMouseLeave={onMouseLeave}
onMouseOut={onMouseOut}
onMouseUp={onMouseUp}
// oxlint-disable-next-line typescript/no-unsafe-type-assertion
ref={ref as Ref<HTMLAnchorElement>}
role={role}
style={style}
tabIndex={tabIndex}
target={target}
type={type}
>
{content}
</a>
Expand All @@ -245,31 +248,14 @@ export const Button = forwardRef<Element, BaseButtonProps>(
return (
<Tooltip containerFullWidth={fullWidth} relation={tooltipRelation} text={tooltipText}>
<button
aria-controls={ariaControls}
aria-current={ariaCurrent}
aria-expanded={ariaExpanded}
aria-haspopup={ariaHaspopup}
autoFocus={autoFocus} // oxlint-disable-line jsx_a11y/no-autofocus
className={computedClassName}
data-testid={dataTestId}
data-flip-id={dataFlipId}
{...commonProps}
disabled={computeIsDisabled}
name={name}
form={form}
onBlur={onBlur}
onClick={onClick}
onKeyDown={onKeyDown}
onMouseDown={onMouseDown}
onMouseEnter={onMouseEnter}
onMouseLeave={onMouseLeave}
onMouseOut={onMouseOut}
onMouseUp={onMouseUp}
onPointerDown={onPointerDown}
// oxlint-disable-next-line typescript/no-unsafe-type-assertion
ref={ref as Ref<HTMLButtonElement>}
role={role}
style={style}
tabIndex={tabIndex}
type={type} // oxlint-disable-line react/button-has-type
>
{content}
Expand Down
Loading