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

[euiScreenReaderOnly] Use clip to fix scrolling issues caused by absolute positioning #5130

Merged
merged 3 commits into from
Sep 2, 2021

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 2, 2021

Summary

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:

scroll_issue

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.

Reproducing

You can check out this commit on my fork to see the issue locally on http://localhost:8030/#/tabular-content/in-memory-tables. You can also cherry-pick that commit to this fixed branch to see the fix.

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 :)

QA

  • Confirm that http://localhost:8030/#/utilities/accessibility links are still visually hidden but appear on keyboard tab/focus
  • Confirm that other components that use the @include euiScreenReaderOnly mixin still work as expected:
    • Checkbox & radio inputs
    • Key pad menu checkboxes/radios
    • Resizable container
    • Datepicker

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

- this prevents issues with absolute positioning and scrolling containers, and works on all modern browsers supported by EUI
@kibanamachine
Copy link

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

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.

❤️ I love the research that you put into this and specifically trust the A11y project's solution. And definitely a good one to share in the newsletter since I've seen some "hacks" in Kibana around this issue.

I only tested the Accessibility page (in Chrome, FF & Safari). 👍

src/global_styling/mixins/_helpers.scss Show resolved Hide resolved
@cee-chen cee-chen enabled auto-merge (squash) September 2, 2021 17:57
@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit bca85fb into elastic:master Sep 2, 2021
@cee-chen cee-chen deleted the screenreader-only-clip branch September 2, 2021 18:34
@cee-chen cee-chen restored the screenreader-only-clip branch September 7, 2021 17:41
cee-chen added a commit to cee-chen/eui that referenced this pull request Sep 7, 2021
cee-chen pushed a commit that referenced this pull request Sep 7, 2021
* Revert "[euiScreenReaderOnly] Use `clip` to fix scrolling issues caused by absolute positioning (#5130)"

This reverts commit bca85fb.

* Add changelog entry
@cee-chen cee-chen deleted the screenreader-only-clip branch September 7, 2021 18:56
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.

3 participants