New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onChange handler for radio buttons does not fire according to spec. #1471

Closed
EamonNerbonne opened this Issue May 1, 2014 · 29 comments

Comments

Projects
None yet
@EamonNerbonne

EamonNerbonne commented May 1, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 1, 2014

Member

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.

Member

sophiebits commented May 1, 2014

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

This comment has been minimized.

Show comment
Hide comment
@EamonNerbonne

EamonNerbonne May 5, 2014

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).

EamonNerbonne commented May 5, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 23, 2014

Member

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.

Member

sophiebits commented May 23, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 26, 2014

Member

@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.)

Member

sophiebits commented May 26, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 26, 2014

Member

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

Member

gaearon commented May 26, 2014

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

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 26, 2014

Member

Sure, no rush.

Member

sophiebits commented May 26, 2014

Sure, no rush.

@Anenth

This comment has been minimized.

Show comment
Hide comment
@Anenth

Anenth Jul 15, 2014

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

Anenth commented Jul 15, 2014

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

@itrelease

This comment has been minimized.

Show comment
Hide comment
@itrelease

itrelease Jul 27, 2014

@spicyj can't we just add

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

in _handleChange method?

itrelease commented Jul 27, 2014

@spicyj can't we just add

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

in _handleChange method?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jul 27, 2014

Member

@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.

Member

sophiebits commented Jul 27, 2014

@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.

fczuardi added a commit to fczuardi/mnmo-components that referenced this issue Mar 30, 2015

@EtaiG

This comment has been minimized.

Show comment
Hide comment
@EtaiG

EtaiG May 7, 2015

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..

EtaiG commented May 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gyzerok

gyzerok Jun 29, 2015

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

gyzerok commented Jun 29, 2015

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

@EtaiG

This comment has been minimized.

Show comment
Hide comment
@EtaiG

EtaiG Jun 30, 2015

@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.

EtaiG commented Jun 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jul 19, 2015

Member

@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?

Member

sophiebits commented Jul 19, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@EtaiG

EtaiG Jul 19, 2015

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

EtaiG commented Jul 19, 2015

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

@EtaiG

This comment has been minimized.

Show comment
Hide comment
@EtaiG

EtaiG Jul 21, 2015

@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?

EtaiG commented Jul 21, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@matchu

matchu Sep 10, 2015

Contributor

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 :)

Contributor

matchu commented Sep 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@matchu

matchu Sep 10, 2015

Contributor

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?
Contributor

matchu commented Sep 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@matchu

matchu Sep 10, 2015

Contributor

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.)

Contributor

matchu commented Sep 10, 2015

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

This comment has been minimized.

Show comment
Hide comment
@EtaiG

EtaiG Sep 10, 2015

@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.

EtaiG commented Sep 10, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@matchu

matchu Sep 11, 2015

Contributor

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.

Contributor

matchu commented Sep 11, 2015

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.

@keuss keuss referenced this issue Dec 1, 2015

Closed

Fix radio btn #1

@jimfb jimfb closed this in #5746 Apr 15, 2016

jimfb added a commit that referenced this issue Apr 15, 2016

Only fire input value change events when the value changes (#5746)
* Only fire input value change events when the value changes

fixes #554, fixes #1471, fixes #2185 (still trying to figure out why)

* catch programmatic value changes

* move value tracking to seperate module

@zpao zpao added the Component: DOM label Aug 3, 2016

ajreed79 pushed a commit to ajreed79/react that referenced this issue Aug 29, 2016

Only fire input value change events when the value changes (#5746)
* Only fire input value change events when the value changes

fixes #554, fixes #1471, fixes #2185 (still trying to figure out why)

* catch programmatic value changes

* move value tracking to seperate module
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 11, 2017

Contributor

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?

Contributor

ljharb commented Aug 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Aug 11, 2017

Collaborator

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

Collaborator

jquense commented Aug 11, 2017

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

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 11, 2017

Contributor

@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?

Contributor

ljharb commented Aug 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Aug 11, 2017

Collaborator

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.

Collaborator

jquense commented Aug 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Aug 11, 2017

Collaborator

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!

Collaborator

jquense commented Aug 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 11, 2017

Contributor

@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.

Contributor

ljharb commented Aug 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@peterbartos

peterbartos Sep 7, 2017

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.

peterbartos commented Sep 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@HoldOffHunger

HoldOffHunger May 30, 2018

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.

HoldOffHunger commented May 30, 2018

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

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon May 30, 2018

Member

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

Member

gaearon commented May 30, 2018

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

@facebook facebook locked and limited conversation to collaborators May 30, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.