From 8c8619552344bba1557cf384eed43fc1b1bef06d Mon Sep 17 00:00:00 2001 From: Juan Andrade Date: Wed, 9 Oct 2024 14:28:37 -0400 Subject: [PATCH] Form: Fix a11y issue on RadioGroup and ChoiceGroup (#2328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 | |--------|--------| | Screenshot 2024-09-19 at 5 10 44 PM | Screenshot 2024-09-19 at 5 09 48 PM | | Screenshot 2024-09-19 at 5 34 49 PM | Screenshot 2024-09-19 at 5 33 58 PM | 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: https://github.com/Khan/wonder-blocks/pull/2328 --- .changeset/fresh-onions-peel.md | 5 ++ .../checkbox-group.stories.tsx | 9 +++ .../radio-group.stories.tsx | 9 +++ .../__tests__/checkbox-group.test.tsx | 23 ++++++ .../components/__tests__/radio-group.test.tsx | 23 ++++++ .../src/components/checkbox-group.tsx | 75 ++++++++++--------- .../src/components/group-styles.ts | 2 + .../src/components/radio-group.tsx | 75 ++++++++++--------- 8 files changed, 147 insertions(+), 74 deletions(-) create mode 100644 .changeset/fresh-onions-peel.md diff --git a/.changeset/fresh-onions-peel.md b/.changeset/fresh-onions-peel.md new file mode 100644 index 000000000..7254f0124 --- /dev/null +++ b/.changeset/fresh-onions-peel.md @@ -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 diff --git a/__docs__/wonder-blocks-form/checkbox-group.stories.tsx b/__docs__/wonder-blocks-form/checkbox-group.stories.tsx index 4b9a0091f..120cf52b2 100644 --- a/__docs__/wonder-blocks-form/checkbox-group.stories.tsx +++ b/__docs__/wonder-blocks-form/checkbox-group.stories.tsx @@ -26,6 +26,15 @@ export default { type StoryComponentType = StoryObj; +/** + * `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 ( diff --git a/__docs__/wonder-blocks-form/radio-group.stories.tsx b/__docs__/wonder-blocks-form/radio-group.stories.tsx index a3379e479..8bc5f926d 100644 --- a/__docs__/wonder-blocks-form/radio-group.stories.tsx +++ b/__docs__/wonder-blocks-form/radio-group.stories.tsx @@ -24,6 +24,15 @@ export default { type StoryComponentType = StoryObj; +/** + * `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 ( diff --git a/packages/wonder-blocks-form/src/components/__tests__/checkbox-group.test.tsx b/packages/wonder-blocks-form/src/components/__tests__/checkbox-group.test.tsx index e2d3cf33d..c3234d410 100644 --- a/packages/wonder-blocks-form/src/components/__tests__/checkbox-group.test.tsx +++ b/packages/wonder-blocks-form/src/components/__tests__/checkbox-group.test.tsx @@ -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( + {}} + selectedValues={[]} + > + + + + , + ); + + 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([ diff --git a/packages/wonder-blocks-form/src/components/__tests__/radio-group.test.tsx b/packages/wonder-blocks-form/src/components/__tests__/radio-group.test.tsx index e2a5e21c3..b832143c6 100644 --- a/packages/wonder-blocks-form/src/components/__tests__/radio-group.test.tsx +++ b/packages/wonder-blocks-form/src/components/__tests__/radio-group.test.tsx @@ -34,6 +34,29 @@ describe("RadioGroup", () => { ); }; + describe("a11y", () => { + it("should associate the label with the fieldset", async () => { + // Arrange, Act + render( + {}} + selectedValue="a" + > + + + + , + ); + + 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 diff --git a/packages/wonder-blocks-form/src/components/checkbox-group.tsx b/packages/wonder-blocks-form/src/components/checkbox-group.tsx index 72091a8a8..1424f5f08 100644 --- a/packages/wonder-blocks-form/src/components/checkbox-group.tsx +++ b/packages/wonder-blocks-form/src/components/checkbox-group.tsx @@ -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"; @@ -130,43 +130,44 @@ const CheckboxGroup = React.forwardRef(function CheckboxGroup( const allChildren = React.Children.toArray(children).filter(Boolean); return ( - - {/* We have a View here because fieldset cannot be used with flexbox*/} - - {label && ( - - {label} - - )} - {description && ( - - {description} - - )} - {errorMessage && ( - {errorMessage} - )} - {(label || description || errorMessage) && ( - - )} + + {label && ( + + {label} + + )} + {description && ( + + {description} + + )} + {errorMessage && ( + {errorMessage} + )} + {(label || description || errorMessage) && ( + + )} - {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", - }); - })} - + {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", + }); + })} ); }); diff --git a/packages/wonder-blocks-form/src/components/group-styles.ts b/packages/wonder-blocks-form/src/components/group-styles.ts index d8a01ff33..6ce5e91c9 100644 --- a/packages/wonder-blocks-form/src/components/group-styles.ts +++ b/packages/wonder-blocks-form/src/components/group-styles.ts @@ -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, diff --git a/packages/wonder-blocks-form/src/components/radio-group.tsx b/packages/wonder-blocks-form/src/components/radio-group.tsx index c529bd2d2..33f9e06d5 100644 --- a/packages/wonder-blocks-form/src/components/radio-group.tsx +++ b/packages/wonder-blocks-form/src/components/radio-group.tsx @@ -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"; @@ -115,43 +115,44 @@ const RadioGroup = React.forwardRef(function RadioGroup( const allChildren = React.Children.toArray(children).filter(Boolean); return ( - - {/* We have a View here because fieldset cannot be used with flexbox*/} - - {label && ( - - {label} - - )} - {description && ( - - {description} - - )} - {errorMessage && ( - {errorMessage} - )} - {(label || description || errorMessage) && ( - - )} + + {label && ( + + {label} + + )} + {description && ( + + {description} + + )} + {errorMessage && ( + {errorMessage} + )} + {(label || description || errorMessage) && ( + + )} - {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", - }); - })} - + {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", + }); + })} ); });