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

Jump to bottom

Update EuiScreenReaderLive and use it in EUI Docs to announce page loads to screen readers #5995

Merged
merged 18 commits into from
Jun 28, 2022
Merged

Update EuiScreenReaderLive and use it in EUI Docs to announce page loads to screen readers #5995

merged 18 commits into from
Jun 28, 2022

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Jun 23, 2022 •
edited by cee-chen
Loading

Summary

Updated the EuiScreenReaderLive component to to accept an isFocusable boolean prop. The change lets users set focus on the outer containing DIV, so we can use it as a consistent announcement for SPA route changes.

This is a second PR that explores a different implementation of the same EUI Docs change as #5991. The second PR was prompted by @constancecchen comment here: #5991 (comment)

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
Adding focus control to EuiScreenReaderLive
de9e5eb 
* Updating screen_reader_live to accept focus.
* Added two tests for isFocusable behavior.
* Adding documentation for isFocusable prop.
@1Copenut
Updating skip link tests.
458ddfa
@1Copenut 1Copenut added documentation Issues or PRs that only affect documentation - will not need changelog entries accessibility accessibility - screen reader labels Jun 23, 2022
@1Copenut 1Copenut self-assigned this Jun 23, 2022
@1Copenut 1Copenut marked this pull request as ready for review June 23, 2022 20:20
1Copenut
1Copenut commented Jun 23, 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 one comment about adding same page link EuiScreenReaderOnly text

src-docs/src/components/guide_page/guide_page_chrome.js Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@1Copenut 1Copenut requested a review from cee-chen June 24, 2022 16:26
@1Copenut
Copy link
Contributor Author

1Copenut commented Jun 24, 2022 •
edited
Loading

@constancecchen && @thompsongl I have a philosophical question RE this PR and the alternate take I proposed in #5991.

This PR (5995) uses Greg's EuiScreenReaderLive component and keys on URL strings to update the children prop to toggle local state. In my opinion this is the ideal for a design system component, to keep that toggle in the component's local state. What it means for our docs site (and I'm guessing Kibana), is that the toggle will only fire once on route change, and will always mean the first child div[role="status"] will be populated with the desired message.

PR 5995 only fires setToggle() once on route change, but fires it every time we click an in-page route. It doesn't announce the same page title a second time because focusRegionOnTextChange is not updated; we're not setting focus to the status region at the top of the source order.

I think this is okay, but wanted to get your thoughts. The alternate PR 5991 doesn't have this side effect because it uses a single status div and doesn't "juggle" two status live regions. 5991 depends on focus being set on a single div[role="status"] with text inside it to announce to screen readers. It's a subtle difference, but one I noticed while updating my SPA-routing-experience-body component to match this ^^ PR.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@1Copenut 1Copenut mentioned this pull request Jun 24, 2022
Closed
11 tasks
@1Copenut 1Copenut requested a review from cee-chen June 24, 2022 20:01
@cee-chen
misc tweaks/fixes to previous constance commits
0df382f 
- actually revert guide_page_chrome to original code

- fix scroll_to_hash comments, syntax
@cee-chen
[PR feedback] EuiSkipLink color prop
f9b8e56 
- DRY out ButtonColor typing
- remove unnecessary `= 'primary'` fallback - EuiButton already has this by default, so passing undefined is fine
cee-chen
cee-chen reviewed Jun 24, 2022
View reviewed changes
src/components/accessibility/screen_reader_live/screen_reader_live.test.tsx

const focusableDiv = component.find('div').at(0);

expect(focusableDiv.is(':focus')).toBe(true);
Copy link
Member

Choose a reason for hiding this comment

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

Awesome unit tests!

cee-chen and others added 2 commits June 24, 2022 13:22
@cee-chen
[PR feedback] Revert EuiSkipLink changes
8306354 
- the component already extends `EuiButtonProps` and already accepts `color` with all the necessary typing, which I only just realized 🤦
@1Copenut
Update src-docs/src/views/accessibility/accessibility_example.js
52cc406 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@kibanamachine
Copy link

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

cee-chen
cee-chen approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 This feels really robust and well tested - thanks for taking the extra time and effort on this Trevor!

@kibanamachine
Copy link

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

thompsongl
thompsongl approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM! Also confirmed that SR results for searchable EuiSelectable are unchanged.

miukimiu
miukimiu approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks, @1Copenut.

The color="ghost" in EuiSkipLink looks good to me! 🎉

@1Copenut 1Copenut merged commit 6e72317 into elastic:main Jun 28, 2022
@1Copenut 1Copenut deleted the feature/tpierce-live-region-docs-ii branch June 28, 2022 14:44
@1Copenut 1Copenut mentioned this pull request Jun 28, 2022
Merged
@1Copenut 1Copenut mentioned this pull request Jul 5, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@thompsongl thompsongl thompsongl approved these changes

@miukimiu miukimiu miukimiu approved these changes

@cchaos cchaos Awaiting requested review from cchaos

Assignees

@1Copenut 1Copenut

Labels
accessibility - screen reader 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.
5 participants
@1Copenut @kibanamachine @cee-chen @thompsongl @miukimiu