Skip to content

Commit

Permalink
Form: Fix a11y issue on RadioGroup and ChoiceGroup (#2328)
Browse files Browse the repository at this point in the history
## Summary:

`RadioGroup` and `ChoiceGroup` components are not associating correctly their
labels with their `fieldset` elements. The underlying issue is that these
components are appending a `View` element as the only child of the `fieldset`
element, which is causing the `fieldset` element to not associate correctly the
`legend` value as its label.

This PR fixes that issue by removing the `View` element and using the `legend`
element directly as the first child of the `fieldset` element.


Issue: XXX-XXXX

## Test plan:

Navigate to:

- CheckboxGroup: /?path=/docs/packages-form-checkboxgroup--docs
- RadioGroup: /?path=/docs/packages-form-radiogroup--docs

Verify that the first example associates the `label` value with the `fieldset`
element correctly.

| BEFORE | AFTER |
|--------|--------|
| <img width="1122" alt="Screenshot 2024-09-19 at 5 10 44 PM" src="https://github.com/user-attachments/assets/8e52b298-7280-4215-8b28-481363762be5"> | <img width="1067" alt="Screenshot 2024-09-19 at 5 09 48 PM" src="https://github.com/user-attachments/assets/bc30b90f-21cd-460e-98ff-ed9ebee0003a"> |
| <img width="1087" alt="Screenshot 2024-09-19 at 5 34 49 PM" src="https://github.com/user-attachments/assets/e7ccc91d-9b06-49b0-8da3-31accdccaa69"> | <img width="882" alt="Screenshot 2024-09-19 at 5 33 58 PM" src="https://github.com/user-attachments/assets/c8c8115a-454f-4d6d-967b-c04936eb7076"> |

Author: jandrade

Reviewers: jandrade, beaesguerra

Required Reviewers:

Approved By: beaesguerra

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Lint (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test (ubuntu-latest, 20.x, 2/2), ✅ Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Lint (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️  dependabot, ✅ Publish npm snapshot (ubuntu-latest, 20.x)

Pull Request URL: #2328
  • Loading branch information
jandrade authored Oct 9, 2024
1 parent 0b3a28a commit 8c86195
Show file tree
Hide file tree
Showing 8 changed files with 147 additions and 74 deletions.
5 changes: 5 additions & 0 deletions .changeset/fresh-onions-peel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-form": patch
---

Modify `RadioGroup` and `CheckboxGroup` to append `legend` as the first child in `fieldset`, so the accessibility tree can associate the legend contents with the fieldset group and announce its label correctly
9 changes: 9 additions & 0 deletions __docs__/wonder-blocks-form/checkbox-group.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ export default {

type StoryComponentType = StoryObj<typeof CheckboxGroup>;

/**
* `CheckboxGroup` is a component that groups multiple `Choice` components
* together. It is used to allow users to select multiple options from a list.
*
* Note that by using a `label` prop, the `CheckboxGroup` component will render
* a `legend` as the first child of the `fieldset` element. This is important to
* include as it ensures that Screen Readers can correctly identify and announce
* the group of checkboxes.
*/
export const Default: StoryComponentType = {
render: (args) => {
return (
Expand Down
9 changes: 9 additions & 0 deletions __docs__/wonder-blocks-form/radio-group.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ export default {

type StoryComponentType = StoryObj<typeof RadioGroup>;

/**
* `RadioGroup` is a component that groups multiple `Choice` components
* together. It is used to allow users to select a single option from a list.
*
* Note that by using a `label` prop, the `RadioGroup` component will render
* a `legend` as the first child of the `fieldset` element. This is important to
* include as it ensures that Screen Readers can correctly identify and announce
* the group of radio buttons.
*/
export const Default: StoryComponentType = {
render: (args) => {
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,29 @@ import CheckboxGroup from "../checkbox-group";
import Choice from "../choice";

describe("CheckboxGroup", () => {
describe("a11y", () => {
it("should associate the label with the fieldset", async () => {
// Arrange, Act
render(
<CheckboxGroup
label="Test label"
groupName="test"
onChange={() => {}}
selectedValues={[]}
>
<Choice label="a" value="a" aria-labelledby="test-a" />
<Choice label="b" value="b" aria-labelledby="test-b" />
<Choice label="c" value="c" aria-labelledby="test-c" />
</CheckboxGroup>,
);

const fieldset = screen.getByRole("group", {name: /test label/i});

// Assert
expect(fieldset).toBeInTheDocument();
});
});

describe("behavior", () => {
const TestComponent = ({errorMessage}: {errorMessage?: string}) => {
const [selectedValues, setSelectedValue] = React.useState([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,29 @@ describe("RadioGroup", () => {
);
};

describe("a11y", () => {
it("should associate the label with the fieldset", async () => {
// Arrange, Act
render(
<RadioGroup
label="Test label"
groupName="test"
onChange={() => {}}
selectedValue="a"
>
<Choice label="a" value="a" aria-labelledby="test-a" />
<Choice label="b" value="b" aria-labelledby="test-b" />
<Choice label="c" value="c" aria-labelledby="test-c" />
</RadioGroup>,
);

const fieldset = screen.getByRole("group", {name: /test label/i});

// Assert
expect(fieldset).toBeInTheDocument();
});
});

describe("behavior", () => {
it("selects only one item at a time", async () => {
// Arrange, Act
Expand Down
75 changes: 38 additions & 37 deletions packages/wonder-blocks-form/src/components/checkbox-group.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from "react";

import {View, addStyle} from "@khanacademy/wonder-blocks-core";
import {addStyle} from "@khanacademy/wonder-blocks-core";
import {Strut} from "@khanacademy/wonder-blocks-layout";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelMedium, LabelSmall} from "@khanacademy/wonder-blocks-typography";
Expand Down Expand Up @@ -130,43 +130,44 @@ const CheckboxGroup = React.forwardRef(function CheckboxGroup(
const allChildren = React.Children.toArray(children).filter(Boolean);

return (
<StyledFieldset data-testid={testId} style={styles.fieldset} ref={ref}>
{/* We have a View here because fieldset cannot be used with flexbox*/}
<View style={style}>
{label && (
<StyledLegend style={styles.legend}>
<LabelMedium>{label}</LabelMedium>
</StyledLegend>
)}
{description && (
<LabelSmall style={styles.description}>
{description}
</LabelSmall>
)}
{errorMessage && (
<LabelSmall style={styles.error}>{errorMessage}</LabelSmall>
)}
{(label || description || errorMessage) && (
<Strut size={spacing.small_12} />
)}
<StyledFieldset
data-testid={testId}
style={[styles.fieldset, style]}
ref={ref}
>
{label && (
<StyledLegend style={styles.legend}>
<LabelMedium>{label}</LabelMedium>
</StyledLegend>
)}
{description && (
<LabelSmall style={styles.description}>
{description}
</LabelSmall>
)}
{errorMessage && (
<LabelSmall style={styles.error}>{errorMessage}</LabelSmall>
)}
{(label || description || errorMessage) && (
<Strut size={spacing.small_12} />
)}

{allChildren.map((child, index) => {
// @ts-expect-error [FEI-5019] - TS2339 - Property 'props' does not exist on type 'ReactChild | ReactFragment | ReactPortal'.
const {style, value} = child.props;
const checked = selectedValues.includes(value);
// @ts-expect-error [FEI-5019] - TS2769 - No overload matches this call.
return React.cloneElement(child, {
checked: checked,
error: !!errorMessage,
groupName: groupName,
id: `${groupName}-${value}`,
key: value,
onChange: () => handleChange(value, checked),
style: [index > 0 && styles.defaultLineGap, style],
variant: "checkbox",
});
})}
</View>
{allChildren.map((child, index) => {
// @ts-expect-error [FEI-5019] - TS2339 - Property 'props' does not exist on type 'ReactChild | ReactFragment | ReactPortal'.
const {style, value} = child.props;
const checked = selectedValues.includes(value);
// @ts-expect-error [FEI-5019] - TS2769 - No overload matches this call.
return React.cloneElement(child, {
checked: checked,
error: !!errorMessage,
groupName: groupName,
id: `${groupName}-${value}`,
key: value,
onChange: () => handleChange(value, checked),
style: [index > 0 && styles.defaultLineGap, style],
variant: "checkbox",
});
})}
</StyledFieldset>
);
});
Expand Down
2 changes: 2 additions & 0 deletions packages/wonder-blocks-form/src/components/group-styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import type {StyleDeclaration} from "aphrodite";

const styles: StyleDeclaration = StyleSheet.create({
fieldset: {
display: "flex",
flexDirection: "column",
border: "none",
padding: 0,
margin: 0,
Expand Down
75 changes: 38 additions & 37 deletions packages/wonder-blocks-form/src/components/radio-group.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from "react";

import {View, addStyle} from "@khanacademy/wonder-blocks-core";
import {addStyle} from "@khanacademy/wonder-blocks-core";
import {Strut} from "@khanacademy/wonder-blocks-layout";
import {spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelMedium, LabelSmall} from "@khanacademy/wonder-blocks-typography";
Expand Down Expand Up @@ -115,43 +115,44 @@ const RadioGroup = React.forwardRef(function RadioGroup(
const allChildren = React.Children.toArray(children).filter(Boolean);

return (
<StyledFieldset data-testid={testId} style={styles.fieldset} ref={ref}>
{/* We have a View here because fieldset cannot be used with flexbox*/}
<View style={style}>
{label && (
<StyledLegend style={styles.legend}>
<LabelMedium>{label}</LabelMedium>
</StyledLegend>
)}
{description && (
<LabelSmall style={styles.description}>
{description}
</LabelSmall>
)}
{errorMessage && (
<LabelSmall style={styles.error}>{errorMessage}</LabelSmall>
)}
{(label || description || errorMessage) && (
<Strut size={spacing.small_12} />
)}
<StyledFieldset
data-testid={testId}
style={[styles.fieldset, style]}
ref={ref}
>
{label && (
<StyledLegend style={styles.legend}>
<LabelMedium>{label}</LabelMedium>
</StyledLegend>
)}
{description && (
<LabelSmall style={styles.description}>
{description}
</LabelSmall>
)}
{errorMessage && (
<LabelSmall style={styles.error}>{errorMessage}</LabelSmall>
)}
{(label || description || errorMessage) && (
<Strut size={spacing.small_12} />
)}

{allChildren.map((child, index) => {
// @ts-expect-error [FEI-5019] - TS2339 - Property 'props' does not exist on type 'ReactChild | ReactFragment | ReactPortal'.
const {style, value} = child.props;
const checked = selectedValue === value;
// @ts-expect-error [FEI-5019] - TS2769 - No overload matches this call.
return React.cloneElement(child, {
checked: checked,
error: !!errorMessage,
groupName: groupName,
id: `${groupName}-${value}`,
key: value,
onChange: () => onChange(value),
style: [index > 0 && styles.defaultLineGap, style],
variant: "radio",
});
})}
</View>
{allChildren.map((child, index) => {
// @ts-expect-error [FEI-5019] - TS2339 - Property 'props' does not exist on type 'ReactChild | ReactFragment | ReactPortal'.
const {style, value} = child.props;
const checked = selectedValue === value;
// @ts-expect-error [FEI-5019] - TS2769 - No overload matches this call.
return React.cloneElement(child, {
checked: checked,
error: !!errorMessage,
groupName: groupName,
id: `${groupName}-${value}`,
key: value,
onChange: () => onChange(value),
style: [index > 0 && styles.defaultLineGap, style],
variant: "radio",
});
})}
</StyledFieldset>
);
});
Expand Down

0 comments on commit 8c86195

Please sign in to comment.