Eui image focus states

[miukimiu]

Summary

Closes #1983

Added hover and focus states when allowFullScreen is true in EuiImage.

Non-clickable images were being wrapped by a button so I removed the button for images that aren't clickable.

Checklist

• Checked in dark mode
• Checked in mobile
• Checked in IE11 and Firefox
• Props have proper autodocs
• Added documentation 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

[chandlerprall]

Code changes LGTM; pulled and tested functionality locally, would be best to have a designer approve as well.

[cchaos]

Looks like this is still missing hover and focus states while in the actual full screen mode.

Maybe also displaying a cross icon on hover while in full screen mode will help to indicate that clicking will exit full screen?

---

As we discussed on zoom, I think the slight shadow change may be too subtle, at least for the focus state. See if there's a way to use the focus ring variables or mixin?

[myasonik]

How'd you feel about tackling some of the other a11y issues with images as well? 🙏

TL;DR my proposed final markup (including the optional figcaption and full screen button) is:

<div>
	<figure role="figure" aria-label={figcaption}>
		<img src={src} alt={alt} />
		<figcaption>{figcaption}</figcaption>
	</figure>
	
	<button aria-label=`Show ${alt} image full screen`>
		{fullScreenIcon}
	</button>
</div>

So, why these changes?

1. figure elements aren't allowed inside of button elements which makes it tricky to get to the image for screen readers (you can try it out and get more info about it from w3c's nu validator
2. Figure needs the role fallback for IE11 and the aria-label for some screen reader/browser combinations to be accessible (Scott O' Hara's if you got more details)
3. The button will now always tell screen readers what it does

Other notes

1. You can move the focus state to the containing div using :focus-within for supporting browsers (excludes IE11 and Edge, though Edge@next supports it, so you'll still need a workable fallback)
2. I 100% agree with you that the drop-shadow is way to subtle. I had to go double check that I was on the right branch because I thought there was no focus state and then I had to go check in the dev tools to see that it was being applied.
3. If there's no button you can get rid of the surrounding div as well though maybe that's more effort than it's worth. If there's no figcaption probably removing the figure is the right call all together though it is hard to discern implementing developer intent. If there's no figcaption or button you could remove everything except the img which is now a pretty substantial simplification of the DOM so maybe all of this is worth it after all.
4. Not included in my a11y improvements suggestions is any mention of that our focus trap isn't perfect for full screen takeovers like this but I think that's a big enough problem left for later ([EuiModal] Accessibility  #502).

[cchaos]

Thanks @myasonik ! However, I think all of that is out of scope of this PR. This PR is mainly to just address the missing :hover, :focus states. Would you mind, however, creating an issue with your findings and proposed solutions?

[miukimiu]

@cchaos when I've seen your comment I was almost finishing the a11y suggestions.

@myasonik and @cchaos here's what I implemented so far:

1. I removed the figure elements from inside the button.
2. I moved the focus state to the containing div using :focus-within. Now we need to support (IE11 and Edge). I was wondering how I can tackle this. Should I create a new issue?
3. I simplified the structure so when there's no figcaption there's no figure surrounding the image. But there's a div to handle some required classes.

@cchaos can you review the new focus and hover states?

[cchaos]

@myasonik @miukimiu It seems to me that there's an even simpler approach to the clickability other than using :focus-within and absolute positioning the button.

The spec states that <figure> can contain any flow content. A <button> is a type of flow content. Therefore, we can wrap the <img> tag with the <button> but inside of <figure> like so:

<figure role="figure" aria-label={figcaption}>
  
  <button aria-label={`Show ${alt} image full screen`}>
    <img src={src} alt={alt} />
    {fullScreenIcon aria-hidden="true"}
  </button>

  <figcaption>{figcaption}</figcaption>

</figure>

[miukimiu]

@cchaos and @myasonik I reverted to structure to what it was before the a11y changes. I also created a new branch here, so we can have the accessibility discussion there. I'm going to create an issue for that.

[cchaos]

This is great! I think pushing off the a11y work to another PR is a good idea as to separate the concerns and just focus on actually fixing the issue referenced.

Just saw a typo and want to make sure you've tested in other browsers, but visually it works well and the code looks good! 👍

[cchaos]

Much better 🎉

[thompsongl]

@miukimiu #2328 merged, so I'll get the conflicts sorted for you

[thompsongl]

jenkins test this