React: onChange handler for radio buttons does not fire according to spec.

[EamonNerbonne]

As described on http://facebook.github.io/react/docs/forms.html, the onChange handler of an input should fire when the checked state changes. However, it actually fires when the radio button is clicked,

In other words, it fails to fire when a checked radio button is unchecked (by checking a different radio button), and it fires even without a state change when a checked radio button is clicked.

In short, it's missing checked transtions true->false, and it's reporting spurious transitions true->true.

[sophiebits]

I believe it's intentional that it doesn't fire for true->false (because you really only want one event) but the true->true should not be counted. Essentially you're getting a change event for the radio group rather than for the individual radio button.

[EamonNerbonne]

I guess that makes some sense (I'm not sure it's ideal, but it's what the DOM does too...), but in that case the docs aren't accurate (and the true->true issue remains).

[sophiebits]

Agree. If someone wants to tackle this, it would be nice if "changing" from checked to checked didn't trigger an onChange event. I'm also happy to accept a docs PR to make the expected behavior clearer.

[sophiebits]

@gaearon Interested in taking a look at this one?

Right now ChangeEventPlugin fires an onChange event for every click that it receives from the browser (for both checkboxes and radio buttons). My guess is it's necessary to track the current value when the focus event is triggered, sort of like we do for activeElementValue in old IE. Not sure how tricky it will be to solve. (http://benalpert.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html explains approximately how the onChange event currently works for text inputs.)

[gaearon]

@spicyj Yup, I'm interested. I can take a look closer to the weekend, OK?

[sophiebits]

Sure, no rush.

[Anenth]

Onchange is not triggered when there is a change from true->false

[itrelease]

@spicyj can't we just add

if (this.props.type === 'radio' && this.state.checked) {
  return;
}

in _handleChange method?

[sophiebits]

@itrelease No – at the point that handleChange is called, the checked value has already changed and we do want to fire the onChange event if a radio button becomes checked.

[EtaiG]

Hi,

I encountered this issue as well - it still happens.

jsfiddle- native html, vanilla js: https://jsfiddle.net/etai/yfsmqsqo/2/
jsfiddle- react 0.13.1 - http://jsfiddle.net/etai/5pcht4ap/2/

on each, select a radio, then select one which is already selected.

Any idea when this will be fixed?

This forces us to abort the onChange when 'this' radio is the selected value, which is just ugly..

[gyzerok]

@spicyj Can you give a starting point to dive into this? I'd like to try this as my first PR for React.

[EtaiG]

@gyzerok I have a fix for this, but unfortunately it's nearly impossible to write tests for react. It was a nightmare, I spent over 12 hours just trying to get it working, even with help of some people on the react IRC channel, with no success.

[sophiebits]

@EtaiG Sorry for the delay. If the change is simple, we can take it without tests and if it's complex, maybe I can write some. Do you want to post a PR with your code as it is now?

[EtaiG]

@spicyj Thanks for answering.
I'll make a PR tomorrow... it's such a small change

[EtaiG]

@spicyj,

It Looks like I deleted my changes in my local react clone since. Though I still have my test written (even though I can't run it) :)

Anwyay, I merged react's master locally and checked the code again, so I'll explain the problem and the possible solutions (PR later if you want).
Sorry for it being lengthy, but I think this should clear up what's going on and how we can move onwards from here to get the radio to work according to the spec.

The problem is caused by the fact that react uses the native click event to listen to user interaction with the radio, and then calls the onchange for the input. The reason for this (at least according to the comment in the code) is because IE8 does not fire the change event until blur, so in order to be 'compatible' with IE8, the click event is used.

There is a tradeoff- supporting IE8 for a normal radio, while losing the W3 standard for radio, and also causing anyone who uses react and wants to specifically know when the radio selection has changed, to have to write more stateful code.

So the possible solutions to this problem:

1. Not support IE8, listen to the change event. Probably unwanted due to the fact that there are other solutions, and that this _fix_ was done intentionally.
2. Make the radio input stateful. It will keep track of the current selection, and won't call the onchange if the same radio button is clicked on. Although I generally like to keep things stateless if possible, this solution is good because anyway the react library currently forces developers using it to have state for this use case ('bubbling' the state up), and if written properly, this will just keep the state under the hood.
3. Assume that there is always either a 'checked' prop passed to the radio (if it will be the one which is checked). This is good too, since it's just changing the API to state that you must pass 'checked' for the checked radio button, which forces the developer to be declarative and is a legitimate requirement in react

2 will work even if 'checked' is not passed to any radio, and 3 will work otherwise and is probably the cleanest solution (with the smallest amount of code).

It's not possible to try and check the DOM for the value, since the 'click' event arrives after the change already occurred.

I personally like 3 the most, but 2 is good too. Maybe there's something else I didn't think of.

So... what do you think?

[matchu]

For what it's worth, we're running into this, too. I saw some code that relied on onChange firing even when the checkbox was already checked, so I wanted to know whether that was intentional, stable behavior. Sounds like no.

Thanks for being on this, y'all :)

[matchu]

My vague thoughts:

• If a solution exists that supports IE8, we should do it.
• Changing the radio button API feels dangerous. There might be apps out there that depend on passing no checked prop to a radio, so, if a solution exists that wouldn't break such apps, we should do that.
• That seems to lead to the state-based solution. Have you already written it up, @EtaiG?
  
  
  
  • My paranoid concern with the state-based solution is whether there's any way in all the universe that we could possibly get out of sync. I guess click events are the only way to change checked state aside from direct programmatic changes (input.checked = true), though, and I feel like the rest of React doesn't defend against programmatic changes (?), so it might be okay to ignore those concerns here, too?
  
  

[matchu]

And, I guess if we get out of sync, it's not _that_ bad.

If we only use the stored state to decide whether the click event is really a change, then we won't, like, deadlock the checkbox and make it ignore inputs.

Worst-case scenario, if someone screws with the state underneath us, we miss an onChange (bad) or fire a redundant one (not as bad :P). My gut says that most apps can recover from that.

(And, even then, I _think_ programmatic changes fire onChange events? So, if one happens, we _could_ try to catch it and update our stored state accordingly.)

[EtaiG]

@matchu I didn't write it up as a PR yet, though I tried all of the versions here to see what's possible.

I'll write them up this weekend and post them here, and then maybe make a PR out of one or all of them.

[matchu]

Sweet! Thanks, @EtaiG :)

Incidentally, today I discovered a new issue in synthetic events for radio buttons: #4854. That might be an interesting follow-up project since you've already explored React's radio button event system... or you might just be tired of it all, which is fine, too ;P Our app is gonna patch over it for the time being, anyway.

[ljharb]

This kind of seems like it might be a breaking change for some people; especially since the behavior's been the same for all versions of React up until 15.6 :-/ should this have waited for v16?

[jquense]

@ljharb What specifically are you concerned about? This seems like maybe nit the issue you meant to comment on?

[ljharb]

@jquense this is the correct issue - new in React 15.6 (not in 15.5 and below), onChange will no longer fire for clicks/activations on already-selected radio buttons. Someone relying on that to log activations (including keyboard activations) on already-selected radio buttons would have their code broken in v15.6, if I'm understanding correctly?

[jquense]

Ah ok, onChange firing twice for double clicks or for already selected radios was a bug, it wasn't ever the intended behavior. I'd place it in the same category as the other issues fixed by the PR, such as range inputs not firing consistently or inputs missing events in IE.

[jquense]

Just adding, if you think we missed a case we're the old behavior is more "correct" in some sense or for a use case we'd be glad chat about it, but might be better to open another issue since this one has a ton history at this point.

I'm also sorry if we inadvertently moved your cheese :/ the input events code is hairy and the tension between semver breaking changes and bug fixes is particularly tough to navigate here!

[ljharb]

@jquense perhaps, but it was a "bug" for fifteen major versions of React. I definitely agree that this change ends up with the correct/better behavior - but we're having to make changes in Airbnb's codebase to account for it pre-upgrade (that would have broken with the upgrade, but worked with 0.13, v14, through v15.4) - so I think it probably would have been better to consider it semver-major, and I hope that in the future, React will err further on the side of caution in non-major releases.

[peterbartos]

I agree that it is a breaking change and in our app it changes behaviour.

The original behaviour was more convenient: We were able to improve a Radio Button Group -> after selecting the same radio item, it was unselected. Without having to introduce an extra button to unselect all – which is needed sometimes.

Now there is no event, we could use to unselect already selected radio group.

[HoldOffHunger]

I'm also having this problem.

Radio onChange={(e) => this.handleOnChange(e)} is not firing for radios, whether the selected radio's value has changed or not. On the contrary, this fires, though :

onClick={(e) => this.handleOnChange(e)}

Also, onChange=... works if I change the input type to text.

[gaearon]

If you have a problem please post a new issue. It’s impossible to keep track of old closed issues.