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.

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: Add describedBy prop to announce the popover dialog contents to screen readers. #2110

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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/short-eggs-deliver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-popover": minor
---

Add `describedBy` prop to announce the popover dialog contents to screen readers.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ The Popover component should follow these guidelines:
The popover component will populate the
[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.
instance. Internally, it will be set on one of the contents of the dialog
element. This will depend on which `describedBy` value is set. For more info,
check the [describedBy](../?path=/docs/popover-popover--docs#described-by)
example.

## Keyboard Interaction

Expand Down
74 changes: 71 additions & 3 deletions __docs__/wonder-blocks-popover/popover.argtypes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ const CoreWithIconWrapper = CoreWithIcon as React.ElementType;
const CoreWithDetailCellWrapper = CoreWithDetailCell as React.ElementType;
const CoreDarkWrapper = CoreDark as React.ElementType;

export const ContentMappings: {
[key: string]: React.ReactNode;
} = {
export const ContentMappings = {
withTextOnly: <DefaultWrapper {...Default.args} />,
withEmphasis: <EmphasizedWrapper {...Emphasized.args} />,
withIcon: <WithIconWrapper {...WithIcon.args} />,
Expand All @@ -36,20 +34,90 @@ export const ContentMappings: {

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,
},
describedBy: {
control: {
type: "select",
},
description:
"The type of content labelling this popover, if applicable.",
defaultValue: "title",
table: {
type: {summary: `"title" | "content" | "all-content"`},
defaultValue: {summary: `"title"`},
},
type: {
name: "enum",
value: ["title", "content", "all-content"],
required: false,
},
},
dismissEnabled: {
description:
`When enabled, user can hide the popover content by pressing the ` +
`\`esc\` key or clicking/tapping outside of it.`,
},
id: {
description: `The unique identifier to give to the 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",
},
},
};
75 changes: 65 additions & 10 deletions __docs__/wonder-blocks-popover/popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,29 @@ import packageConfig from "../../packages/wonder-blocks-popover/package.json";
import ComponentInfo from "../../.storybook/components/component-info";
import PopoverArgtypes 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 Down Expand Up @@ -85,7 +99,6 @@ export const Default: StoryComponentType = {
content="The popover content."
/>
),

placement: "top",
dismissEnabled: true,
id: "",
Expand Down Expand Up @@ -338,6 +351,48 @@ WithInitialFocusId.parameters = {
},
};

/**
* In order to make the popover accessible, we need to make sure that:
* - The popover is focusable.
* - The popover is keyboard accessible.
* - The popover is announced to screen readers.
*
* This example shows how to make the popover accessible by using the
* `describedBy` prop.
* - When `describedBy` is set to `title`, the popover will be announced
* as "Nice work!".
* - When `describedBy` is set to `content`, the popover will be announced
* as "You've completed this step. Now onto the next!".
* - When `describedBy` is set to `all-content`, the popover will be
* announced as "Nice work! You've completed this step. Now onto the next!".
*/
export const DescribedBy: StoryComponentType = {
args: {
describedBy: "all-content",
} as PopoverArgs,
render: (args) => (
<Popover
content={
<PopoverContent
title="Nice work!"
content="You've completed this step. Now onto the next!"
closeButtonVisible
icon={
<img
src="./logo.svg"
width="100%"
alt="Wonder Blocks logo"
/>
}
/>
}
{...args}
>
<Button>Open popover</Button>
</Popover>
),
};

/**
* Alignment example
*/
Expand Down
22 changes: 19 additions & 3 deletions packages/wonder-blocks-popover/src/components/popover-content.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ 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 =
Expand Down Expand Up @@ -209,6 +216,7 @@ export default class PopoverContent extends React.Component<Props> {
style,
title,
testId,
uniqueId,
} = this.props;

return (
Expand All @@ -231,11 +239,19 @@ export default class PopoverContent extends React.Component<Props> {

{this.maybeRenderIcon()}

<View style={styles.text}>
<HeadingSmall style={styles.title}>
<View
id={`${uniqueId}-all-content`}
style={styles.text}
>
<HeadingSmall
id={`${uniqueId}-title`}
style={styles.title}
>
{title}
</HeadingSmall>
<Body>{content}</Body>
<Body id={`${uniqueId}-content`}>
{content}
</Body>
</View>
</View>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export default class PopoverDialog extends React.Component<Props> {
style,
showTail,
"aria-describedby": ariaDescribedby,
// "aria-labelledby": ariaLabelledBy,
} = this.props;

const contentProps = children.props as any;
Expand Down
30 changes: 25 additions & 5 deletions packages/wonder-blocks-popover/src/components/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ type Props = AriaProps &
* or clicking/tapping outside of it.
*/
dismissEnabled?: boolean;

/**
* The type of content labelling this popover, if applicable.
* - title: References to the title of the popover.
* - content: References to the content of the popover.
* - all-content: References to both the title and content of the
* popover.
*
* This is used to allow screen readers to announce the popover content
* when it is opened.
* Defaults to "title".
*/
describedBy: "title" | "content" | "all-content";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describedBy: "title" | "content" | "all-content";
describedBy: "title" | "content" | "title-and-content";

I think this is slightly clearer

/**
* The unique identifier to give to the popover. Provide this in cases where
* you want to override the default accessibility solution. This identifier
Expand Down Expand Up @@ -117,6 +130,7 @@ type State = Readonly<{
}>;

type DefaultProps = Readonly<{
describedBy: Props["describedBy"];
placement: Props["placement"];
showTail: Props["showTail"];
}>;
Expand Down Expand Up @@ -146,6 +160,7 @@ type DefaultProps = Readonly<{
*/
export default class Popover extends React.Component<Props, State> {
static defaultProps: DefaultProps = {
describedBy: "title",
placement: "top",
showTail: true,
};
Expand Down Expand Up @@ -203,7 +218,7 @@ export default class Popover extends React.Component<Props, State> {
}
};

renderContent(): PopoverContents {
renderContent(uniqueId: string): PopoverContents {
const {content} = this.props;

const popoverContents: PopoverContents =
Expand All @@ -214,11 +229,16 @@ export default class Popover extends React.Component<Props, State> {
: 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 props only injected by Popover
uniqueId,
describedBy: this.props.describedBy,
});
}

renderPopper(uniqueId: string): React.ReactNode {
const {initialFocusId, placement, showTail} = this.props;
const {describedBy, initialFocusId, placement, showTail} = this.props;
const {anchorElement} = this.state;

return (
Expand All @@ -233,12 +253,12 @@ export default class Popover extends React.Component<Props, State> {
{(props: PopperElementProps) => (
<PopoverDialog
{...props}
aria-describedby={`${uniqueId}-anchor`}
aria-describedby={`${uniqueId}-${describedBy}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

So it looks like this existing aria-describedby prop makes it so the popover is described by the content that it is pointing at. I think it would be best not to totally remove that, as we want some way to tie the popover to the content it points at. Adobe's react-aria-components library uses aria-labelledby for this, so maybe a solution would be:

Suggested change
aria-describedby={`${uniqueId}-${describedBy}`}
aria-labelledby={`${uniqueId}-anchor`}
aria-describedby={`${uniqueId}-${describedBy}`}

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case pointing labelledby to the anchor element would not work as we expect. If we make that, then the dialog would be announced with the contents of the button/anchor element and the intention would be to use that to announce the dialog title instead.

From: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role#associated_aria_roles_states_and_properties

aria-labelledby
Use this attribute to label the dialog. Often, the value of the aria-labelledby attribute will be the id of the element used to title the dialog.

That makes me think that we can even use that instead of the describedBy solution. Let me try that alternative.

id={uniqueId}
onUpdate={(placement) => this.setState({placement})}
showTail={showTail}
>
{this.renderContent()}
{this.renderContent(uniqueId)}
</PopoverDialog>
)}
</TooltipPopper>
Expand Down
Loading