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

Upgrade success Alerts to role="alert" so they're read consistently #573

Closed
wants to merge 1 commit into from
Closed

Upgrade success Alerts to role="alert" so they're read consistently #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions src/app/prototype/alert/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
Test:
When Alert shows on the page,
SR reads the message.
*/

'use client';

import { Alert } from '@/components/uswds/Alert';
import { useState } from 'react';

export default function AlertPage() {
const [alertShown, setAlertShown] = useState(false);

return (
<>
<button onClick={() => setAlertShown(true)}>show alert</button>
<button onClick={() => setAlertShown(false)}>hide alert</button>
{alertShown && <Alert type="error">this is an alert!</Alert>}
</>
);
}
15 changes: 5 additions & 10 deletions src/components/UsersActions/UsersActionsOrgRoles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,6 @@ export function UsersActionsOrgRoles({

return (
<>
{/* aria-live region needs to show up on initial page render. */}
<div
role="region"
aria-live="polite"
aria-atomic={true}
className="usa-sr-only"
>
{actionErrors.join(', ')}
</div>
{actionStatus === 'success' && (
<Alert type="success">Org roles have been saved!</Alert>
)}
Expand Down Expand Up @@ -226,7 +217,11 @@ export function UsersActionsOrgRoles({
</Button>
)}
</div>
{actionStatus === 'pending' && <p>submission in progress...</p>}
{actionStatus === 'pending' && (
<div role="alert">
<p>submission in progress...</p>
</div>
)}
</fieldset>
</form>
</>
Expand Down
6 changes: 5 additions & 1 deletion src/components/UsersActions/UsersActionsSpaceRoles/UsersActionsSpaceRoles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,11 @@ export function UsersActionsSpaceRoles({
</Button>
)}
</div>
{actionStatus === 'pending' && <p>Submission in progress...</p>}
{actionStatus === 'pending' && (
<div role="alert">
<p>submission in progress...</p>
</div>
)}
</form>
);
}
7 changes: 6 additions & 1 deletion src/components/UsersList/UsersList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,12 @@ export function UsersList({
aria-live region needs to show up on initial page render.
More info: https://tetralogical.com/blog/2024/05/01/why-are-my-live-regions-not-working/
*/}
<div role="region" aria-live="assertive" aria-atomic={true}>
<div
role="region"
aria-live="assertive"
aria-atomic={true}
className="usa-sr-only"
>
{successMsg}
</div>
<OverlayDrawer
Expand Down
5 changes: 1 addition & 4 deletions src/components/uswds/Alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,7 @@ export function Alert({
const Heading = headingLevel;

let role = 'region';
if (type === 'success') {
role = 'status';
}
if (type === 'error' || type === 'emergency') {
if (type === 'error' || type === 'emergency' || type === 'success') {
role = 'alert';
Copy link
Contributor

Choose a reason for hiding this comment

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

Eleni! Thank you very much for pulling all this together. (And for all the research in #531!)

I’m curious if this change might potentially be trading one set of usability issues for another? At least, I was under the impression that role="alert"/aria-live="assertive" were to be reserved for urgent messages that need a timely response, as they’ll interrupt any other message currently being read to the user by their AT. I’m worried that could be differently disorienting than what we’ve already got.

Dunno; what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point and I agree that this seems odd. In our use cases, success messages were not being read when they were given a role of status, which to me seemed even more disorienting for AT users. But they were read consistently when upgraded to role of alert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm up for fiddling with the /prototype/alert page more to see if we can still get status alerts to work more consistently. Adding and removing the status alert from the DOM gave no SR announcements. I tried keeping a status alert in the DOM while updating the text inside of it, but the SR would only read it the first time it changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Eleni! I was finally able to access /prototype/alert (#575), and can confirm messages aren’t being read consistently in Safari/VO.

Out of curiosity, would it be possible to have an empty ARIA live region in the document by default, and inject our messages into that container? I wonder if that’d fix the timing issue. (I think we might have discussed this briefly on a call, and specifically that this wasn’t possible; apologies if I’m asking a pre-answered question.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that, but ultimately concluded that it would be a workaround for alerts not acting properly, which they should be. I'd like for future devs to be able to use the Alert component wherever and have it work as expected. I'm happy to talk through it more (maybe at the next a11y jam?).

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great, let’s do it!

}

Expand Down