-
Notifications
You must be signed in to change notification settings - Fork 844
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
Improving EuiImage accessibility #2447
Improving EuiImage accessibility #2447
Conversation
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.
The markup is as was agreed on in the issue. I just found one underline bug, a BEM change, and a translation needed.
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.
Markup looks good to me and the behavior is the same as before. I'd like to get @myasonik to check the a11y works as expected.
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.
Found a problem while testing - there's no way to navigate "into" a button...
So, right now, when you land on a button it reads out the aria-label
and there's no way to get to the image. Removing the aria-label
reads out the image's alt
which isn't too bad but it doesn't say it's an image, it keeps the role of button
and you don't know what the button does.
I see two paths we can take (if someone else has a proposal, please throw it in because I'm not a huge fan of either):
- Revert back to the HTML I proposed earlier in the ticket though this will force some hacky CSS
- We can add some hacky HTML to make this read a little better. Something like this:
<button>
<img alt="" src={`Image of ${altProp}`} />
<EuiScreenReaderOnly>Open in full screen</EuiScreenReaderOnly>
</button>
@cchaos @miukimiu - thoughts?
Co-Authored-By: Caroline Horn <[email protected]>
I think the second option (with the EuiScreenReaderOnly) is totally valid and is better than hacky CSS that is hard to maintain and ensure correct rendering in all browsers. However, there is a third option that I like best, which @miukimiu just implemented which is to add the |
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.
🎉 Big improvement! Awesome!
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.
If @myasonik's happy, I'm happy 😜
Summary
Improves
EuiImage
accessibility. Closes #2347With this PR I'm trying to remove the
figure
element from inside thebutton
to provide a better accessibility.I also:
euiImage
class is now being shared when the image is in fullscreen or not. When the image is in fullscreen some specific styles are added by using the modifiereuiImage--isFullScreen
outline
styles to the button rather than the image<img />
and<EuiIcon />
Checklist