-
Notifications
You must be signed in to change notification settings - Fork 86
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(toast): ensure closed toasts do not trigger axe warnings #6945
Conversation
src/components/toast/toast.pw.tsx
Outdated
|
||
await button(page).nth(0).click(); | ||
|
||
await page.waitForTimeout(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(non-blocking): Rather than having a waitforTimeout
here, you should be able to add toastComponent(page)
or another locator for Toast to the checkAccessibility
function. Something like await checkAccessibility(page, toastComponent(page));
should only run the accessibility checks once the locator is found.
We also have the option of using the waitForAnimationEnd()
function too, which is what the checkAccessibility
function should do.
await waitForAnimationEnd(Toast locator goes here);
.
If that doesn't work, the Toast animation transition is 300ms so you might be able to reduce the 1000
value to 300
.
includeStories: [ | ||
"Default", | ||
"Visual", | ||
"ToastWhenOtherModalRenders", | ||
"ToastWithConditionalContent", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(non-blocking): what do you think of doing the inverse here? This would have the benefit of including any new stories automatically unless they have been explicitly excluded.
includeStories: [ | |
"Default", | |
"Visual", | |
"ToastWhenOtherModalRenders", | |
"ToastWithConditionalContent", | |
], | |
excludeStories: [ | |
"TopAndBottom" | |
], |
src/components/toast/toast.pw.tsx
Outdated
@@ -320,4 +321,21 @@ test.describe("Accessibility tests for Toast component", () => { | |||
|
|||
await checkAccessibility(page); | |||
}); | |||
|
|||
test("should check accessibility more than one Toast is rendered with undefined content on close and defined on open", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: what do you think of this wording tweak? It might help the title flow a bit more smoothly when reading:
test("should check accessibility more than one Toast is rendered with undefined content on close and defined on open", async ({ | |
test("passes accessibility checks when multiple Toasts only have content when opened", async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was struggling on the wording for this test so this is perfect 😅
When a Toast's content is only defined on open, its accessible name cannot be computed and is "empty" while closed. This causes an axe issue when more than one Toast is rendered on a page with undefined content as their accessible name is not "unique". This fix ensures that the region is aria-hidden while closed and aria-labelledby is only set when the Toast is opened.
🎉 This PR is included in version 142.11.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix #6923
Proposed behaviour
Set "aria-hidden" on Toast region while its closed.
Only define the "aria-labelledby" when the Toast is open.
Current behaviour
When a Toast's content is only defined on open, its accessible name cannot be computed and is "empty" while closed. This causes an axe issue when more than one Toast is rendered on a page with undefined content as their accessible name is not "unique".
Checklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions
Sandbox demonstrating issue
Screen reader behaviour should not change with fix.