From 66f233d45bce8576399146fddd869e332f267fc6 Mon Sep 17 00:00:00 2001 From: Juan Andrade Date: Tue, 5 Dec 2023 13:56:32 -0500 Subject: [PATCH] WB-1636: Announce popover contents correctly to screen readers (#2134) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: The Popover dialog element is not assigning correctly the `aria-describedby` value, thus causing Screen Readers not being able to read the popover contents correctly. This PR fixes the issue by assigning the `aria-describedby` value to the popover content, and also associates the popover title with the popover dialog via the `aria-labelledby` attribute. By implementing this, the popover dialog will be read correctly by Screen Readers. Also fixed some issues with the Storybook docs by adding correct descriptions and fixing some of the Arg types to the Popover component. Issue: WB-1636 ## Test plan: This is better to be tested with Safari + VoiceOver. 1. Open any popover story in Storybook and click on the popover trigger. 2. Verify that the popover dialog is read correctly by VoiceOver. The popover title should be read as soon as the popover dialog is opened. BEFORE popover-sr-before AFTER popover-sr-after Author: jandrade Reviewers: jandrade, jeresig Required Reviewers: Approved By: jeresig Checks: ✅ codecov/project, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Lint (ubuntu-latest, 16.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 16.x), ⏭ Chromatic - Skip on Release PR (changesets), 🚫 Chromatic - Get results on regular PRs, ✅ Publish npm snapshot (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ gerald, ✅ Test (ubuntu-latest, 16.x, 2/2), ✅ Check build sizes (ubuntu-latest, 16.x), ✅ Test (ubuntu-latest, 16.x, 1/2), ✅ Lint (ubuntu-latest, 16.x), ✅ Publish npm snapshot (ubuntu-latest, 16.x), 🚫 Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 16.x), ⏭ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ⏭ dependabot, ✅ gerald Pull Request URL: https://github.com/Khan/wonder-blocks/pull/2134 --- .changeset/dirty-dingos-battle.md | 5 + .../popover-content-core.stories.tsx | 18 ++-- .../popover-content.stories.tsx | 8 +- .../popover.accessibility.stories.mdx | 7 +- .../popover.argtypes.tsx | 95 +++++++++++++++---- .../wonder-blocks-popover/popover.stories.tsx | 64 +++++++++---- .../src/components/__tests__/popover.test.tsx | 69 ++++++++++++++ .../src/components/popover-content-core.tsx | 5 +- .../src/components/popover-content.tsx | 16 +++- .../src/components/popover-dialog.tsx | 2 + .../src/components/popover.tsx | 34 +++---- 11 files changed, 250 insertions(+), 73 deletions(-) create mode 100644 .changeset/dirty-dingos-battle.md diff --git a/.changeset/dirty-dingos-battle.md b/.changeset/dirty-dingos-battle.md new file mode 100644 index 000000000..c82f45e0a --- /dev/null +++ b/.changeset/dirty-dingos-battle.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/wonder-blocks-popover": patch +--- + +Pass the correct ID to the title and content so screen readers can announce popovers correctly diff --git a/__docs__/wonder-blocks-popover/popover-content-core.stories.tsx b/__docs__/wonder-blocks-popover/popover-content-core.stories.tsx index 9383d2934..0a81e6b27 100644 --- a/__docs__/wonder-blocks-popover/popover-content-core.stories.tsx +++ b/__docs__/wonder-blocks-popover/popover-content-core.stories.tsx @@ -34,15 +34,6 @@ export default { version={packageConfig.version} /> ), - docs: { - description: { - component: null, - }, - source: { - // See https://github.com/storybookjs/storybook/issues/12596 - excludeDecorators: true, - }, - }, }, decorators: [ (Story: any): React.ReactElement> => ( @@ -94,14 +85,17 @@ export const WithIcon: StoryComponentType = { <> - This is an article - With the content + + This is an article + + With the content ), closeButtonVisible: true, style: styles.popoverWithIcon, }, + render: (args) => , }; // NOTE: Adding a wrapper to cast the component so Storybook doesn't complain. @@ -116,6 +110,7 @@ export const WithDetailCell: StoryComponentType = { children: , style: styles.popoverWithCell, }, + render: (args) => , }; WithDetailCell.parameters = { @@ -179,6 +174,7 @@ export const Dark: StoryComponentType = { color: "darkBlue", style: styles.customPopover, }, + render: (args) => , }; Dark.parameters = { diff --git a/__docs__/wonder-blocks-popover/popover-content.stories.tsx b/__docs__/wonder-blocks-popover/popover-content.stories.tsx index 359176717..bdb91db65 100644 --- a/__docs__/wonder-blocks-popover/popover-content.stories.tsx +++ b/__docs__/wonder-blocks-popover/popover-content.stories.tsx @@ -59,6 +59,7 @@ export const Default: StoryComponentType = { content: "The default version only includes text.", closeButtonVisible: true, }, + render: (args) => , }; Default.storyName = "Default (text)"; @@ -89,6 +90,7 @@ export const Emphasized: StoryComponentType = { ), }, + render: (args) => , }; Emphasized.parameters = { @@ -106,8 +108,9 @@ export const WithIcon: StoryComponentType = { args: { title: "Popover with Icon", content: "Popovers can include images on the left.", - icon: Wonder Blocks logo, + icon: Wonder Blocks logo, }, + render: (args) => , }; WithIcon.parameters = { @@ -125,7 +128,7 @@ export const WithIllustration: StoryComponentType = { "As you can see, this popover includes a full-bleed illustration.", image: ( An illustration of a person skating on a pencil , }; WithIllustration.parameters = { diff --git a/__docs__/wonder-blocks-popover/popover.accessibility.stories.mdx b/__docs__/wonder-blocks-popover/popover.accessibility.stories.mdx index 6d50052bb..94e714496 100644 --- a/__docs__/wonder-blocks-popover/popover.accessibility.stories.mdx +++ b/__docs__/wonder-blocks-popover/popover.accessibility.stories.mdx @@ -32,9 +32,12 @@ The Popover component should follow these guidelines: **Popover Dialog:** The popover component will populate the +[aria-labelledby](https://www.w3.org/TR/wai-aria/#aria-labelledby) and [aria-describedby](https://www.w3.org/TR/wai-aria-1.1/#aria-describedby) -attribute automatically, unless the user sets an `id` prop inside the Popover -instance. Internally, it will be set on the trigger element. +attributes automatically. Internally, it will be set in the `dialog` element and +the `PopoverContent` component to reference the `title` and `content` +elements/props. By doing this, the popover will be announced by screen readers +as a dialog with a title and content. ## Keyboard Interaction diff --git a/__docs__/wonder-blocks-popover/popover.argtypes.tsx b/__docs__/wonder-blocks-popover/popover.argtypes.tsx index 10ba93b68..69cbec8f5 100644 --- a/__docs__/wonder-blocks-popover/popover.argtypes.tsx +++ b/__docs__/wonder-blocks-popover/popover.argtypes.tsx @@ -14,42 +14,103 @@ import { } from "./popover-content-core.stories"; // NOTE: Casting to any to avoid type errors. -const DefaultWrapper = Default as React.ElementType; -const EmphasizedWrapper = Emphasized as React.ElementType; -const WithIconWrapper = WithIcon as React.ElementType; -const WithIllustrationWrapper = WithIllustration as React.ElementType; -const CoreWithIconWrapper = CoreWithIcon as React.ElementType; -const CoreWithDetailCellWrapper = CoreWithDetailCell as React.ElementType; -const CoreDarkWrapper = CoreDark as React.ElementType; +const DefaultWrapper = Default as any; +const EmphasizedWrapper = Emphasized as any; +const WithIconWrapper = WithIcon as any; +const WithIllustrationWrapper = WithIllustration as any; +const CoreWithIconWrapper = CoreWithIcon as any; +const CoreWithDetailCellWrapper = CoreWithDetailCell as any; +const CoreDarkWrapper = CoreDark as any; -export const ContentMappings: { - [key: string]: React.ReactNode; -} = { - withTextOnly: , - withEmphasis: , - withIcon: , - withIllustration: , - coreWithIcon: , - coreWithCell: , - coreDark: , +// NOTE: We have to use the `render` method to fix a bug in Storybook where +// reusable stories don't render properly with CSF v3. +// See https://github.com/storybookjs/storybook/issues/15954#issuecomment-1835905271 +export const ContentMappings = { + withTextOnly: DefaultWrapper.render({...Default.args}), + withEmphasis: EmphasizedWrapper.render({...Emphasized.args}), + withIcon: WithIconWrapper.render({...WithIcon.args}), + withIllustration: WithIllustrationWrapper.render({ + ...WithIllustration.args, + }), + coreWithIcon: CoreWithIconWrapper.render({...CoreWithIcon.args}), + coreWithCell: CoreWithDetailCellWrapper.render({ + ...CoreWithDetailCell.args, + }), + coreDark: CoreDarkWrapper.render({...CoreDark.args}), }; export default { children: { + description: + `The element that triggers the popover. This element will be ` + + `used to position the popover. It can be either a Node or a ` + + `function using the children-as-function pattern to pass an open ` + + `function for use anywhere within children. The latter provides ` + + `a lot of flexibility in terms of what actions may trigger the ` + + `\`Popover\` to launch the popover dialog.`, control: { type: null, }, }, placement: { + description: + `Where the popover should try to appear in relation to the ` + + `trigger element.,`, control: { type: "select", options: ["top", "bottom", "right", "left"], }, }, content: { + description: + `The content of the popover. You can either use ` + + `[PopoverContent](../?path=/docs/popover-popovercontent--docs) ` + + `with one of the pre-defined variants, or include your own ` + + `custom content using ` + + `[PopoverContentCore](../?path=/docs/popover-popovercontentcore--docs) ` + + `directly.`, control: {type: "select"}, defaultValue: ContentMappings.withTextOnly, options: Object.keys(ContentMappings) as Array, mapping: ContentMappings, }, + id: { + description: + `The unique identifier to give to the popover. Provide ` + + `this in cases where you want to override the default accessibility ` + + `solution. This identifier will be applied to the popover title and ` + + `content.\n\n` + + `This is also used as a prefix to the IDs of the popover's elements. ` + + `For example, if you pass \`"my-popover"\` as the ID, the popover ` + + `title will have the ID \`"my-popover-title"\` and the popover ` + + `content will have the ID \`"my-popover-content"\`.`, + }, + initialFocusId: { + description: + `The selector for the element that will be focused when the ` + + `popover content shows. When not set, the first focusable ` + + `element within the popover content will be used.`, + }, + opened: { + description: + `When true, the popover content is shown.\n\n` + + `Using this prop makes the component behave as a controlled ` + + `component. The parent is responsible for managing the ` + + `opening/closing of the popover when using this prop.`, + control: { + type: "boolean", + }, + }, + onClose: { + description: `Called when the popover content is closed.`, + }, + testId: { + description: `Test ID used for e2e testing.`, + }, + showTail: { + description: `Whether to show the popover tail or not.`, + control: { + type: "boolean", + }, + }, }; diff --git a/__docs__/wonder-blocks-popover/popover.stories.tsx b/__docs__/wonder-blocks-popover/popover.stories.tsx index 4257ea9fd..43b05c594 100644 --- a/__docs__/wonder-blocks-popover/popover.stories.tsx +++ b/__docs__/wonder-blocks-popover/popover.stories.tsx @@ -13,8 +13,31 @@ import {Popover, PopoverContent} from "@khanacademy/wonder-blocks-popover"; import packageConfig from "../../packages/wonder-blocks-popover/package.json"; import ComponentInfo from "../../.storybook/components/component-info"; -import PopoverArgtypes from "./popover.argtypes"; +import PopoverArgtypes, {ContentMappings} from "./popover.argtypes"; +/** + * Popovers provide additional information that is related to a particular + * element and/or content. They can include text, links, icons and + * illustrations. The main difference with `Tooltip` is that they must be + * dismissed by clicking an element. + * + * This component uses the `PopoverPopper` component to position the + * `PopoverContentCore` component according to the children it is wrapping. + * + * ### Usage + * + * ```jsx + * import {Popover, PopoverContent} from "@khanacademy/wonder-blocks-popover"; + * + * {}} + * content={ + * + * }> + * + * + * ``` + */ export default { title: "Popover/Popover", component: Popover as unknown as React.ComponentType, @@ -26,15 +49,6 @@ export default { version={packageConfig.version} /> ), - docs: { - description: { - component: null, - }, - source: { - // See https://github.com/storybookjs/storybook/issues/12596 - excludeDecorators: true, - }, - }, // TODO(WB-1170): Reassess this after investigating more about Chromatic // flakyness. chromatic: { @@ -43,9 +57,7 @@ export default { }, decorators: [ (Story): React.ReactElement> => ( - - - + {Story()} ), ], } as Meta; @@ -78,14 +90,7 @@ type PopoverArgs = Partial; export const Default: StoryComponentType = { args: { children: , - content: ( - - ), - + content: ContentMappings.withTextOnly, placement: "top", dismissEnabled: true, id: "", @@ -338,6 +343,23 @@ WithInitialFocusId.parameters = { }, }; +/** + * Popovers can have custom layouts. This is done by using the + * `PopoverContentCore` component. + * + * _NOTE:_ If you choose to use this component, you'll have to set the + * `aria-labelledby` and `aria-describedby` attributes manually. Make sure to + * pass the `id` prop to the `Popover` component and use it as the value for + * these attributes. Also, make sure to assign the `${id}-title` prop to the + * `title` element and `${id}-content` prop to the `content` element. + */ +export const CustomPopoverContent: StoryComponentType = { + args: { + children: , + content: ContentMappings.coreWithIcon, + id: "custom-popover", + } as PopoverArgs, +}; /** * Alignment example */ diff --git a/packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx b/packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx index f9f979ed8..a2be8682a 100644 --- a/packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx +++ b/packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx @@ -287,4 +287,73 @@ describe("Popover", () => { // Assert expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); }); + + describe("a11y", () => { + it("should announce a popover correctly by reading the title contents", async () => { + // Arrange + render( + + } + > + + , + ); + + // Act + // Open the popover + userEvent.click( + screen.getByRole("button", {name: "Open default popover"}), + ); + + // Assert + expect( + screen.getByRole("dialog", { + name: "The title is read by the screen reader", + }), + ).toBeInTheDocument(); + }); + + it("should announce a custom popover correctly by reading the title contents", async () => { + // Arrange + render( + +

+ This is a custom popover title +

+

+ The custom popover description +

+ + } + > + + , + ); + + // Act + // Open the popover + userEvent.click( + screen.getByRole("button", {name: "Open default popover"}), + ); + + // Assert + expect( + screen.getByRole("dialog", { + name: "This is a custom popover title", + }), + ).toBeInTheDocument(); + }); + }); }); diff --git a/packages/wonder-blocks-popover/src/components/popover-content-core.tsx b/packages/wonder-blocks-popover/src/components/popover-content-core.tsx index effed1cd6..90642be65 100644 --- a/packages/wonder-blocks-popover/src/components/popover-content-core.tsx +++ b/packages/wonder-blocks-popover/src/components/popover-content-core.tsx @@ -132,9 +132,10 @@ const styles = StyleSheet.create({ * elements */ closeButton: { + margin: 0, position: "absolute", - right: Spacing.xSmall_8, - top: Spacing.xSmall_8, + right: Spacing.xxxSmall_4, + top: Spacing.xxxSmall_4, // Allows the button to be above the title and/or custom content zIndex: 1, }, diff --git a/packages/wonder-blocks-popover/src/components/popover-content.tsx b/packages/wonder-blocks-popover/src/components/popover-content.tsx index 98973bbfd..6807c43da 100644 --- a/packages/wonder-blocks-popover/src/components/popover-content.tsx +++ b/packages/wonder-blocks-popover/src/components/popover-content.tsx @@ -47,6 +47,12 @@ type CommonProps = AriaProps & { * Test ID used for e2e testing. */ testId?: string; + /** + * Unique ID for the popover. This is used as a prefix to the IDs of the + * popover's elements. + * @ignore + */ + uniqueId?: string; }; type Props = @@ -209,6 +215,7 @@ export default class PopoverContent extends React.Component { style, title, testId, + uniqueId, } = this.props; return ( @@ -232,10 +239,15 @@ export default class PopoverContent extends React.Component { {this.maybeRenderIcon()} - + {title} - {content} + + {content} + diff --git a/packages/wonder-blocks-popover/src/components/popover-dialog.tsx b/packages/wonder-blocks-popover/src/components/popover-dialog.tsx index d54229f2b..cd3244950 100644 --- a/packages/wonder-blocks-popover/src/components/popover-dialog.tsx +++ b/packages/wonder-blocks-popover/src/components/popover-dialog.tsx @@ -77,6 +77,7 @@ export default class PopoverDialog extends React.Component { style, showTail, "aria-describedby": ariaDescribedby, + "aria-labelledby": ariaLabelledBy, } = this.props; const contentProps = children.props as any; @@ -90,6 +91,7 @@ export default class PopoverDialog extends React.Component { { } }; - renderContent(): PopoverContents { + renderContent(uniqueId: string): PopoverContents { const {content} = this.props; const popoverContents: PopoverContents = @@ -214,7 +210,12 @@ export default class Popover extends React.Component { : content; // @ts-expect-error: TS2769 - No overload matches this call. - return React.cloneElement(popoverContents, {ref: this.contentRef}); + return React.cloneElement(popoverContents, { + ref: this.contentRef, + // internal prop: only injected by Popover + // This allows us to announce the popover content when it is opened. + uniqueId, + }); } renderPopper(uniqueId: string): React.ReactNode { @@ -233,12 +234,13 @@ export default class Popover extends React.Component { {(props: PopperElementProps) => ( this.setState({placement})} showTail={showTail} > - {this.renderContent()} + {this.renderContent(uniqueId)} )}