fix(gatsby-image): Remove onClick prop

[madalynrose]

Description

This pull request reverts #18465 as discussed in #18468

Quoting @marcysutton:

Even with notes about affordances in the docs, encouraging clicking on images has the very real potential to produce inaccessible websites; something that conflicts with our commitment to accessibility.

[sidharthachatterjee]

Thank you @madalynrose 💜

[hu0p]

Would it be practical to include a custom propType validator that requires the necessary attributes to make this element accessible when an onClick prop is present e.g. title, aria-label, etc.?

If no, could we include a custom validator that throws an error when an onClick prop is present? I'm concerned that this is a bad developer experience. If someone attempts to add an onClick prop now, the functionality fails without any notification. They're left to wonder what they're doing wrong, and they may try to debug various components/libraries before finding the real culprit here in the repo issues/PR. If pre-existing functionality that utilized this prop was already present and they upgrade, that functionality is now broken without notification as well.

Ultimately, if nothing is done, I think that also means this change was breaking and the package version should semantically reflect that. 😕

I'm not sure if I should open a new issue or comment here, so if we should take this discussion somewhere else, let me know and I'll be happy to do so.

[polarathene]

@hu0p that feature was only available for about a month. While it's unfortunate for early adopters of the feature to experience what you have, it likely didn't see widespread use.

AFAIK, major versions for the package are for much larger changes where the breakage in API will be far more likely/widespread for consumers(especially libs/plugins that use gatsby-image, not direct users alone for features like this). Features do get deprecated with minor releases, among other changes. You could use semver to stick to patch version updates only?

There was a mentioned workaround, which wraps the image in a div to add the onClick prop, perhaps you can use that? If major versions were bumped for stuff like this, they'd have less significance/value tbh. The distinction between major and minor is still relevant and afaik handled correctly here.

EDIT: Seems only patch version was updated, not minor version, so yes I agree with you that has been handled wrong.

[hu0p]

@polarathene I'm glad someone at least somewhat agrees on this point. I anticipated some opposition here and I want to note that this is not a sword I want to die on. It's not my intention to be contrarian or oppositional. I just want to make sure everyone gets the best experience. I think most people will definitely acknowledge semantic versioning is not a perfect specification.

With that being said, I'd like to focus on the changes to propTypes. If we can get a new PR in (see below) that throws an error when an onClick prop is present, I think it would be a better experience for everyone. As anyone reading this probably knows, people expect adding an onClick prop to any component to "do something", and I don't think we should break with that pattern of behavior here. It should take minimal effort, and once someone sees it, if they take umbrage with the implementation, they're more than welcome to head to the repo and propose a better solution.

I've gone ahead and submitted a PR (#19723) that would do this with minimal effort. At the time this happened to me, I didn't have TS services turned on in VS Code (disabled by default), so I wasn't aware it had been invalidated. I think this will help users that don't know or aren't aware of TS at all.

[martin2844]

And here I was going crazy why onClick did not work on IMG tags.

I have a multilingual website that changes languages when you click on IMG with the flag. Putting Div tags is not a solution, at least elegant.

[Mattsi-Jansky]

@martin2844 there is another I think more intuitive solution, wrap the element in an <a> tag. Eg:

From:

<Img fluid={data} onClick={() => { window.open(data.src)}} />

To:

<a href={data.src} target="_blank">
  <Img fluid={data}/>
</a>

As an aside, I agree that this should've been a major version change. Your major version number is just an arbitrary number; I don't see why it matters if it becomes quite big.

[martin2844]

@Mattsi-Jansky is it though?

this is my code

`

    {data.allFile.edges.map((edge) => {
        let lang = edge.node.childImageSharp.fixed.originalName.substring(1, 3);
        let lang2 = lang.toUpperCase();
        return (
            <span key={lang} onClick={ () => {dispatch({type: 'TOGGLE_' + lang2})}}>
            <Img id={lang} className={'flags' + (lang === state.lang ? 'active' : '')} key={lang}   alt={lang} fixed={edge.node.childImageSharp.fixed} />
            </span>
        )

    }
    
  ).slice(0,3)}
    </span>

`

Before this change, my Img tag would have the dispatch onclick, but now, If I need to wrap it round another element I'd prefer a span tag.

Don't think they'll take the onclick property from span tags.

[Mattsi-Jansky]

Oh, my bad. My usecase is just turning the image into a hyperlink; I got wrapped up in that and missed that you're using it to run a more involved script.

[madalynrose]

Hello everyone!

This topic has gotten a lot of attention so we wanted to give a worthy reply. There are several topics converging here which makes it all the harder to know what to do. First off, we'd like to apologize and acknowledge that we made a few mistakes. The initial PR should not have been merged because it goes against our core philosophy of supporting the needs of accessible-required humans. Additionally after being merged, reverting in a minor version was semantically incorrect and caused frustration, loss of time, and general confusion. We are sorry!

While I don't have all the answers right now I wanted to shed light on some things in the sake of transparency. First, versioning is a topic we are sorting through as a core team right now. We've historically been intentional about creating major versions. While that is great for many reasons, issues like this come up and this one was bad. Additionally, we acknowledge that accessibility adds complexity to coding and is not always clear. While this is our mission, we have a lot of work to do to help people make the right accessibility choices.

For this specific use-case of wanting to attach an onClick handler to an image, the most semantic and accessible solution is to wrap the image in an anchor tag or a button, as suggested above by @Mattsi-Jansky (it should be a button for @martin2844's example since it is changing page content but not location). To add some context, img is a non-interactive element meant only to contain content and does not support event handlers. Adding an event handler to a non-interactive element will create a barrier to access for keyboard users, who wouldn't be able to reach that element. It will also make it difficult for the accessibility APIs that interpret HTML from the browser to accurately describe what's on the page to assistive technologies, thus creating an unintuitive experience for users relying on screen readers. The library Gatsby uses for accessibility linting, eslint-plugin-jsx-a11y, has a great explanation of this problem and examples of semantic approaches. I highly recommend using that library as a resource.

We are more than open to ideas to create logical APIs that support accessible-by-default implementations. But unfortunately the onClick handler is not coming back to the gatsby-image component. Again, we're very sorry to those who experienced this issue.

[Mattsi-Jansky]

Thanks for the thoughtful explanation. It was frustrating to have to figure out what was failing and why but I'm glad for the change, it has genuinely made my site more accessible and taught me something about web accessibility in the process.

And props to the team for prioritising accessibility, too many web developers don't have that in mind whatsoever.