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

section-alert: Improve announcements across screen readers #1893

Merged
merged 3 commits into from
Dec 3, 2024
Merged

section-alert: Improve announcements across screen readers #1893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23e19b5
section-alert: Assign role & aria-label to improve announcements acro…
stowball Dec 2, 2024
4b1ed3b
Fix announcements for iOS
stowball Dec 3, 2024
671eced
Simplify example content
stowball Dec 3, 2024
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
7 changes: 7 additions & 0 deletions .changeset/gentle-socks-impress.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@ag.ds-next/react': minor
---

icon: Add `id` as allowed prop.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused now, but it feels like it should still be added


section-alert: Assign `role` and `aria-label` to improve announcements across screen readers.
25 changes: 14 additions & 11 deletions packages/react/src/icon/Icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export type IconProps = {
'aria-label'?: NativeSvgProps['aria-label'];
className?: string;
color?: IconColor;
id?: NativeSvgProps['id'];
size?: ResponsiveProp<IconSize>;
style?: NativeSvgProps['style'];
weight?: IconWeight;
Expand All @@ -55,6 +56,7 @@ export const createIcon = (children: ReactNode, name: string) => {
'aria-label': ariaLabel,
className,
color,
id,
size = 'md',
style,
weight = 'regular',
Expand All @@ -66,14 +68,10 @@ export const createIcon = (children: ReactNode, name: string) => {
);
return (
<svg
// Note the width and height attribute is a fallback for older browsers.
// This may not be required and could potentially be removed.
width="24"
height="24"
viewBox="0 0 24 24"
aria-hidden={ariaHidden}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just alphabetising and adding id

aria-label={ariaLabel}
className={className}
clipRule="evenodd"
fillRule="evenodd"
xmlns="http://www.w3.org/2000/svg"
css={mq({
color: color ? iconColors[color] : 'currentcolor',
fill: 'none',
Expand All @@ -85,12 +83,17 @@ export const createIcon = (children: ReactNode, name: string) => {
strokeWidth: resolvedWeight,
width: resolvedSize,
})}
fillRule="evenodd"
focusable="false"
// Note the width and height attribute is a fallback for older browsers.
// This may not be required and could potentially be removed.
height="24"
id={id}
role="img"
style={style}
className={className}
focusable="false"
aria-hidden={ariaHidden}
aria-label={ariaLabel}
viewBox="0 0 24 24"
width="24"
xmlns="http://www.w3.org/2000/svg"
>
{children}
</svg>
Expand Down
19 changes: 17 additions & 2 deletions packages/react/src/section-alert/SectionAlert.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,18 @@ describe('SectionAlert', () => {
const { container } = renderSectionAlert({ tone });
expect(container).toHTMLValidate({
extends: ['html-validate:recommended'],
rules: {
'prefer-native-element': 'off',
// react 18s `useId` break this rule
'valid-id': 'off',
},
});
});
});
});
}

describe(`with a description`, () => {
describe('with a description', () => {
it('renders correctly', () => {
const { container } = renderSectionAlert({
children: <Text as="p">Section alert description text</Text>,
Expand All @@ -49,11 +54,16 @@ describe('SectionAlert', () => {
});
expect(container).toHTMLValidate({
extends: ['html-validate:recommended'],
rules: {
'prefer-native-element': 'off',
// react 18s `useId` break this rule
'valid-id': 'off',
},
});
});
});

describe(`which is closable`, () => {
describe('which is closable', () => {
const onClose = jest.fn();
const onDismiss = jest.fn();

Expand All @@ -69,6 +79,11 @@ describe('SectionAlert', () => {
});
expect(container).toHTMLValidate({
extends: ['html-validate:recommended'],
rules: {
'prefer-native-element': 'off',
// react 18s `useId` break this rule
'valid-id': 'off',
},
});
});
it('calls the onClose function when clicked', () => {
Expand Down
46 changes: 36 additions & 10 deletions packages/react/src/section-alert/SectionAlert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import {
MouseEventHandler,
ReactNode,
} from 'react';
import { visuallyHiddenStyles } from '../a11y';
import { useId } from '../core';
import { useFocus } from '../core/utils/useFocus';
import { Flex } from '../flex';
import { getOptionalCloseHandler } from '../getCloseHandler';
import { Text } from '../text';
import { SectionAlertDismissButton } from './SectionAlertDismissButton';
import { sectionAlertIconMap, SectionAlertTone } from './utils';
import { sectionAlertIconMap, type SectionAlertTone } from './utils';

type DivProps = HTMLAttributes<HTMLDivElement>;

Expand Down Expand Up @@ -40,9 +42,9 @@ export const SectionAlert = forwardRef<HTMLDivElement, SectionAlertProps>(
function SectionAlert(
{
children,
id,
focusOnMount,
focusOnUpdate,
id,
onClose,
onDismiss,
role,
Expand All @@ -58,34 +60,50 @@ export const SectionAlert = forwardRef<HTMLDivElement, SectionAlertProps>(
focusOnUpdate,
forwardedRef,
});
const { childrenId, titleId, toneId } = useSectionAlertIds(id);

const Icon = sectionAlertIconMap[tone];
const icon = sectionAlertIconMap[tone];
const closeHandler = getOptionalCloseHandler(onClose, onDismiss);

return (
<Flex
{...props}
alignItems="center"
aria-labelledby={`${toneId} ${titleId} ${children ? childrenId : ''}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to use aria-describedby for childrenId, but that made things worse in NVDA (the first one I tested), so stopped trying that

background={tone}
borderColor={tone}
borderLeft
borderLeftWidth="xl"
focusRingFor="all"
gap={0.5}
highContrastOutline
id={id}
focusRingFor="all"
justifyContent="space-between"
padding={1}
ref={ref}
role={role}
// Not using default arg because is if someone accidentally passes a falsey value, we still need the role to be set.
role={role || 'region'}
stowball marked this conversation as resolved.
Show resolved Hide resolved
rounded
tabIndex={tabIndex ?? (focusOnMount || focusOnUpdate ? -1 : undefined)}
{...props}
>
<Flex gap={0.5}>
{Icon}
<Flex gap={0.25} flexDirection={'column'}>
{title && <Text fontWeight="bold">{title}</Text>}
{children}
<span
css={{
display: 'inline-flex',
}}
>
{icon}
<span css={visuallyHiddenStyles} id={toneId}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to use aria-label, but there's no way of using it on a generic element that axe and ARC won't complain at. This means that iOS's virtual cursor is the size of the word and not the icon, but it's better than any alternative

{tone}
</span>
</span>
<Flex flexDirection="column" gap={0.25}>
{title && (
<Text fontWeight="bold" id={titleId}>
{title}
</Text>
)}
{children && <div id={childrenId}>{children}</div>}
</Flex>
</Flex>
{closeHandler ? (
Expand All @@ -95,3 +113,11 @@ export const SectionAlert = forwardRef<HTMLDivElement, SectionAlertProps>(
);
}
);

function useSectionAlertIds(idProp?: string) {
const autoId = useId(idProp);
const childrenId = `section-alert-children-${autoId}`;
const titleId = `section-alert-title-${autoId}`;
const toneId = `section-alert-icon-${autoId}`;
return { childrenId, titleId, toneId };
}
Loading
Loading