WB-1636: Announce popover contents correctly to screen readers (#2134)

## 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 <img width="972" alt="popover-sr-before" src="https://github.com/Khan/wonder-blocks/assets/843075/9b57fb30-d773-4b53-94c4-f9b7fad9ad38"> AFTER <img width="897" alt="popover-sr-after" src="https://github.com/Khan/wonder-blocks/assets/843075/cf9ffff8-32b2-487b-8d73-9580310e7e70"> 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: #2134
Khan · Dec 5, 2023 · 66f233d · 66f233d
1 parent 860d9ef
commit 66f233d
Show file tree

Hide file tree

Showing 11 changed files with 250 additions and 73 deletions.
diff --git a/.changeset/dirty-dingos-battle.md 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
@@ -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<React.ComponentProps<typeof View>> => (
@@ -94,14 +85,17 @@ export const WithIcon: StoryComponentType = {
             <>
                 <PhosphorIcon size="large" icon={IconMappings.article} />
                 <View>
-                    <LabelLarge>This is an article</LabelLarge>
-                    <Body>With the content</Body>
+                    <LabelLarge id="custom-popover-title">
+                        This is an article
+                    </LabelLarge>
+                    <Body id="custom-popover-content">With the content</Body>
                 </View>
             </>
         ),
         closeButtonVisible: true,
         style: styles.popoverWithIcon,
     },
+    render: (args) => <PopoverContentCore {...args} />,
 };
 
 // NOTE: Adding a wrapper to cast the component so Storybook doesn't complain.
@@ -116,6 +110,7 @@ export const WithDetailCell: StoryComponentType = {
         children: <ClickableDetailCellWrapper {...ClickableDetailCell.args} />,
         style: styles.popoverWithCell,
     },
+    render: (args) => <PopoverContentCore {...args} />,
 };
 
 WithDetailCell.parameters = {
@@ -179,6 +174,7 @@ export const Dark: StoryComponentType = {
         color: "darkBlue",
         style: styles.customPopover,
     },
+    render: (args) => <PopoverContentCore {...args} />,
 };
 
 Dark.parameters = {

diff --git 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) => <PopoverContent {...args} />,
 };
 
 Default.storyName = "Default (text)";
@@ -89,6 +90,7 @@ export const Emphasized: StoryComponentType = {
             </>
         ),
     },
+    render: (args) => <PopoverContent {...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: <img src="/logo.svg" width="100%" alt="Wonder Blocks logo" />,
+        icon: <img src="./logo.svg" width="100%" alt="Wonder Blocks logo" />,
     },
+    render: (args) => <PopoverContent {...args} />,
 };
 
 WithIcon.parameters = {
@@ -125,14 +128,15 @@ export const WithIllustration: StoryComponentType = {
             "As you can see, this popover includes a full-bleed illustration.",
         image: (
             <img
-                src="/illustration.svg"
+                src="./illustration.svg"
                 alt="An illustration of a person skating on a pencil"
                 width={288}
                 height={200}
             />
         ),
         closeButtonVisible: true,
     },
+    render: (args) => <PopoverContent {...args} />,
 };
 
 WithIllustration.parameters = {

diff --git 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
@@ -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: <DefaultWrapper {...Default.args} />,
-    withEmphasis: <EmphasizedWrapper {...Emphasized.args} />,
-    withIcon: <WithIconWrapper {...WithIcon.args} />,
-    withIllustration: <WithIllustrationWrapper {...WithIllustration.args} />,
-    coreWithIcon: <CoreWithIconWrapper {...CoreWithIcon.args} />,
-    coreWithCell: <CoreWithDetailCellWrapper {...CoreWithDetailCell.args} />,
-    coreDark: <CoreDarkWrapper {...CoreDark.args} />,
+// 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<React.ReactNode>,
         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
@@ -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";
+ *
+ * <Popover
+ *  onClose={() => {}}
+ *  content={
+ *      <PopoverContent title="Title" content="Some content" closeButtonVisible />
+ *  }>
+ *      <Button>Open popover</Button>
+ *  </Popover>
+ * ```
+ */
 export default {
     title: "Popover/Popover",
     component: Popover as unknown as React.ComponentType<any>,
@@ -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<React.ComponentProps<typeof View>> => (
-            <View style={styles.example}>
-                <Story />
-            </View>
+            <View style={styles.example}>{Story()}</View>
         ),
     ],
 } as Meta<typeof Popover>;
@@ -78,14 +90,7 @@ type PopoverArgs = Partial<typeof Popover>;
 export const Default: StoryComponentType = {
     args: {
         children: <Button>Open default popover</Button>,
-        content: (
-            <PopoverContent
-                closeButtonVisible
-                title="Title"
-                content="The popover 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: <Button>Open custom popover</Button>,
+        content: ContentMappings.coreWithIcon,
+        id: "custom-popover",
+    } as PopoverArgs,
+};
 /**
  * Alignment example
  */