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

fix(Notification Drawer): Added screen reader text for notification drawer item read state #9569

Merged
merged 5 commits into from
Nov 2, 2023
Merged

fix(Notification Drawer): Added screen reader text for notification drawer item read state #9569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfe1e5a
fix(Notification Drawer): Added screen reader text for notification d…
tlabaj Sep 6, 2023
ccc2cea
fix snapshot
tlabaj Sep 6, 2023
394cd16
Update prop description
tlabaj Sep 27, 2023
4c65cfb
add aria-live
tlabaj Oct 18, 2023
b761913
update snapshots
tlabaj Oct 23, 2023
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
4 changes: 3 additions & 1 deletion packages/react-core/src/components/NotificationDrawer/NotificationDrawerHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ export const NotificationDrawerHeader: React.FunctionComponent<NotificationDrawe
{title}
</Text>
{(customText !== undefined || count !== undefined) && (
<span className={css(styles.notificationDrawerHeaderStatus)}>{customText || `${count} ${unreadText}`}</span>
<span className={css(styles.notificationDrawerHeaderStatus)} aria-live="polite">
{customText || `${count} ${unreadText}`}
</span>
)}
{(children || onClose) && (
<div className={css(styles.notificationDrawerHeaderAction)}>
Expand Down
12 changes: 12 additions & 0 deletions packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export interface NotificationDrawerListItemProps extends React.HTMLProps<HTMLLIE
isRead?: boolean;
/** Callback for when a list item is clicked */
onClick?: (event: any) => void;
/** Visually hidden text that conveys the current read state of the notification list item */
readStateScreenReaderText?: string;
/** Tab index for the list item */
tabIndex?: number;
/** Variant indicates the severity level */
Expand All @@ -26,6 +28,7 @@ export const NotificationDrawerListItem: React.FunctionComponent<NotificationDra
isRead = false,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
onClick = (event: React.MouseEvent) => undefined as any,
readStateScreenReaderText,
tabIndex = 0,
variant = 'custom',
...props
Expand All @@ -38,6 +41,14 @@ export const NotificationDrawerListItem: React.FunctionComponent<NotificationDra
}
}
};

let readStateSRText;
if (readStateScreenReaderText) {
readStateSRText = readStateScreenReaderText;
} else {
readStateSRText = isRead ? 'read' : 'unread';
}

return (
<li
{...props}
Expand All @@ -52,6 +63,7 @@ export const NotificationDrawerListItem: React.FunctionComponent<NotificationDra
onClick={(e) => onClick(e)}
onKeyDown={onKeyDown}
>
<span className="pf-v5-screen-reader">{readStateSRText}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll most likely need an aria-live="polite" added somewhere, as right now there's no indication that anything has changed when marking a notification as read. Personally I lean a bit more towards adding it to the pf-v5-c-notification-drawer__header-status element (line ~45 of NotificationDrawerHeader), so that the current count is announced when updated. Doing that would:

  • provide users the context of the read status of the current notification item when traversing through the list from this readStateSRText
  • alert users that the notification they are on is marked as read by the announcement of "[number] unread" from the notification drawer header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jessiehuff I would be interested in your opinion here as well. I know we talked about a couple of different options.

Copy link
Contributor

Choose a reason for hiding this comment

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

When testing on VO, I noticed that when I "read" a notification, the screen reader does currently say that specific notification is read. (It looks like from your comment that that was true for you as well. Though it might be wise to check this in other screen readers like JAWS.) If I'm understanding correctly, we're thinking of making the "2 unread" notifications the live region and then updating that text every time a user reads a notification? I suppose that as long as the live region is set to polite, that wouldn't be too bad because then it ideally should let the user get through the notification they're on before informing them of what they have left. I think that could make sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

could remove 44-52 and do this

Suggested change
<span className="pf-v5-screen-reader">{readStateSRText}</span>
<span className="pf-v5-screen-reader">{readStateScreenReaderText || (isRead ? 'read' : 'unread')}</span>

{children}
</li>
);
Expand Down
15 changes: 14 additions & 1 deletion ...eact-core/src/components/NotificationDrawer/__tests__/NotificationDrawerListItem.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,21 @@ describe('NotificationDrawerListItem', () => {
expect(screen.getByRole('listitem')).toHaveClass('pf-m-hoverable');
});

test('drawer list item with isRead applied', () => {
test('drawer list item with isRead applied and screen reader text is set to read', () => {
render(<NotificationDrawerListItem isRead />);
expect(screen.getByRole('listitem')).toHaveClass('pf-m-read');
expect(screen.getByRole('listitem')).toContainHTML('<span class="pf-v5-screen-reader">read</span>');

});

test('drawer list item has screen reader text set to unread by default', () => {
render(<NotificationDrawerListItem />);
expect(screen.getByRole('listitem')).toContainHTML('<span class="pf-v5-screen-reader">unread</span>');
});

test('drawer list item screen reader textcan be customized', () => {
render(<NotificationDrawerListItem readStateScreenReaderText="was read"/>);
expect(screen.getByRole('listitem')).toContainHTML('<span class="pf-v5-screen-reader">was read</span>');
});
});

2 changes: 2 additions & 0 deletions ...ponents/NotificationDrawer/__tests__/__snapshots__/NotificationDrawerHeader.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ exports[`NotificationDrawerHeader drawer header with count applied 1`] = `
Notifications
</h1>
<span
aria-live="polite"
class="pf-v5-c-notification-drawer__header-status"
>
2 unread
Expand All @@ -38,6 +39,7 @@ exports[`NotificationDrawerHeader drawer header with custom unread text applied
Notifications
</h1>
<span
aria-live="polite"
class="pf-v5-c-notification-drawer__header-status"
>
2 unread alerts
Expand Down
8 changes: 7 additions & 1 deletion ...nents/NotificationDrawer/__tests__/__snapshots__/NotificationDrawerListItem.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ exports[`NotificationDrawerListItem renders with PatternFly Core styles 1`] = `
<li
class="pf-v5-c-notification-drawer__list-item pf-m-hoverable pf-m-custom"
tabindex="0"
/>
>
<span
class="pf-v5-screen-reader"
>
unread
</span>
</li>
</DocumentFragment>
`;
Loading