Fix popover issues with screen readers

Khan · Dec 4, 2023 · d17c268 · d17c268
1 parent 9092363
commit d17c268
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
  */