Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Recent radio input onChange changes break expected behavior #10739

Closed
darth-cheney opened this issue Sep 18, 2017 · 8 comments · Fixed by #11028
Closed

Recent radio input onChange changes break expected behavior #10739

darth-cheney opened this issue Sep 18, 2017 · 8 comments · Fixed by #11028
Assignees
@jquense @aweary
Labels
Component: DOM Type: Regression

Comments

@darth-cheney
Copy link

darth-cheney commented Sep 18, 2017 •
edited
Loading

Background

I see from this issue thread that recent changes were pushed in 15.6.x that have – to my mind – broken some of the expected behavior when it comes to firing onChange events in a group of radio button inputs.

I am aware of the conversation in #1471, but the problem it's describing is not exactly the same as this so far as I can tell.

Note that the problem I'm about to describe does not occur in React <=15.4.x.

Expectations

We would expect an individual radio button's onChange to fire in these two conditions (among others):

  1. A radio input is clicked and it's state changes
  2. A label for any radio input is clicked, causing (1).

Indeed, native elements work precisely this way:

<html>
    <head></head>
    <body>
        <input type="radio" id="radio1" name="radiogroup"/>
        <label for="radio1">One</label>
        <input type="radio" id="radio2" name="radiogroup"/>
        <label for="radio2">Two</label>
    </body>
    <script>
        var r1 = document.getElementById('radio1');
        var r2 = document.getElementById('radio2');
        var handler = function(event){
            console.log(event);
        };
        r1.addEventListener('change', handler);
        r2.addEventListener('change', handler);
    </script>
</html>

If you click the above radio buttons and observe the console output, you'll see that the event is firing as expected: only when a desired input's state is being changed to something new.

Here is a Codepen example

React Expectations

The following React component will work as expected in 15.4.x but not 15.6.x:

const RadioTest = (props) => {
    let handler = (event) => {
        console.log(event);
    }
    return(){
        <div>
            <input
                type="radio"
                name="radiogroup1"
                id="radio1"
                onChange={handler}/>
            <label htmlFor="radio1">One</label>
            <input
                type="radio"
                name="radiogroup1"
                id="radio2"
                onChange={handler}/>
            <label htmlFor="radio2">Two</label>
        </div>
    }
};

In 15.4.x React, this the inputs in this component will work as you'd expect and as the native DOM elements work. This Codepen shows it working properly with 15.4.1

In 15.6.x, the onChange event seems to only fire once. Here is another Codepen showing it doesn't work when 15.6.1 is imported.

It is, of course, entirely possible that I am doing something incorrectly (and have been doing so for a couple of years now), but the breaking change to my components only came with recent updates.
@aweary
Copy link
Contributor

aweary commented Sep 18, 2017

Thanks for the detailed report @darth-cheney! cc @nhunzaker @jquense

@jquense you've dealt with this the most, can you clarify whether this is/isn't expected? I also noticed that the change events only fire once (keep toggling and it will just stop dispatching events). Is that an issue we're already aware of?

@nifgraup
Copy link

nifgraup commented Oct 1, 2017 •
edited
Loading

regressed in 68347c9, pull request #8575

@jquense
Copy link
Contributor

jquense commented Oct 1, 2017

This was fixed in 15.6.2 and shouldn't exist in 16

@jquense jquense closed this as completed Oct 1, 2017
@nifgraup
Copy link

nifgraup commented Oct 1, 2017

issue is there on master

@darth-cheney
Copy link
Author

@jquense If you change the versions of the imports to 15.6.2 in by Codepen above, you'll see that the change event still only fires the first time. I am unable to test with React 16 in the Codepen for whatever reason.

@jquense jquense reopened this Oct 2, 2017
@jquense
Copy link
Contributor

jquense commented Oct 2, 2017

you're right! Sorry about that, i'm not sure how that happened...

@jquense jquense mentioned this issue Oct 2, 2017
Merged
@darth-cheney
Copy link
Author

Just to clarify, this also seems to be broken in v16. I swapped out the import in the Codepen with:

<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>

@jquense jquense mentioned this issue Oct 9, 2017
Closed
@zhenya-zhu
Copy link

broken in v16 too Orz

@aweary aweary mentioned this issue Feb 8, 2018
Closed
@sangle7 sangle7 mentioned this issue Feb 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jquense jquense

@aweary aweary

Labels
Component: DOM Type: Regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update value tracking on cousin radios facebook/react
6 participants
@nifgraup @jquense @gaearon @darth-cheney @zhenya-zhu @aweary