[EuiScreenReaderOnly] Add clip property to fix scrolling bug

[cee-chen]

[EuiScreenReaderOnly] summary

This PR contains #5130, which I'll re-copy the description & problem for here:

Problem

@qn895 reported an issue with their usage of EuiInMemoryTable within a scrolling container causing strange scrolling on the rest of their page/body. I was able to reproduce this behavior in our own docs page as well as in a CodeSandbox:

Notice the page scrollbar jumping/increasing every time the table rows (and consequently table height) increases.

CSS-tricks put me on the right track of thinking it was a hidden absolutely positioned element that was causing the overflow issue. I also noticed when I had a table with plain text and no external link, I didn't have the same scroll issues. Well, I put 2 and 2 together, and it turns out it's the .euiScreenReaderOnly part of the table content that's causing the scroll/overflow shenanigans. In above screencap, the external link icon has SR text to indicate the link opens in a new window, and in the table @qn895 had issues with, the actions column icons have SR text to indicate their behavior.

Solution

Adding to clip prevents the scrolling shenanigans mentioned above, and works on all modern browsers supported by EUI.

This should be fairly safe change in terms of browser support - additionally, WebAIM, WordPress, 18f, and a11yproject all have snippets using clip.

Some fun asides I stumbled upon during testing:

• clip is apparently going to be deprecated, which is why I included clip-path (per the above links)
• Although interestingly enough on Firefox, clip-path didn't solve the above scroll issue by itself - only clip did
• On the subject of browser-specific shenanigans: in FF, I was able to remove the top and left positioning properties totally and the fix worked. However, on Chromium/webkit, just clip alone didn't fix the scrolling issue: I needed left: -10000px too 😖 The only other place I could find where someone else had this issue was this filed bug, but they seemed to come to the conclusion the issue was with FontAwesome, not Chromium (which I sorta disagree with, but c'est la vie). Maybe someday someone will google around and come across this issue, and the cycle of the internet will be complete :)

[EuiCheckbox/EuiRadio] summary

The above clip CSS broke single EuiCheckbox/EuiRadio that had no labels (#5149). @cchaos came up with a solution in cee-chen#2, where we avoided using .euiScreenReaderOnly on checkbox/radios inputs entirely and instead positioned hidden inputs over their faux inputs.

Additionally, @cchaos added and cleaned up documentation, and fixed external EuiLinks outlines going off-screen in Safari+VO by setting left: 0 and adding a position: relative wrapper.

Checklist

- [ ] Check against all themes for compatibility 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 the any docs 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

[cee-chen]

I put this in draft mode since we should likely wait for 37.6.2 to release first to do a 38.0.0, but this should be ready for any extra review/QA on top of what was done in #5130 and #5149.

Also heads up on this @qn895 - due to this being a breaking change release, this the will take a little longer to get into Kibana master, but should still arrive well before 7.16 FF. Please feel free to ignore the scrolling behavior on your plugin until then!

[kibanamachine]

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

[kibanamachine]

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

[kibanamachine]

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

[kibanamachine]

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

[cchaos]

👍 LGTM. Though I wonder if we should also make not of the checkbox/radio input changes in the CL as well.

[cee-chen]

👍 I'm good with that - what do you think about VoiceOver+Safari: Fixed focus outline going off-screen for checkboxes, radios, and external links? (Per the wiki the focus of the changelog is on functionality affecting end-users, not on internal implementation details, although it's not clear to me whether CSS is 'internal implementation' 🤔)

[cchaos]

I don't think that's a change though from what exists in master?

[cee-chen]

I just tested https://elastic.github.io/eui/#/forms/form-controls#checkbox in Safari+VO and the outlines are going off-screen. Are they not for you?

[cchaos]

Right so there's no change from master to this PR, so there's nothing to note? Sorry, I guess I'm not understanding ...

[cee-chen]

This PR fixes that per the commit you merged in: 5833109

https://elastic.github.io/eui/#/forms/form-controls#checkbox:

https://eui.elastic.co/pr_5152/#/forms/form-controls#checkbox:

[cee-chen]

@cchaos quick re-ping, can I get a sense of where you're at on the additional CL entry? It sounds like you don't think we should add one after all for checkbox/radio changes (or possibly the affected users is so minimal it's not worth an entry) - am I good to merge in this PR in that case?

[kibanamachine]

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

[cee-chen]

Heya @1Copenut! Would you mind reviewing this PR today or tomorrow since it's accessibility-related? Happy to hop on a call if needed to provide any extra context!

[1Copenut]

@constancecchen I'll give this a look first thing in the morning. The afternoon got away fast.

[1Copenut]

👍 LGTM! I like what you did to bring the SR-only text into proximity with the inputs and links. I've noticed VoiceOver moving the black border around in the past, but never put it together this was because of the SR-only text's layout placement.

What do you think about adding a couple of guardrail rules to your CSS? Your screen shots are good representations of the visual, so they may not be needed. I'm defensive-minded and tend to plan for unexpected cases.

! global_styling/mixins/_helpers.scss
! These suggestions are preventative, probably not needed

@mixin euiScreenReaderOnly {
+ border: 0;
+ padding: 0;
}

[cee-chen]

Nice! I've no strong opinion about resetting border or padding personally - @cchaos, any thoughts there?

[cchaos]

I would first check to see if those properties even affect the display. Technically, if it's just a plain element our reset file should have already eliminated those and without adding the !important modifier it may not have any affect at all.

[cee-chen]

Thanks Caroline! Since I'm trying to get this PR in for the next release/today, I'm going to go ahead and merge in for now for expedience. But we can definitely add any extra resets needed in a future PR or if we have issues with EuiScreenReaderOnly again in the future (knock on wood)!