Skip to content

Commit

Permalink
refactor!: Removing aria attributes from Banner component (#2318)
Browse files Browse the repository at this point in the history
The useBanner hook is adding the `aria-labelledby` and `aria-describedby` properties to the Banner button element. I removed the call to the hook so the aria attributes are not applied.

The Banner button is using `aria-labelledby` referencing the ActionText, and `aria-describedby` referencing the LabelText sub-component. It is difficult to point to measurable "problems" with this, because it does sound nice with screen readers. When `isSticky` prop is used, the ActionText disappears from the screen, while still being referenced as the main label for the button.

This will break the experience for Dragon Dictation and Voice Control users because the visible LabelText is not being used as the accessible name of the button element.

[category:Components]

### BREAKING CHANGES
We have removed the useBanner hook, the only function of which was to add `aria-labelledby` and `aria-describedby` references to the text inside of the Banner. This was not required for accessibility, and browsers can compute the `name` of the Banner from the text given inside.

Co-authored-by: @mannycarrera4 <[email protected]>
Co-authored-by: manuel.carrera <[email protected]>
  • Loading branch information
3 people authored Sep 12, 2023
1 parent b0ece1d commit 9a26501
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 30 deletions.
14 changes: 0 additions & 14 deletions cypress/integration/Banner.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,6 @@ describe('Banner', () => {
it('should have an element with a role of "button"', () => {
cy.findByRole('button').should('be.visible');
});

it('should have an "aria-labelledby" that matches the action', () => {
cy.findByRole('button').then($button => {
const id = $button.attr('aria-labelledby');
cy.findByText('View All').should('have.attr', 'id', id);
});
});

it('should have an "aria-describedby" that matches the label', () => {
cy.findByRole('button').then($button => {
const id = $button.attr('aria-describedby');
cy.findByText('3 Errors').should('have.attr', 'id', id);
});
});
});
});
});
7 changes: 7 additions & 0 deletions modules/docs/mdx/10.0-UPGRADE_GUIDE.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ any questions.
- [Codemod](#codemod)
- [Removals](#removals)
- [Menu (Preview)](#menu-preview)
- [useBanner](#useBanner)
- [Deprecations](#deprecations)
- [Token Updates](#token-updates)
- [Space and Depth](#space-and-depth)
Expand Down Expand Up @@ -68,6 +69,12 @@ migrate from the `Menu` in Preview to the `Menu` in Main.
This change is **not** handled by the codemod due to the differences in API between the `Menu` in
Preview and the `Menu` in Main.

## useBanner

We have removed the `useBanner` hook, the only function of which was to add `aria-labelledby` and
`aria-describedby` references to the text inside of the Banner. This was not required for accessibility,
and browsers can compute the name of the Banner from the text given inside.

## Deprecations

## Token Updates
Expand Down
3 changes: 1 addition & 2 deletions modules/react/banner/lib/Banner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
} from '@workday/canvas-kit-react/common';
import {Flex} from '@workday/canvas-kit-react/layout';

import {useBanner, useBannerModel, useThemedPalette} from './hooks';
import {useBannerModel, useThemedPalette} from './hooks';

import {BannerIcon} from './BannerIcon';
import {BannerLabel} from './BannerLabel';
Expand Down Expand Up @@ -70,7 +70,6 @@ const styles: CSSProperties = {
export const Banner = createContainer('button')({
displayName: 'Banner',
modelHook: useBannerModel,
elemPropsHook: useBanner,
subComponents: {
/**
* `Banner.Icon` is a styled {@link SystemIcon}. The icon defaults to exclamationTriangleIcon or
Expand Down
1 change: 0 additions & 1 deletion modules/react/banner/lib/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,4 @@ export * from './useThemedPalette';
export * from './useBannerIcon';
export * from './useBannerLabel';
export * from './useBannerActionText';
export * from './useBanner';
export {getPaletteColors};
13 changes: 0 additions & 13 deletions modules/react/banner/lib/hooks/useBanner.tsx

This file was deleted.

7 changes: 7 additions & 0 deletions modules/react/banner/stories/Banner.stories.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {Banner} from '@workday/canvas-kit-react/banner';
import {ActionText} from './examples/ActionText';
import {Basic} from './examples/Basic';
import {Error} from './examples/Error';
import {IconBanner} from './examples/IconBanner';
import {Sticky} from './examples/Sticky';
import {ThemedAlert} from './examples/ThemedAlert';
import {ThemedError} from './examples/ThemedError';
Expand Down Expand Up @@ -49,6 +50,12 @@ banner. This will change the defualt icon to `exclamationCircleIcon`.

<ExampleCodeBlock code={Error} />

### Icon Banner

When only using an icon in the `Banner`, use our `Tooltip` component to both show a visible text alternative, and assign an `aria-label` string to the child `Banner`.

<ExampleCodeBlock code={IconBanner} />

### Sticky

Set the `isSticky` prop of the `Banner` to display it along the right edge of the page. When true,
Expand Down
16 changes: 16 additions & 0 deletions modules/react/banner/stories/examples/IconBanner.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react';

import {Banner} from '@workday/canvas-kit-react/banner';
import {Tooltip} from '@workday/canvas-kit-react/tooltip';
import {styled} from '@workday/canvas-kit-react/common';

export const IconBanner = () => {

return (
<Tooltip title="Warning">
<Banner width="4em">
<Banner.Icon />
</Banner>
</Tooltip>
);
};

0 comments on commit 9a26501

Please sign in to comment.