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.

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

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Conversation

nuria1110
Copy link
Contributor

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".

image

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

Sandbox demonstrating issue

  • Run axe scan with both Toasts closed.
  • See issue shown in screenshot.

Screen reader behaviour should not change with fix.


await button(page).nth(0).click();

await page.waitForTimeout(1000);
Copy link
Contributor

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.

Comment on lines 11 to 16
includeStories: [
"Default",
"Visual",
"ToastWhenOtherModalRenders",
"ToastWithConditionalContent",
],
Copy link
Contributor

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.

Suggested change
includeStories: [
"Default",
"Visual",
"ToastWhenOtherModalRenders",
"ToastWithConditionalContent",
],
excludeStories: [
"TopAndBottom"
],

@@ -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 ({
Copy link
Contributor

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:

Suggested change
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 ({

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 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.
@Parsium Parsium marked this pull request as ready for review September 12, 2024 14:17
@Parsium Parsium requested review from a team as code owners September 12, 2024 14:17
@nuria1110 nuria1110 merged commit c694161 into master Sep 20, 2024
24 checks passed
@nuria1110 nuria1110 deleted the FE-6791 branch September 20, 2024 07:44
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 142.11.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Toast - cannot compute accessible name during entrance animation sequence
5 participants