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

Add showOnFocus prop to EuiScreenReader Only & Add EuiSkipLink #2976

Merged
merged 9 commits into from
Mar 5, 2020

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Mar 3, 2020

Fixes #2356
Fixes #2962

Summary

In order to address WCAG A guidelines, a skip link should be provided that allows keyboard and screen reader users to bypass navigation and jump to the main content of the page.

As discussed in #2356 , the consensus approach was to:

  1. Add a showOnFocus prop to EuiScreenReaderOnly that forces the display of visually hidden content when it receives focus via keyboard, and
  2. Create a new EuiSkipLink component that leverages the aforementioned prop while providing additional styles.

Notes

Docs fix

This PR also fixes the empty Props tab for EuiScreenReaderOnly as noted in #2962 . This component only returns a cloned element (no JSX), so a workaround pattern suggested by @chandlerprall was used to populate the Props tab.

Regarding the EuiSkipLink docs example

Due to the way the EUI routing system handles hashed URLs, the EuiSkipLink component does not currently work on the EUI docs site (i.e. it treats an anchor href like href="a11ySkipToMain" as a new URL).

Once the upcoming infrastructure work towards a static system is in place, we can hook this up properly on the EUI site. In the meantime, I've added a TODO regarding an override in the code and a note on the docs example explaining that the link won't work.

For that reason, the "Skip to main content" link will only appear when you tab into the Utilities > Accessibility page (i.e. doesn't work globally) and I'm thus using an EuiPortal and tabIndex={1} to effectively fake it for the time being. Ultimately, this will live near the top of the body.

Keyboard focus in Firefox

For mac users, the skip to content won't display unless you've complete the two items in the first answer of this SO post: https://stackoverflow.com/questions/11704828/how-to-allow-keyboard-focus-of-links-in-firefox

IE11

It does display in IE11, but the tab order feels slightly off. That said, due to the aforementioned issues with making this work in the EUI docs, I'm using an EuiPortal so its not really at the top of markup since I'm not able to make this work globally (yet).

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@ryankeairns ryankeairns marked this pull request as ready for review March 3, 2020 23:10
@ryankeairns ryankeairns requested review from cchaos and myasonik March 3, 2020 23:11
@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Functionally this all works great, just some tweaks to implementation.

src/components/accessibility/_screen_reader.scss Outdated Show resolved Hide resolved
src/components/accessibility/screen_reader.tsx Outdated Show resolved Hide resolved
src/components/accessibility/screen_reader.tsx Outdated Show resolved Hide resolved
src-docs/src/views/accessibility/screen_reader.tsx Outdated Show resolved Hide resolved
src-docs/src/views/accessibility/accessibility_example.js Outdated Show resolved Hide resolved
src/components/accessibility/_skip_link.scss Outdated Show resolved Hide resolved
src/components/accessibility/_skip_link.scss Outdated Show resolved Hide resolved
src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
src/components/accessibility/skip_link.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@ryankeairns ryankeairns requested review from myasonik and cchaos March 4, 2020 17:29
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Mar 4, 2020

@ryankeairns I pushed the prop change we talked about and the type, test and example updates as well. I think all it needs now are some snippets?

@ryankeairns
Copy link
Contributor Author

Thanks @cchaos , I'll add some snippets.

@kibanamachine
Copy link

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

@ryankeairns
Copy link
Contributor Author

For those reviewing, the fixed example is appended to the end of the markup which is only temporary until the EUI docs site switches to the static system. At that point, we'll add an actual 'Skip to main content' link to the site in a global location.

If you reviewed this previously, I was hacking around this by using a tabIndex={1} on the fixed example which made it appear as the first item after the address bar, but with recent changes that is no longer possible (for good reason). In other words, when used the position="fixed" layout, the tabIndex is now forced to 0.

@ryankeairns ryankeairns requested a review from cchaos March 4, 2020 20:11
@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

All the code LGTM! One thing I hadn't tested before though is that when the fixed version (even though it's just docs) is in a portal, since portals are at the end of the DOM, the next tab is to the browser's nav instead of the next element in the DOM structure. What was the reasoning for the portal on that one?

Screen Recording 2020-03-04 at 03 59 PM

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Mar 5, 2020

@cchaos placing it in a portal with a tabindex of 1 originally allowed me to fake the correct order and visual/fixed placement on the docs site. With your last commit, that index value is now forced to 0 for the fixed position, which is a good enforcement, and it also means my hacky solution no longer works.

I should be able to remove the portal now and it will remain the next in tab order after the switch. I’ll try that in the morning, and then we’ll just live with it until the static site structure is in place, at which point we can place it correctly atop the body and do away with the switch altogether (i.e. we can then just say “Try it on this site”).

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🎉

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

Successfully merging this pull request may close these issues.

EuiScreenReaderOnly - Props do not appear on docs site Skip links component
5 participants