Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Dec 5, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dirty-dingos-battle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
18 changes: 7 additions & 11 deletions __docs__/wonder-blocks-popover/popover-content-core.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>> => (
Expand Down Expand Up @@ -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} />,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: These render wrappers are included to solve a known SB issue with reusable stories + CSF 3 (I added a comment in another part to point to this ticket): See storybookjs/storybook#15954 (comment)

};

// NOTE: Adding a wrapper to cast the component so Storybook doesn't complain.
Expand All @@ -116,6 +110,7 @@ export const WithDetailCell: StoryComponentType = {
children: <ClickableDetailCellWrapper {...ClickableDetailCell.args} />,
style: styles.popoverWithCell,
},
render: (args) => <PopoverContentCore {...args} />,
};

WithDetailCell.parameters = {
Expand Down Expand Up @@ -179,6 +174,7 @@ export const Dark: StoryComponentType = {
color: "darkBlue",
style: styles.customPopover,
},
render: (args) => <PopoverContentCore {...args} />,
};

Dark.parameters = {
Expand Down
8 changes: 6 additions & 2 deletions __docs__/wonder-blocks-popover/popover-content.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)";
Expand Down Expand Up @@ -89,6 +90,7 @@ export const Emphasized: StoryComponentType = {
</>
),
},
render: (args) => <PopoverContent {...args} />,
};

Emphasized.parameters = {
Expand All @@ -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" />,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Adding relative paths to fix some issues in the production version of SB (these images are broken in there).

},
render: (args) => <PopoverContent {...args} />,
};

WithIcon.parameters = {
Expand All @@ -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 = {
Expand Down
7 changes: 5 additions & 2 deletions __docs__/wonder-blocks-popover/popover.accessibility.stories.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
95 changes: 78 additions & 17 deletions __docs__/wonder-blocks-popover/popover.argtypes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Added descriptions to improve the Popover documentation in SB.

`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",
},
},
};
64 changes: 43 additions & 21 deletions __docs__/wonder-blocks-popover/popover.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand All @@ -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: {
Expand All @@ -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>;
Expand Down Expand Up @@ -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: "",
Expand Down Expand Up @@ -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 = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I noticed that we didn't include a story with custom popovers, so I added it here. This also helps to manually test custom popovers with screen readers.

args: {
children: <Button>Open custom popover</Button>,
content: ContentMappings.coreWithIcon,
id: "custom-popover",
} as PopoverArgs,
};
/**
* Alignment example
*/
Expand Down
Loading
Loading