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

Semantic HTML improvements #343

Open
wants to merge 8 commits into
base: feature/react-18-useid
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 14 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ import 'react-accessible-accordion/dist/fancy-example.css';
We recommend that you copy them into your own app and modify them to suit your
needs, particularly if you're using your own `className`s.

The accordion trigger is built using native button and heading elements which
have default browser styling, these can be overridden in your stylesheet.

## Component API

### Accordion
Expand Down Expand Up @@ -137,9 +140,9 @@ Class(es) to apply to the 'heading' element.

#### aria-level : `number` [*optional*, default: `3`]

Semantics to apply to the 'heading' element. A value of `1` would make your
heading element hierarchically equivalent to an `<h1>` tag, and likewise a value
of `6` would make it equivalent to an `<h6>` tag.
Will determine which 'heading' element is used in the markup. A value of `1`
would make your element an `<h1>` tag, and likewise a value of `6` would make it
an `<h6>` tag.

### AccordionItemButton

Expand Down Expand Up @@ -185,7 +188,8 @@ you, including:

- Applying appropriate aria attributes (`aria-expanded`, `aria-controls`,
`aria-disabled`, `aria-hidden` and `aria-labelledby`).
- Applying appropriate `role` attributes (`button`, `heading`, `region`).
- Applying appropriate `role` attributes (`region`).
- Using semantic HTML elements (`h1` - `h6`, `button`).
- Applying appropriate `tabindex` attributes.
- Applying keyboard interactivity ('space', 'end', 'tab', 'up', 'down', 'home'
and 'end' keys).
Expand All @@ -196,10 +200,10 @@ spec-compliant:
- Only ever use
[phrasing content](https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#Phrasing_content)
inside of your `AccordionItemHeading` component. If in doubt, use text only.
- Always provide an `aria-level` prop to your `AccordionItemHeading`
component, _especially_ if you are nesting accordions. This attribute is a
signal used by assistive technologies (eg. screenreaders) to determine which
heading level (ie. `h1`-`h6`) to treat your heading as.
- Remember to provide an `aria-level` prop to your `AccordionItemHeading`
component, when you are nesting accordions. The levels are used by assistive
technologies (eg. screenreaders) to infer structure, by default each heading
uses `h3` .

If you have any questions about your implementation, then please don't be afraid
to get in touch via our
Expand All @@ -225,8 +229,8 @@ description, as written above. By "accordion-like", we mean components which
have collapsible items but require bespoke interactive mechanisms in order to
expand, collapse and 'disable' them. This includes (but is not limited to)
multi-step forms, like those seen in many cart/checkout flows, which we believe
require (other) complex markup in order to be considered 'accessible'.
This also includes disclosure widgets.
require (other) complex markup in order to be considered 'accessible'. This also
includes disclosure widgets.

### How do I disable an item?

Expand Down
5 changes: 4 additions & 1 deletion demo/src/main.css
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,14 @@ footer {
margin-top: 10px;
}
.code__button {
font-weight: bold;
cursor: pointer;
display: inline-block;
padding: 10px 0;
color: var(--colorPageLinks);
background-color: transparent;
font: inherit;
font-weight: bold;

Choose a reason for hiding this comment

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

AI guess there's a question of how styled this should be. Are our current buttons bold? https://react-accessible-accordion.springload.co.nz/

Copy link
Author

Choose a reason for hiding this comment

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

This is the Show code button css, yep its bold, I just moved the rule down to be with the other text ones

border: none;
text-decoration: underline solid var(--colorGreenShadow);
}
.code__button:hover {
Expand Down
40 changes: 7 additions & 33 deletions integration/wai-aria.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,42 +237,16 @@ describe('WAI ARIA Spec', () => {
});

describe('WAI-ARIA Roles, States, and Properties', () => {
it(`The title of each accordion header is contained in an element with
role button.`, async () => {
// TODO: Use 'title' elements inside the headings.

const { browser, page, buttonsHandles } = await setup();
expect(buttonsHandles).toHaveLength(3);
for (const buttonHandle of buttonsHandles) {
expect(
await page.evaluate(
(button) => button.getAttribute('role'),
buttonHandle,
),
).toBe('button');
}
});

it(`Each accordion header button is wrapped in an element with role
heading that has a value set for aria-level that is appropriate for
the information architecture of the page.`, async () => {
const { browser, page, buttonsHandles } = await setup();
expect(buttonsHandles).toHaveLength(3);
for (const buttonHandle of buttonsHandles) {
expect(
await page.evaluate(
(button) => button.parentElement.getAttribute('role'),
buttonHandle,
),
).toBe('heading');

it(`The title of each accordion header is contained in a heading tag`, async () => {
const { browser, page, headingsHandles } = await setup();
expect(headingsHandles).toHaveLength(3);
for (const headingsHandle of headingsHandles) {
expect(
await page.evaluate(
(button) =>
button.parentElement.getAttribute('aria-level'),
buttonHandle,
(heading) => heading.tagName,
headingsHandle,
),
).toBeTruthy();
).toContain('H3');
sarah-j-lancaster marked this conversation as resolved.
Show resolved Hide resolved
}
});

Expand Down
8 changes: 0 additions & 8 deletions src/components/AccordionContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import * as React from 'react';
import AccordionStore, {
InjectedButtonAttributes,
InjectedHeadingAttributes,
InjectedPanelAttributes,
} from '../helpers/AccordionStore';
import { ID } from './ItemContext';
Expand All @@ -28,7 +27,6 @@ export interface AccordionContext {
uuid: ID,
dangerouslySetExpanded?: boolean,
): InjectedPanelAttributes;
getHeadingAttributes(uuid: ID): InjectedHeadingAttributes;
getButtonAttributes(
uuid: ID,
dangerouslySetExpanded?: boolean,
Expand Down Expand Up @@ -78,11 +76,6 @@ export class Provider extends React.PureComponent<
return this.state.getPanelAttributes(key, dangerouslySetExpanded);
};

getHeadingAttributes = (): InjectedHeadingAttributes => {
// uuid: UUID
return this.state.getHeadingAttributes();
};

getButtonAttributes = (
key: ID,
dangerouslySetExpanded?: boolean,
Expand All @@ -102,7 +95,6 @@ export class Provider extends React.PureComponent<
isItemDisabled: this.isItemDisabled,
isItemExpanded: this.isItemExpanded,
getPanelAttributes: this.getPanelAttributes,
getHeadingAttributes: this.getHeadingAttributes,
getButtonAttributes: this.getButtonAttributes,
}}
>
Expand Down
16 changes: 8 additions & 8 deletions src/components/AccordionItemButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import {
focusPreviousSiblingOf,
} from '../helpers/focus';
import keycodes from '../helpers/keycodes';
import { DivAttributes } from '../helpers/types';
import { assertValidHtmlId } from '../helpers/id';
import { ButtonAttributes } from '../helpers/types';

import { Consumer as ItemConsumer, ItemContext } from './ItemContext';

type Props = DivAttributes & {
type Props = ButtonAttributes & {
toggleExpanded(): void;
};

Expand All @@ -21,7 +21,9 @@ const AccordionItemButton = ({
className = 'accordion__button',
...rest
}: Props) => {
const handleKeyPress = (evt: React.KeyboardEvent<HTMLDivElement>): void => {
const handleKeyPress = (
evt: React.KeyboardEvent<HTMLButtonElement>,
): void => {
const keyCode = evt.key;

if (
Expand Down Expand Up @@ -74,11 +76,9 @@ const AccordionItemButton = ({
}

return (
<div
<button
className={className}
{...rest}
role="button"
tabIndex={0}
onClick={toggleExpanded}
onKeyDown={handleKeyPress}
data-accordion-component="AccordionItemButton"
Expand All @@ -87,8 +87,8 @@ const AccordionItemButton = ({
};

type WrapperProps = Pick<
DivAttributes,
Exclude<keyof DivAttributes, keyof InjectedButtonAttributes>
ButtonAttributes,
Exclude<keyof ButtonAttributes, keyof InjectedButtonAttributes>

Choose a reason for hiding this comment

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

Good to rename all these 👍

>;

const AccordionItemButtonWrapper: React.SFC<WrapperProps> = (
Expand Down
33 changes: 33 additions & 0 deletions src/components/AccordionItemHeading.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,39 @@ describe('AccordionItem', () => {
});
});

describe('aria-level prop', () => {
it('is h3 by default', () => {
const { getByTestId } = render(
<Accordion>
<AccordionItem>
<AccordionItemHeading data-testid={UUIDS.FOO}>
<AccordionItemButton />
</AccordionItemHeading>
</AccordionItem>
</Accordion>,
);

expect(getByTestId(UUIDS.FOO).tagName).toEqual('H3');
});

it('can be overridden', () => {
const { getByTestId } = render(
<Accordion>
<AccordionItem>
<AccordionItemHeading
data-testid={UUIDS.FOO}
aria-level={4}
>
<AccordionItemButton />
</AccordionItemHeading>
</AccordionItem>
</Accordion>,
);

expect(getByTestId(UUIDS.FOO).tagName).toEqual('H4');
sarah-j-lancaster marked this conversation as resolved.
Show resolved Hide resolved
});
});

describe('children prop', () => {
it('is respected', () => {
const { getByText } = render(
Expand Down
80 changes: 38 additions & 42 deletions src/components/AccordionItemHeading.tsx
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
import * as React from 'react';
import { InjectedHeadingAttributes } from '../helpers/AccordionStore';
import DisplayName from '../helpers/DisplayName';
import { DivAttributes } from '../helpers/types';
import { assertValidHtmlId } from '../helpers/id';
import { HeadingAttributes } from '../helpers/types';

import { Consumer as ItemConsumer, ItemContext } from './ItemContext';

type Props = DivAttributes;

const defaultProps = {
className: 'accordion__heading',
'aria-level': 3,
};
interface AccordionItemHeadingProps extends HeadingAttributes {
className?: string;
'aria-level'?: number;
}

export const SPEC_ERROR = `AccordionItemButton may contain only one child element, which must be an instance of AccordionItemButton.

Expand All @@ -21,12 +16,30 @@ From the WAI-ARIA spec (https://www.w3.org/TR/wai-aria-practices-1.1/#accordion)

`;

export class AccordionItemHeading extends React.PureComponent<Props> {
static defaultProps: typeof defaultProps = defaultProps;
const Heading = React.forwardRef<HTMLHeadingElement, AccordionItemHeadingProps>(
(
{
'aria-level': ariaLevel = 3,
className = 'accordion__heading',
...props
}: AccordionItemHeadingProps,
ref,
) => {
const headingTag = `h${ariaLevel}`;
return React.createElement(headingTag, {
className,
...props,
ref,
'data-accordion-component': 'AccordionItemHeading',
});
},
);
Heading.displayName = 'Heading';

ref: HTMLDivElement | undefined;
export class AccordionItemHeading extends React.PureComponent<AccordionItemHeadingProps> {
ref: HTMLHeadingElement | undefined;

static VALIDATE(ref: HTMLDivElement | undefined): void | never {
static VALIDATE(ref: HTMLHeadingElement | undefined): void | never {
if (ref === undefined) {
throw new Error('ref is undefined');
}
Expand All @@ -43,7 +56,7 @@ export class AccordionItemHeading extends React.PureComponent<Props> {
}
}

setRef = (ref: HTMLDivElement): void => {
setRef = (ref: HTMLHeadingElement): void => {
this.ref = ref;
};

Expand All @@ -56,36 +69,19 @@ export class AccordionItemHeading extends React.PureComponent<Props> {
}

render(): JSX.Element {
return (
<div
data-accordion-component="AccordionItemHeading"
{...this.props}
ref={this.setRef}
/>
);
return <Heading ref={this.setRef} {...this.props} />;
}
}

type WrapperProps = Pick<
DivAttributes,
Exclude<keyof DivAttributes, keyof InjectedHeadingAttributes>
>;

const AccordionItemHeadingWrapper: React.SFC<DivAttributes> = (
props: WrapperProps,
): JSX.Element => (
<ItemConsumer>
{(itemContext: ItemContext): JSX.Element => {
const { headingAttributes } = itemContext;

if (props.id) {
assertValidHtmlId(props.id);
}

return <AccordionItemHeading {...props} {...headingAttributes} />;
}}
</ItemConsumer>
);
const AccordionItemHeadingWrapper: React.FC<AccordionItemHeadingProps> = (
props: AccordionItemHeadingProps,
): JSX.Element => {
if (props.id) {
assertValidHtmlId(props.id);
}

return <AccordionItemHeading {...props} />;
};

AccordionItemHeadingWrapper.displayName = DisplayName.AccordionItemHeading;

Expand Down
4 changes: 0 additions & 4 deletions src/components/ItemContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import * as React from 'react';
import {
InjectedButtonAttributes,
InjectedHeadingAttributes,
InjectedPanelAttributes,
} from '../helpers/AccordionStore';
import {
Expand All @@ -30,7 +29,6 @@ export type ItemContext = {
expanded: boolean;
disabled: boolean;
panelAttributes: InjectedPanelAttributes;
headingAttributes: InjectedHeadingAttributes;
buttonAttributes: InjectedButtonAttributes;
toggleExpanded(): void;
};
Expand All @@ -57,7 +55,6 @@ const Provider = ({
uuid,
dangerouslySetExpanded,
);
const headingAttributes = accordionContext.getHeadingAttributes(uuid);
const buttonAttributes = accordionContext.getButtonAttributes(
uuid,
dangerouslySetExpanded,
Expand All @@ -71,7 +68,6 @@ const Provider = ({
disabled,
toggleExpanded: toggleExpanded,
panelAttributes,
headingAttributes,
buttonAttributes,
}}
>
Expand Down