ethanselzer / react-image-magnify

A responsive image zoom component designed for shopping sites.
https://ethanselzer.github.io/react-image-magnify
MIT License
649 stars 160 forks source link

Option to change injected HTML container #56

Open markology opened 5 years ago

markology commented 5 years ago

Hey, I work as a part of the UX team for bedbathandbeyond.com. We leverage your plugin for our product images and we have to defer to our legal department to uphold code standards for accessibility.

The issue we are having is the div container thats injected. We wrap the result of your plugin inside of an anchor tag which doesn't have a valid href and could cause some confusion with traffic using screen readers. We would like to change it to a button for accessibility's sake but divs are invalid children of buttons and that wouldn't be correct practices either.

Is the div necessary and if so, is there a way that we can have an alternative such as a span so we can adhere to safe and accessible implementation?

Any feedback is appreciated. Thanks.

ethanselzer commented 5 years ago

@markology - Thanks for opening this issue.

Is the div necessary and if so, is there a way that we can have an alternative such as a span so we can adhere to safe and accessible implementation?

react-image-magnify uses div elements as the positioning context for the enlarged image container and enlarged image. I think the div elements are necessary and that div is an appropriate element choice in this case.

In your second paragraph you describe using an anchor tag as the parent element to react-image-magnify. I'm wondering if you have considered implementing role="button on your anchor element instead of changing it to a button element. From MDN: "The button role identifies an element as a button to screen readers". Please let me know your thoughts.

markology commented 5 years ago

@ethanselzer Thanks for the quick reply and the recommendation.

We do use role=button currently but unfortunately it is not fully compliant across all browser/screen reader combinations and doesn't fully comply with WCAG 2.0 AA guidelines as we are faking semantic behavior of an element. A true button element has built-in keyboard and screen reader functionality, so it's better to leverage this when we can.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role#Accessibility_concerns

If there was an optional plugin prop that could dynamically set that wrapping element (experimentally at users risk) or a span with display block it would be greatly appreciated and satisfy this need. This plugin is awesome and we'd love to be able to give our customers access to your features inclusively.

markology commented 5 years ago

Hey @ethanselzer, just following up to see if you had thought more into this or had any plans to add some functionality to facilitate this ask. Any feedback or code updates are greatly appreciated.

Thanks

markology commented 5 years ago

Hey @ethanselzer , just another follow up since I haven't heard back with any updates on this issue. Would love to move forward with some sort of solution!

markology commented 5 years ago

Hey @ethanselzer , just another follow up since I haven't heard back with any updates on this issue. Would love to forward with some sort of solution!