Docs & Comments & Updates styles for EuiScreenReaderOnly#2
Conversation
…favor of always having the input
| focusable element, you should consider adding{' '} | ||
| <EuiCode language="scss">{'position: relative;'}</EuiCode> to the | ||
| focusable element. This will fix any screen reader focus rings to | ||
| stay within the bounds of the focusable element. |
There was a problem hiding this comment.
After some thought, I'm on the fence about adding an extra documentation section for this behavior, since as far as I can tell it mostly applies to just Safari + VO.
We might want to evaluate end-user usage and whether it's worth adding this documentation/advisement for a single browser+screenreader combo that WebAim reports in its 2021 survey to be 5% of desktop screen reader users (compared to JAWS and NVDA on Windows, which is 90%+). I think in light of the usage stats, that:
- This is a non-issue for the vast majority screen reader users on desktop
- Although I do want to caveat that for mobile screen reader users (50%+ of SR users), iOS Safari + VO is 50% of the market share. However, VO doesn't behave the same way on mobile as it does on desktop and this may be a non-issue there - we won't know for sure until we test on an iOS device. But to that point: IIRC we don't technically support/test EUI or Kibana on mobile devices.
- This is not an issue for Safari keyboard users, as Safari does not show the off-screen outline hitbox when not using VO
- This is also purely a visual outline issue - for a tool that is meant for non-visual users. I acknowledge there's a spectrum of vision impairment and it's possible some screen reader users will still see the off-screen outline and may be slightly confused by it, but nevertheless the audio behavior still works perfectly. With that, I'm going to argue that it's not worth it to document a workaround for a problem that maybe half of 5% of our users might affect.
To clarify, I'm 100% on board with adding position: relative for our external EUI links since it's common component usage and a small/easy fix for us, I just think this extra documentation section for it is unnecessary since it's incredibly browser+screen reader specific and functionally low-impact, as above.
There was a problem hiding this comment.
I just err on giving the consumers full knowledge up front of known issues and how to work around them. If you feel strongly, feel free to remove.
| }, | ||
| { | ||
| title: 'Skip link', | ||
| source: [ |
There was a problem hiding this comment.
I love the new page sections/organization!! It's a huge improvement and brings a ton of clarity to the previous iteration just IMO 😍
|
Sorry, I definitely got sidetracked looking up screen reader usage and that ended up dominating my comments/thoughts, but I also wanted to say everything else in the PR looks super great! 3rd comment is definitely my only blocking-ish comment/discussion I'd like to have, otherwise I'm happy to merge this in to the open PR 🎉 |
|
Apologies for the slowness! I realized yesterday all my email notifications for this PR were going to my personal email address instead of my work inbox 🤦♀️ After some thought I still feel fairly strongly about not documenting the position relative workaround for Safari+VO (unless we have precedence elsewhere for browser-specific workarounds or issues in our documentation). I think what could make sense instead is to open a GitHub issue documenting the known behavior - this will make the issue discoverable for more GitHub users and also allow them to comment/discuss, which will give us more helpful insight into usage and users affected. I'll go ahead and merge this PR, open an issue, and remove the section/example in the main branch if no objections! |
* Update .euiScreenReaderOnly to use clip - this prevents issues with absolute positioning and scrolling containers, and works on all modern browsers supported by EUI * [PR feedback] Add comment with more context * Fix screenReaderOnly CSS to only apply to check/radio inputs with labels * Clean up noLabel input CSS that was overriding previous screen reader CSS * Add changelog entry * [PR feedback] Docs & Comments & Updates styles for EuiScreenReaderOnly cee-chen#2 * [Docs] Updated Accessiblity Utility pages with more nuances about EuiScreenReaderOnly * Remove `euiScreenReaderOnly` mixin from checkbox and radio styles in favor of always having the input * Commented Sass * [EuiLink] Position relative if `target=_blank` for positioning screen reader text * Remove button/position relative documentation/example in favor of #5168 @see cee-chen#2 (comment) * Add 2nd changelog entry Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
tl;dr
This still fixes nothing but the original issue of overflowing content in tables. At least the caveats are documented and we can take a few extra steps down the line to rectify some of them on our side.
Docs
I re-wrote a lot of the Screen Reader Only docs to mention some of the nuances.
Comments
I updated the mixin and added comments, more line-by-line, detailing why each was important.
Updated styles
Based on your advice to just remove the mixin from the checkboxes and radios, I've adjusted those styles.
Here are some screenshots of the placement of the input in all 3 browsers, Chrome - Safari - Firefox (which is blurry cuz i use it to test non-retina stuff).
EuiLink
I also added a style specifically to EuiLink that makes is
position: relativeif it contains that screen reader text which is only there iftarget="_blank". This is just an example really, but we could/should comb through usages that we know exist inside a focusable element.