Skip to content

Create a status component and use it in EUI Docs to announce page loads to screen readers #5991

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

Jump to bottom
Closed
1Copenut wants to merge 5 commits into from
Closed

Create a status component and use it in EUI Docs to announce page loads to screen readers #5991

1Copenut wants to merge 5 commits into from

Conversation

@1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Jun 22, 2022 •
edited
Loading

Summary

Adding an EuiScreenReaderStatus component to announce changes to screen readers without requiring keyboard focus. The component allows focus to be controlled, so we can use it as a consistent announcement for SPA route changes.

Tested for screen reader usability:

  • MacOS Monterey + Safari + VoiceOver
  • MacOS Monterey + Firefox + VoiceOver
  • Reached out to @barlowm for additional screen reader testing

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

1Copenut added 3 commits June 22, 2022 11:31
@1Copenut
Announce route changes to screen readers consistently.
772b22d 
* Added the live region announcement for route changes.
* Adding new component for SR status, tests.
* Adding tests for pageTitle and focus management.
@1Copenut
Refactoring for a true status component.
215dc57
@1Copenut
Adding documentation for status component.
79fe9b5
@1Copenut 1Copenut added documentation Issues or PRs that only affect documentation - will not need changelog entries accessibility labels Jun 22, 2022
@1Copenut 1Copenut self-assigned this Jun 22, 2022
1Copenut
1Copenut commented Jun 22, 2022
View reviewed changes
Copy link
Contributor Author

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

Left a few comments for use cases and testing done to this point.

src-docs/src/components/guide_page/guide_page_chrome.js
<>
{name}
<EuiScreenReaderOnly>
<span> - same page</span>
Copy link
Contributor Author

@1Copenut 1Copenut Jun 22, 2022

Choose a reason for hiding this comment

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

I added this SR-only span to cue that some links in the docs left nav won't load a new route or announce themselves past the "click" sound.

src-docs/src/views/app_view.js

return (
<LinkWrapper>
<EuiScreenReaderStatus
Copy link
Contributor Author

@1Copenut 1Copenut Jun 22, 2022

Choose a reason for hiding this comment

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

Tested in Safari and Firefox with VoiceOver (MacOS Monterey). Request is out for more screen reader testing, but I have 💯 confidence this technique will work.

@kibanamachine
Copy link

kibanamachine commented Jun 22, 2022

Preview documentation changes for this PR: https://eui.elastic.co/pr_5991/

@cee-chen
Copy link
Contributor

cee-chen commented Jun 22, 2022 •
edited
Loading

I'm not seeing the point of the new EuiScreenReaderStatus component over simply modifying/extending our existing EuiScreenReaderLive component. Why not simply modify that component to add either an isFocusable or tabIndex prop, and set tabIndex={-1} on the top-most div?

@1Copenut
Copy link
Contributor Author

1Copenut commented Jun 22, 2022

I considered that, but felt extending the EuiScreenReaderStatus component was getting harder to reason about than I wanted. The new component has one child DIV that is prescriptive with the attributes being set (or not) based on a small API. Each has their use and I feel the cases are different enough to include them both in EUI.

@cee-chen
Copy link
Contributor

cee-chen commented Jun 22, 2022 •
edited
Loading

I don't agree - the double divs in EuiScreenReaderLive were created for a reason, which is to avoid React shenanigans with live regions: #5157 (comment)

the double div approach is one unique to the virtual DOM and also that the intention is not primarily announcement timing but rather de-duplicating announcements

In the case of announcing SPA navigation that is still 100% applicable. If for any reason the consuming application has the same page title on two different pages, we still want the screen reader to read out that they've navigated to the duplicated title, which the current component would not do.

To bypass existing components instead of dogfooding negates research and testing done by multiple libraries. I strongly think we should be using the live region component we already have.

elizabetdev
elizabetdev approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

Tested in Chrome, Safari, Edge, and Firefox. I also read the docs and have one small suggestion.

For the EuiSkipLink what do you think of changing the color to ghost? The skip link will always sit on top of a dark EuiHeader so the ghost button looks better IMO.

skip-link@2x

src-docs/src/views/accessibility/accessibility_example.js
Comment on lines 203 to 210
<p>
<EuiLink
href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role"
external
>
ARIA role status guidelines
</EuiLink>
</p>
Copy link
Contributor

@elizabetdev elizabetdev Jun 23, 2022

Choose a reason for hiding this comment

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

Normally we don't have links in our documentation without any context. So we should tell why consumers should open the link, in this case, to learn more about the status role (I guess):

Suggested change
<p>
<EuiLink
href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role"
external
>
ARIA role status guidelines
</EuiLink>
</p>
<p>
To learn more about the status role make sure to read the{' '}
<EuiLink
href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role"
external
>
ARIA guidelines
</EuiLink>.
</p>

Copy link
Contributor Author

@1Copenut 1Copenut Jun 23, 2022

Choose a reason for hiding this comment

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

Can do! Thank you @miukimiu. I'm also going to work up a second option in a separate PR to address @constancecchen comments above. I'll make the same requested style changes there so we get them regardless of which is merged.

@1Copenut 1Copenut mentioned this pull request Jun 23, 2022
Merged
11 tasks
@kibanamachine
Copy link

kibanamachine commented Jun 23, 2022

Preview documentation changes for this PR: https://eui.elastic.co/pr_5991/

@1Copenut
Copy link
Contributor Author

1Copenut commented Jun 24, 2022

Closing this PR in favor of #5995. We found a better way to re-purpose the EuiScreenReaderLive component there with no loss of clarity or screen reader UX.

@1Copenut 1Copenut closed this Jun 24, 2022
@1Copenut 1Copenut deleted the feature/tpierce-live-region-docs branch June 24, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cee-chen cee-chen Awaiting requested review from cee-chen

@cchaos cchaos Awaiting requested review from cchaos

1 more reviewer

@elizabetdev elizabetdev elizabetdev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@1Copenut 1Copenut

Labels

accessibility documentation Issues or PRs that only affect documentation - will not need changelog entries

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@1Copenut @kibanamachine @cee-chen @elizabetdev