alphagov / govuk_frontend_toolkit

❗️GOV.UK Frontend Toolkit is deprecated, and will only receive major bug fixes and security patches.
MIT License
403 stars 107 forks source link

Proposal: listen for change events rather than click events in show-hide-content.js #392

Closed pcraig3 closed 6 years ago

pcraig3 commented 7 years ago

This is a bit odd because this problem doesn't exist right now in the show-hide-content.js code, but it seems like a gotcha just waiting to trip up anyone thinking of extending it (for example, it happened to me).

Inside the click event for a radio, the clicked radio element will give back different values of .checked depending on whether it has been clicked in a browser or a .click() event.

-> If it has been clicked in a browser, this.checked returns true -> If it has been clicked using .click(), this.checked returns false

I've done up a little demo on jsfiddle.

It seems like checkboxes used to behave the same way, until a few years ago when this behaviour was changed (here's the jQuery bug report).

Additionally, it looks like the latest version of jQuery (version 3.2.0) doesn't do this anymore (here's the commit).

I think we can call this a bug.

We're not checking if radios are checked anywhere inside the code, although we are for checkboxes. Seems like it might be a problem we run into in the future.

There are different ways to fix this -- these are a few that occurred to me:

I ran into this problem on the digitalmarketplace frontend toolkit (where we need to know the checked state of a radio button) and I went with listening for change events because it seemed the most lightweight change and I couldn't think of a situation where we would get the wrong behaviour. We don't get events anymore for multiple clicks on already-checked radios but I can't think of a situation where we would care about that.

What do we think?

tombye commented 7 years ago

Just out of curiosity, why is support for click events fired by JS useful? I'm assuming it's to support selenium-like browser testing (like capybara) but be good to be sure.

36degrees commented 6 years ago

Thanks for raising, @pcraig3.

Following the launch of the GOV.UK Design System, GOV.UK Frontend Toolkit will now only get major bug fixes and security patches, so I'm going to close this.