-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiPopover] Allow content to be accessible during opening animation #5249
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5249/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5249/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5249/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give @1Copenut a chance to review/test before approving, but code and staging both LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM! I tested this out with VoiceOver on MacOS and iOS, and NVDA (full version on trial) in Firefox and Chrome. The adjustment to remove visibility
doesn't disrupt the screen reader experience and no axe browser plugin issues were detected.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5249/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I checked most of our popover usage in Safari. Don't forget to revert the change to the progress chart. 😉
This reverts commit bf3a363.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5249/ |
…lastic#5249) * remove visibiliy:hidden; rely on opacity * revert me * more visibility removal * CL * Revert "revert me" This reverts commit bf3a363.
Summary
Resolves #5247 by removing
visibility: hidden
from EuiPopover open animation and instead relying onopacity: 0
.This allows the contents of EuiPopover to be accessible to the
node.innerText
method as well as screen readers.Main question(s) to answer: Is it important to hide the content from screen readers during the animation, or are we doing such users a disservice by delaying access to the content? If the former, how do screen readers handle
visible
content inside ahidden
parent (#5247 (comment))?See the EuiProgress "Progress for charts" docs section for resolution. Each label should have a
title
attr that matches the content.Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest tests