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

fix(ui-breadcrumb): add and update aria tags in Breadcrumb and documentation #1788

Merged
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 10 additions & 2 deletions packages/ui-breadcrumb/src/Breadcrumb/BreadcrumbLink/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,15 @@ class BreadcrumbLink extends Component<BreadcrumbLinkProps> {
}

render() {
const { children, href, renderIcon, iconPlacement, onClick, onMouseEnter } =
this.props
const {
children,
href,
renderIcon,
iconPlacement,
onClick,
onMouseEnter,
isCurrentPage
} = this.props

const props = omitProps(this.props, BreadcrumbLink.allowedProps)

Expand All @@ -69,6 +76,7 @@ class BreadcrumbLink extends Component<BreadcrumbLinkProps> {
onMouseEnter={onMouseEnter}
isWithinText={false}
elementRef={this.handleRef}
{...(isCurrentPage && { 'aria-current': 'page' })}
>
<TruncateText>{children}</TruncateText>
</Link>
Expand Down
11 changes: 9 additions & 2 deletions packages/ui-breadcrumb/src/Breadcrumb/BreadcrumbLink/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ type BreadcrumbLinkOwnProps = {
* Place the icon before or after the text in the Breadcrumb.Link
*/
iconPlacement?: 'start' | 'end'
/**
* Whether the page this breadcrumb points to is the current one. If true, it sets aria-current="page".
* If this prop is not set to true on any breadcrumb element, the one recieving the aria-current="page" will always be the last element, unless the last element's isCurrentPage prop is explicity set to false.
*/
isCurrentPage?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please make an a11y page to the docs, like in https://instructure.design/#Alert where you write how and when this prop should be used

Copy link
Collaborator

Choose a reason for hiding this comment

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

"on the right by default" this is not needed, for RTL languages its different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the a11y box and removed the "on the right by default" part

}

type PropKeys = keyof BreadcrumbLinkOwnProps
Expand All @@ -89,7 +94,8 @@ const propTypes: PropValidators<PropKeys> = {
onMouseEnter: PropTypes.func,
size: PropTypes.oneOf(['small', 'medium', 'large']),
renderIcon: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),
iconPlacement: PropTypes.oneOf(['start', 'end'])
iconPlacement: PropTypes.oneOf(['start', 'end']),
isCurrentPage: PropTypes.bool
}

const allowedProps: AllowedPropKeys = [
Expand All @@ -99,7 +105,8 @@ const allowedProps: AllowedPropKeys = [
'onClick',
'onMouseEnter',
'renderIcon',
'size'
'size',
'isCurrentPage'
]

export type { BreadcrumbLinkProps }
Expand Down
21 changes: 17 additions & 4 deletions packages/ui-breadcrumb/src/Breadcrumb/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type: example
{(props, matches) => {
if (matches.includes('tablet')) {
return (
<Breadcrumb label="You are here:">
<Breadcrumb label="breadcrumb">
<Breadcrumb.Link href="#">Student Forecast</Breadcrumb.Link>
<Breadcrumb.Link href="#">University of Utah</Breadcrumb.Link>
<Breadcrumb.Link href="#">University of Utah Colleges</Breadcrumb.Link>
Expand Down Expand Up @@ -52,7 +52,7 @@ Change the `size` prop to control the font-size of the breadcrumbs (default is `
type: example
---
<div>
<Breadcrumb size="small" label="You are here:" margin="none none medium">
<Breadcrumb size="small" label="breadcrumb" margin="none none medium">
<Breadcrumb.Link href="https://instructure.github.io/instructure-ui/">English 204</Breadcrumb.Link>
<Breadcrumb.Link
onClick={function () {
Expand All @@ -65,7 +65,7 @@ type: example
<Breadcrumb.Link>Rabbit Is Rich</Breadcrumb.Link>
</Breadcrumb>
<View as="div" width="40rem">
<Breadcrumb label="You are here:" margin="none none medium">
<Breadcrumb label="breadcrumb" margin="none none medium">
<Breadcrumb.Link href="https://instructure.github.io/instructure-ui/">English 204</Breadcrumb.Link>
<Breadcrumb.Link
onClick={function () {
Expand All @@ -78,7 +78,7 @@ type: example
<Breadcrumb.Link>Rabbit Is Rich</Breadcrumb.Link>
</Breadcrumb>
</View>
<Breadcrumb size="large" label="You are here:">
<Breadcrumb size="large" label="breadcrumb">
<Breadcrumb.Link href="https://instructure.github.io/instructure-ui/">English 204</Breadcrumb.Link>
<Breadcrumb.Link
onClick={function () {
Expand Down Expand Up @@ -126,3 +126,16 @@ type: embed
</Figure>
</Guidelines>
```

```js
---
type: embed
---
<Guidelines>
<Figure recommendation="a11y" title="Accessibility">
<Figure.Item>
To indicate the current element within a breadcrumb, the <a href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-current">aria-current</a> attribute is used. In this component, aria-current="page" will automatically be applied to the last element, and we recommend that the current page always be the last element in the breadcrumb. If the last element is not the current page, the isCurrentPage property should be applied to the relevant Breadcrumb.Link to ensure compatibility with screen readers.
</Figure.Item>
</Figure>
</Guidelines>
```
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,68 @@ describe('<Breadcrumb />', () => {
expect(icon).toBeInTheDocument()
expect(icon).toHaveAttribute('aria-hidden', 'true')
})

it('should add aria-current="page" to the last element by default', () => {
const { container } = render(
<Breadcrumb label={TEST_LABEL}>
<Breadcrumb.Link href={TEST_LINK}>{TEST_TEXT_01}</Breadcrumb.Link>
<Breadcrumb.Link>{TEST_TEXT_02}</Breadcrumb.Link>
</Breadcrumb>
)
const links = container.querySelectorAll('[class$="--block-link"]')
const firstLink = links[0]
const lastLink = links[links.length - 1]

expect(firstLink).not.toHaveAttribute('aria-current', 'page')
expect(lastLink).toHaveAttribute('aria-current', 'page')
})

it('should add aria-current="page" to the element if isCurrent is true', () => {
const { container } = render(
<Breadcrumb label={TEST_LABEL}>
<Breadcrumb.Link isCurrentPage href={TEST_LINK}>
{TEST_TEXT_01}
</Breadcrumb.Link>
<Breadcrumb.Link>{TEST_TEXT_02}</Breadcrumb.Link>
</Breadcrumb>
)
const links = container.querySelectorAll('[class$="--block-link"]')
const firstLink = links[0]
const lastLink = links[links.length - 1]

expect(firstLink).toHaveAttribute('aria-current', 'page')
expect(lastLink).not.toHaveAttribute('aria-current', 'page')
})

it('should throw a warning when multiple elements have isCurrent set to true ', () => {
render(
<Breadcrumb label={TEST_LABEL}>
<Breadcrumb.Link isCurrentPage href={TEST_LINK}>
{TEST_TEXT_01}
</Breadcrumb.Link>
<Breadcrumb.Link isCurrentPage>{TEST_TEXT_02}</Breadcrumb.Link>
</Breadcrumb>
)

expect(consoleWarningMock).toHaveBeenCalledWith(
expect.stringContaining(
'Warning: Multiple elements with isCurrentPage=true found. Only one element should be set to current.'
)
)
})

it('should not add aria-current="page" to the last element if it set to false', () => {
const { container } = render(
<Breadcrumb label={TEST_LABEL}>
<Breadcrumb.Link href={TEST_LINK}>{TEST_TEXT_01}</Breadcrumb.Link>
<Breadcrumb.Link isCurrentPage={false}>{TEST_TEXT_02}</Breadcrumb.Link>
</Breadcrumb>
)
const links = container.querySelectorAll('[class$="--block-link"]')
const firstLink = links[0]
const lastLink = links[links.length - 1]

expect(firstLink).not.toHaveAttribute('aria-current', 'page')
expect(lastLink).not.toHaveAttribute('aria-current', 'page')
})
})
30 changes: 29 additions & 1 deletion packages/ui-breadcrumb/src/Breadcrumb/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ class Breadcrumb extends Component<BreadcrumbProps> {
this.ref = el
}

addAriaCurrent = (child: React.ReactNode) => {
const updatedChild = React.cloneElement(
child as React.ReactElement<{ 'aria-current'?: string }>,
{
'aria-current': 'page'
}
)
return updatedChild
}

componentDidMount() {
this.props.makeStyles?.()
}
Expand All @@ -77,10 +87,28 @@ class Breadcrumb extends Component<BreadcrumbProps> {
const inlineStyle = {
maxWidth: `${Math.floor(100 / numChildren)}%`
}
let isAriaCurrentSet = false

return React.Children.map(children, (child, index) => {
const isLastElement = index === numChildren - 1
if (React.isValidElement(child)) {
const isCurrentPage = child.props.isCurrentPage || false
if (isAriaCurrentSet && isCurrentPage) {
console.warn(
`Warning: Multiple elements with isCurrentPage=true found. Only one element should be set to current.`
)
}
if (isCurrentPage) {
isAriaCurrentSet = true
}
}
return (
<li css={styles?.crumb} style={inlineStyle}>
{child}
{!isAriaCurrentSet &&
isLastElement &&
(child as React.ReactElement).props.isCurrentPage !== false
? this.addAriaCurrent(child)
: child}
{index < numChildren - 1 && (
<IconArrowOpenEndSolid color="auto" css={styles?.separator} />
)}
Expand Down
Loading