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

Adds aria labels to immersive toolbar buttons for back and close #9411

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented May 6, 2022

Summary

This uses the existing ariaLabel prop on KIconButton and coreStrings to add labels to the "Back" and "Close" buttons on the immersive toolbar.

References

Fixes #9407

Reviewer guidance

The reviewer should test on a screen reader device or on a device with voiceover or similar settings enabled, anywhere where there is an immersive toolbar (Topics Page, many places is facility settings with users)

I have only tested this on MacOS with voiceover... It does seem to be working as expected in this setting, and the reading I have done would indicate that this is fairly universal, but I'm not sure about other devices.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@marcellamaki marcellamaki added TAG: a11y Affecting accessibility TODO: needs review Waiting for review changelog Important user-facing changes labels May 6, 2022
@radinamatic
Copy link
Member

Tested with NVDA 2021.3.5 in Windows 10 with browsers Firefox and Chrome.

Buttons in the immersive toolbar are announced correctly on pages:

  • Learn > Library > Browse channel
  • Coach > Plan > Groups > Enroll learners
  • Coach > Plan > Lessons > Create new lesson
  • Coach > Plan > Lessons > Create new lesson > Manage lesson resources
  • Coach > Plan > Lessons > Create new quiz
  • Coach > Plan > Lessons > Create new quiz > Select exercises
  • User > Edit profile
  • Facility > Classes > Enroll learners
  • Facility > Classes > Assign coaches
  • Facility > Users > New user
  • Device > Channels > Import from...
  • Device > Channels > Select resources from...
  • Device > Channels > Delete channels
  • Device > Channels > Export channels
  • Device > Channels > Manage '...' channel
  • Device > Permissions > View/Edit

Did I miss some occurrence of the immersive toolbar?

Since the buttons are enclosed in the link element a, NVDA announces Close button visited link and Go back button visited link, but that's probably how our router links are constructed 🤷🏽‍♀️

@nucleogenesis
Copy link
Member

@radinamatic I'd bet that NVDA checks if the HREF on that A tag is in the history and announces "visited link" in that case?

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Changes look good to me - I'll be sure we keep these changes throughout the CoreBase refactor as well!

@rtibbles rtibbles merged commit 0b2c575 into learningequality:release-v0.15.x May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important user-facing changes TAG: a11y Affecting accessibility TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants