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.

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

bug fix on input radio #11227

Merged
merged 9 commits into from Nov 19, 2017
Merged

bug fix on input radio #11227

merged 9 commits into from Nov 19, 2017

Conversation

@landvibe
Copy link
Contributor

@landvibe landvibe commented Oct 14, 2017

Fixed: #7630

The PR#7333 made this issue.
And now there is no validation warning that PR#7333 is mentioning.
I think React 16 resolved the validation warning issue, so I reverted it to the previous of PR#7333.
I also added a test case for #7630

landvibe landvibe
@landvibe landvibe mentioned this pull request Oct 15, 2017
11 of 29 tasks complete
// Update the wrapper around inputs *after* updating props. This has to
// happen after `updateDOMProperties`. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
// Update the wrapper around inputs *after* updating props.

This comment has been minimized.

@jquense

jquense Oct 26, 2017
Collaborator

this comment isn't correct anymore since you've moved the updateDOMProperties call below it. We would need to confirm that the original issue with validations firing doesn't occur. Can you write a dom fixture that demonstrates the issue and that it doesn't occur after this change?

@nhunzaker Any suggestions on how to write that, or what conditions triggered it?

@jquense
Copy link
Collaborator

@jquense jquense commented Oct 26, 2017

So i'm not sure that the unit tests will catch this ( @nhunzaker might know) effectively give that it's a browser specific warning? I think we should move the original unit test in #7333 to a DOM fixture and ensure that it works. I think we also need understand why this was fixed inadvertantly in v16 before merging

landvibe added 2 commits Oct 27, 2017
landvibe landvibe
make radio's checked updated before the name
landvibe landvibe
@landvibe
Copy link
Contributor Author

@landvibe landvibe commented Oct 27, 2017

@jquense

I tested https://jsfiddle.net/97gr5e65/1/ made by @aweary.
And I realized React 16 did not resolve the validation warning.
So I don't think it is a good idea to edit @@nhunzaker's code.
And I started over.

After some debugging, I found an interesting point.
When we edit a radio's name with checked===true, browser makes another radio's checked false.
In the middle of an update, it is possible to have multiple checked===true.

Let me explain it with the test case I added.
There are two radio inputs.

initial state:
radio1: {
name: 'firstName',
checked: false,
}
radio2: {
name: 'firstName',
checked: true,
}

promising state after update:
radio1: {
name: 'secondName',
checked: true,
}
radio2: {
name: 'secondName',
checked: false,
}

step1, update radio1:
radio1: {
name: 'secondName',
checked: true,
}
radio2: {
name: 'firstName',
checked: true,
}

step2-1, update radio2's name:
radio1: {
name: 'secondName',
checked: false, // whoops...
}
radio2: {
name: 'secondName',
checked: true,
}

I think if we update checked first, it would be ok
So I added updateChecked function.

@nhunzaker
Copy link
Collaborator

@nhunzaker nhunzaker commented Oct 28, 2017

Hey everyone. Sorry! I was out of town and lost track. I'll start checking this out.

landvibe landvibe
Copy link
Collaborator

@nhunzaker nhunzaker left a comment

Tested in:

  • IE9-11
  • Safari Desktop 7.1, 8, 11
  • Safari iOS 9
  • Firefox 47, 56
  • Chrome 42, 62

This checks out!

@jquense I made a test fixture to manually verify this: nhunzaker@f18c4c4

@landvibe Great work spotting this! Do you mind if I push this fixture to your PR?

Still, we need to figure out why restoreControlledState isn't firing in this instance. Right now, I see that as the root of the problem.

Alternatively, could we just call updateNamedCousins where this PR now has updateChecked? Is there an advantage to one over the other?

// When a checked radio tries to change name, browser makes another radio's checked false.
if (tag === 'input') {
ReactDOMFiberInput.updateChecked(domElement, nextRawProps, 'radio');
}

This comment has been minimized.

@nhunzaker

nhunzaker Nov 7, 2017
Collaborator

I'm trying to figure out why this is necessary (but it is definitely effective).

updateNamedCousins in ReactDOMInput should take care of this, but it looks like it doesn't run in the test case you've provided.

@jquense updateNamedCousins gets called in restoreControlledState, but for what ever reason it only fires on the button, which I guess is clicked and responsible for the last trigger. The radio input never gets targeted by restoreControlledState.

It feels like there's something deeper here that we need to fix.

This comment has been minimized.

@landvibe

landvibe Nov 7, 2017
Author Contributor

I think updateNamedCousins is called only on the button because of this code ReactTestUtils.Simulate.click(buttonNode);
I checked that updateNamedCousins is not called when I run your fixture.

DOMPropertyOperations.setValueForProperty(
node,
'checked',
checked || false,

This comment has been minimized.

@nhunzaker

nhunzaker Nov 7, 2017
Collaborator

Kind of a bummer that we have to || false here. It can't hurt, but I wonder if this is necessary (just with a quick test via https://codepen.io/nhunzaker/pen/qVadLo).

This comment has been minimized.

@landvibe

landvibe Nov 7, 2017
Author Contributor

I also think it's unnecessary.

@@ -669,6 +669,45 @@ describe('ReactDOMInput', () => {
expect(cNode.checked).toBe(true);
});

it('should have correct checked value when radio names changed', () => {

This comment has been minimized.

@nhunzaker

nhunzaker Nov 7, 2017
Collaborator

Do you mind rephrasing this to should check the correct radio when the selected name moves?

This comment has been minimized.

@landvibe

landvibe Nov 7, 2017
Author Contributor

OK. yours is better

@landvibe
Copy link
Contributor Author

@landvibe landvibe commented Nov 7, 2017

@nhunzaker
I think updateChecked is more efficient than updateNamedCousins.
updateChecked just calls setValueForProperty once, but updateNamedCousins calls it group.length times per each updated node

new commit with your fixture

landvibe landvibe
targetType: ?string,
) {
var node = ((element: any): InputWithWrapperState);
if (!targetType || targetType === node.type) {

This comment has been minimized.

@nhunzaker

nhunzaker Nov 13, 2017
Collaborator

In updateNamedCousins, we check for radio buttons like:

function updateNamedCousins(rootNode, props) {
  var name = props.name;
  if (props.type === 'radio' && name != null) {
    // ...
  }
  // ...
}

Can you use that check here? That would allow us to remove the targetType argument and reading from the DOM.

This comment has been minimized.

@landvibe

landvibe Nov 13, 2017
Author Contributor

I added the code in updateProperties because updateWrapper also calls updateChecked

Copy link
Collaborator

@nhunzaker nhunzaker left a comment

Sorry for the delay. I had to spend some time figuring out what updateNamedCousins is actually doing. It looks like this is necessary for controlled radios so that you can programmatically prevent them from changing via state (like not toggling state in a change handler).

This looks good, I'd just like for the conditions for applying updateChecked to mirror that of updateNamedCousins, which I've laid out in the comments.

landvibe landvibe
@landvibe
Copy link
Contributor Author

@landvibe landvibe commented Nov 13, 2017

@nhunzaker applied your comment

Copy link
Collaborator

@nhunzaker nhunzaker left a comment

Looks good! I'm waiting on a build to do some final testing before merging. Thank you for sticking with me!

@nhunzaker
Copy link
Collaborator

@nhunzaker nhunzaker commented Nov 19, 2017

Verified in:

  • IE 9/10/11
  • Edge 14/15/16
  • Chrome 62, 49, 41
  • Safari 7.1, 8, 9.1, 10.1, 11
  • iOS 7, 8, 9, 10.2, 11.1
  • Chrome for Android (latest)
  • Firefox 56, 52 (ESR), 47 (Former ESR)
  • Opera 41, 49

😅 (When am I finally going to automate this...)

Thank you for sending this out. I apologize it took this long. This is a fantastic find!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.