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

Fix uncontrolled radios #10156

Merged
merged 6 commits into from Jul 13, 2017

Conversation

Projects
None yet
4 participants
@jquense
Collaborator

jquense commented Jul 12, 2017

Fixes #9988, this is against master but we should cherry-pick the change back to 15.6 for a patch release.

I added a fixture instead of a test because, writing a unit test that reproduced the issue was confusingly hard actually.

Most of the extra bits here are to handle both fiber and stack, the content of the PR is really the one line in DOMComponent.

@@ -860,6 +860,9 @@ ReactDOMComponent.Mixin = {
// happen after `_updateDOMProperties`. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
ReactDOMInput.updateWrapper(this);
// We also check that we haven't missed a value update, such as a
// Radio group shifting the checked value to another named radio input.
inputValueTracking.updateValueIfChanged(this);

This comment has been minimized.

@jquense

jquense Jul 12, 2017

Collaborator

This is the actual fix

@jquense

jquense Jul 12, 2017

Collaborator

This is the actual fix

@aweary aweary referenced this pull request Jul 12, 2017

Closed

React 15.6.2 Umbrella #10040

6 of 12 tasks complete
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 12, 2017

Member

Looks like Stack tests are failing

 FAIL  src/renderers/dom/shared/__tests__/inputValueTracking-test.js
  ● inputValueTracking › should stop tracking

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      false
    Received:
      true

Also prettier is unhappy.

Member

gaearon commented Jul 12, 2017

Looks like Stack tests are failing

 FAIL  src/renderers/dom/shared/__tests__/inputValueTracking-test.js
  ● inputValueTracking › should stop tracking

    expect(received).toBe(expected)
    
    Expected value to be (using ===):
      false
    Received:
      true

Also prettier is unhappy.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 12, 2017

Collaborator

not sure what the deal with prettier is, i ran it locally. lemme mess around with it. Do I have to do something special to run tests against the stack renderer?

Collaborator

jquense commented Jul 12, 2017

not sure what the deal with prettier is, i ran it locally. lemme mess around with it. Do I have to do something special to run tests against the stack renderer?

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 12, 2017

Collaborator

o nvm on the failing test, that's my bad. sorry

Collaborator

jquense commented Jul 12, 2017

o nvm on the failing test, that's my bad. sorry

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 12, 2017

Collaborator

@jquense This looks great from my end. I'd be happy to take on some manual browser testing. What should I test for?

Collaborator

nhunzaker commented Jul 12, 2017

@jquense This looks great from my end. I'd be happy to take on some manual browser testing. What should I test for?

@nhunzaker

Aside from the reference error (covered by lint), this checks out in:

  • IE11
  • IE15
  • Chrome 59
  • Safari 10
  • Firefox 54
!version || !resolvedIn || semver.gte(version, resolvedIn);
complete = !isTestRelevant || complete;
complete = !isTestFixed || complete;

This comment has been minimized.

@nhunzaker
@nhunzaker

nhunzaker Jul 13, 2017

Collaborator

This comment has been minimized.

@jquense

jquense Jul 13, 2017

Collaborator

Last time I do merge conflict resolve on github :P man.

@jquense

jquense Jul 13, 2017

Collaborator

Last time I do merge conflict resolve on github :P man.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 13, 2017

Collaborator

Here's a test build with the lint fix:
http://react-fix-uncontrolled-radios.surge.sh/

Collaborator

nhunzaker commented Jul 13, 2017

Here's a test build with the lint fix:
http://react-fix-uncontrolled-radios.surge.sh/

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 13, 2017

Collaborator

I don't think this change could change behavior for controlled inputs but technically it's doing slightly more work on every input update. I did run the various fixtures for input changes and didn't see anything but just noting it for posterity.

Collaborator

jquense commented Jul 13, 2017

I don't think this change could change behavior for controlled inputs but technically it's doing slightly more work on every input update. I did run the various fixtures for input changes and didn't see anything but just noting it for posterity.

@nhunzaker nhunzaker merged commit 999df3e into facebook:master Jul 13, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 13, 2017

Collaborator

Checks out for me. @jquense Could you handle bringing this over to 15.x?

Collaborator

nhunzaker commented Jul 13, 2017

Checks out for me. @jquense Could you handle bringing this over to 15.x?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 13, 2017

Member

AFAIK @flarnie's not working on 15.6.x right now. Do you want to handle this release? Would be your first release :-)

Member

gaearon commented Jul 13, 2017

AFAIK @flarnie's not working on 15.6.x right now. Do you want to handle this release? Would be your first release :-)

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 13, 2017

Collaborator

Well if you put it that way.

Sure

Collaborator

nhunzaker commented Jul 13, 2017

Well if you put it that way.

Sure

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 14, 2017

Member

It's yours! Look at what we did for previous releases: merge things to a branch freshly based of 15-stable, let me know when it's ready and we can get it out.

Member

gaearon commented Jul 14, 2017

It's yours! Look at what we did for previous releases: merge things to a branch freshly based of 15-stable, let me know when it's ready and we can get it out.

}
function getValueFromNode(node) {
function getValueFromNode(node: any) {

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

In general we should avoid adding any wherever possible. It very often leads to bugs. I know we used it sometimes, but I just want to point out for future reviews that it should be used only as last measure.

(I don’t mean this particular case is problematic, but this is something to always keep in mind.)

@gaearon

gaearon Jul 16, 2017

Member

In general we should avoid adding any wherever possible. It very often leads to bugs. I know we used it sometimes, but I just want to point out for future reviews that it should be used only as last measure.

(I don’t mean this particular case is problematic, but this is something to always keep in mind.)

},
updateValueIfChanged(inst: InstanceWithWrapperState | Fiber) {
if (!inst) {
updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

Was there any specific reason for changing terminology here? Did inputs change? Or was existing terminology inconsistent? In Stack, we used instance for internal instance, but in Fiber we use it for abstract renderer-specific instance (such as DOM node in case of ReactDOM). So both seemed fitting to me.

@gaearon

gaearon Jul 16, 2017

Member

Was there any specific reason for changing terminology here? Did inputs change? Or was existing terminology inconsistent? In Stack, we used instance for internal instance, but in Fiber we use it for abstract renderer-specific instance (such as DOM node in case of ReactDOM). So both seemed fitting to me.

This comment has been minimized.

@jquense

jquense Jul 16, 2017

Collaborator

The inputs changed as far as I can know. The only place this was called otherwise was in the ChangeEventPlugin where it's just called an "instance" It may very well be a DOM node there in Fiber (now that I think of it). The need for the logic branch here is that we need the actual DOM node, and to handle both renderers that means calling getNodeFromInstance which seems to be fairly unforgiving of you passing in a node already, which I think is somerhow what's happening

As for the terminology it was more to save line space instead of writing InstanceWithWrapperState | ElementWithWrapperState in a few places.

@jquense

jquense Jul 16, 2017

Collaborator

The inputs changed as far as I can know. The only place this was called otherwise was in the ChangeEventPlugin where it's just called an "instance" It may very well be a DOM node there in Fiber (now that I think of it). The need for the logic branch here is that we need the actual DOM node, and to handle both renderers that means calling getNodeFromInstance which seems to be fairly unforgiving of you passing in a node already, which I think is somerhow what's happening

As for the terminology it was more to save line space instead of writing InstanceWithWrapperState | ElementWithWrapperState in a few places.

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

Do I understand correctly that now the code supports passing three different types? Stack instance, Fiber, and a DOM node.

@gaearon

gaearon Jul 16, 2017

Member

Do I understand correctly that now the code supports passing three different types? Stack instance, Fiber, and a DOM node.

This comment has been minimized.

@jquense

jquense Jul 17, 2017

Collaborator

TBH I don't know what the Fiber type is/does here. I didn't add the types originally and AFAICT its never passed a Fiber, only an instance or Dom node

@jquense

jquense Jul 17, 2017

Collaborator

TBH I don't know what the Fiber type is/does here. I didn't add the types originally and AFAICT its never passed a Fiber, only an instance or Dom node

This comment has been minimized.

@gaearon

gaearon Jul 17, 2017

Member

Hmm. I was under impression it was always passed a Fiber or a Stack instance before this change. Since .tag check is for detecting Fibers.

@gaearon

gaearon Jul 17, 2017

Member

Hmm. I was under impression it was always passed a Fiber or a Stack instance before this change. Since .tag check is for detecting Fibers.

This comment has been minimized.

@jquense

jquense Jul 17, 2017

Collaborator

That may be! I still unfamiliar with the Fiber data types but the only place this is called is is ChangeEventPlugin with the targetInst parameter and DOMComponent. I'm unsure what the case is in the changeEventPlugin.

Is the Fiber situation that sometimes it may be an "instance" (DOM Node) and sometimes it may be a "Fiber". In the Stack case it's always an internal Instance

@jquense

jquense Jul 17, 2017

Collaborator

That may be! I still unfamiliar with the Fiber data types but the only place this is called is is ChangeEventPlugin with the targetInst parameter and DOMComponent. I'm unsure what the case is in the changeEventPlugin.

Is the Fiber situation that sometimes it may be an "instance" (DOM Node) and sometimes it may be a "Fiber". In the Stack case it's always an internal Instance

This comment has been minimized.

@gaearon

gaearon Jul 17, 2017

Member

I'll check that, thanks.

@gaearon

gaearon Jul 17, 2017

Member

I'll check that, thanks.

// TODO: remove check when the Stack renderer is retired
if ((subject: any).nodeType !== ELEMENT_NODE) {
node = ReactDOMComponentTree.getNodeFromInstance(subject);

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

Could you explain more about why this change is necessary? I still don’t quite get why it was called unconditionally, but now is called conditionally, even though subject (aka inst) type has not changed (or has it?)

@gaearon

gaearon Jul 16, 2017

Member

Could you explain more about why this change is necessary? I still don’t quite get why it was called unconditionally, but now is called conditionally, even though subject (aka inst) type has not changed (or has it?)

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

Oops, I misread. I now see that previous code also had (a different) check. I wonder if changing that check is what caused the issue. I'm still not sure why though.

@gaearon

gaearon Jul 16, 2017

Member

Oops, I misread. I now see that previous code also had (a different) check. I wonder if changing that check is what caused the issue. I'm still not sure why though.

This comment has been minimized.

@gaearon

gaearon Jul 16, 2017

Member

Hmm no I didn’t misread 😛

This does look like a new check. So my previous comment still stands.

@gaearon

gaearon Jul 16, 2017

Member

Hmm no I didn’t misread 😛

This does look like a new check. So my previous comment still stands.

This comment has been minimized.

@jquense

jquense Jul 16, 2017

Collaborator

more in the comment above, but the check is purely to avoid calling getNodeFromInstance when inst is already a Node, as in the Fiber case. The only reason it was added was to handle both renderers, You can see in backport PR that this file isn't even touched (i'm guessing that one probably doesn't suffer this bug btw)

@jquense

jquense Jul 16, 2017

Collaborator

more in the comment above, but the check is purely to avoid calling getNodeFromInstance when inst is already a Node, as in the Fiber case. The only reason it was added was to handle both renderers, You can see in backport PR that this file isn't even touched (i'm guessing that one probably doesn't suffer this bug btw)

return;
}
node._wrapperState.valueTracker = trackValueOnNode(node, node);
},
track: function(inst: InstanceWithWrapperState) {
track(inst: InstanceWithWrapperState) {
if (getTracker(inst)) {
return;
}
var node = ReactDOMComponentTree.getNodeFromInstance(inst);

This comment has been minimized.

@gaearon

gaearon Jul 17, 2017

Member

I looked at the stacktrace and it's crashing here (rather than in the other place I expected).

@gaearon

gaearon Jul 17, 2017

Member

I looked at the stacktrace and it's crashing here (rather than in the other place I expected).

This comment has been minimized.

@jquense

jquense Jul 17, 2017

Collaborator

ooo I think I understand where the bug is

@jquense

jquense Jul 17, 2017

Collaborator

ooo I think I understand where the bug is

} else {
inputValueTracking.track((inst: any));
inputValueTracking.track((subject: any));

This comment has been minimized.

@gaearon

gaearon Jul 17, 2017

Member

This branching looks odd to me. It seems like it tries to branch on Fiber and Stack code. However now subject could be a DOM node itself. In this case it will go to track() rather than trackNode() which is why it breaks.

I think this shows why it might be better to reduce the polymorphism here and maybe change this to only accept DOM nodes. But I'd also like to understand why tests (including fixtures) didn't catch this. It is not quite obvious to me.

@gaearon

gaearon Jul 17, 2017

Member

This branching looks odd to me. It seems like it tries to branch on Fiber and Stack code. However now subject could be a DOM node itself. In this case it will go to track() rather than trackNode() which is why it breaks.

I think this shows why it might be better to reduce the polymorphism here and maybe change this to only accept DOM nodes. But I'd also like to understand why tests (including fixtures) didn't catch this. It is not quite obvious to me.

return false;
}
var tracker = getTracker(inst);
var tracker = getTracker(subject);
if (!tracker) {

This comment has been minimized.

@gaearon

gaearon Jul 18, 2017

Member

I'm having trouble understanding how to hit this code path. In which case do we not have a tracker already? I assumed we always have it since we create it during mount.

@gaearon

gaearon Jul 18, 2017

Member

I'm having trouble understanding how to hit this code path. In which case do we not have a tracker already? I assumed we always have it since we create it during mount.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon
Member

gaearon commented Jul 18, 2017

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