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

Fix uncontrolled radios #10156

Merged
merged 6 commits into from Jul 13, 2017
Merged

Fix uncontrolled radios #10156

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ce70f13
Add fixture
jquense Jul 12, 2017
25cff31
Fix Uncontrolled radio groups
jquense Jul 12, 2017
84bda97
address feedback
jquense Jul 12, 2017
48fc77f
Merge branch 'master' into fix-uncontrolled-radios
jquense Jul 12, 2017
15228e8
fix tests; prettier
jquense Jul 12, 2017
dede4b6
Update TestCase.js
jquense Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions fixtures/dom/public/react-loader.js
View file
Open in desktop
Expand Up @@ -28,9 +28,8 @@ var query = parseQuery(window.location.search);
var version = query.version || 'local';

if (version !== 'local') {
REACT_PATH = 'https://unpkg.com/react@' + version + '/dist/react.min.js';
DOM_PATH =
'https://unpkg.com/react-dom@' + version + '/dist/react-dom.min.js';
REACT_PATH = 'https://unpkg.com/react@' + version + '/dist/react.js';
DOM_PATH = 'https://unpkg.com/react-dom@' + version + '/dist/react-dom.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to the unminified versions intentionally so was easier to step through stuff in devtools

}

document.write('<script src="' + REACT_PATH + '"></script>');
Expand Down
15 changes: 13 additions & 2 deletions fixtures/dom/src/components/TestCase.js
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ const propTypes = {
children: PropTypes.node.isRequired,
title: PropTypes.node.isRequired,
resolvedIn: semverString,
introducedIn: semverString,
resolvedBy: PropTypes.string,
};

Expand All @@ -31,6 +32,7 @@ class TestCase extends React.Component {
const {
title,
description,
introducedIn,
resolvedIn,
resolvedBy,
affectedBrowsers,
Expand All @@ -40,10 +42,10 @@ class TestCase extends React.Component {
let {complete} = this.state;

const {version} = parse(window.location.search);
const isTestRelevant =
const isTestFixed =
!version || !resolvedIn || semver.gte(version, resolvedIn);

complete = !isTestRelevant || complete;
complete = !isTestFixed || complete;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing some local testing, looks like this causes a failure on line 103:
https://github.com/jquense/react/blob/15228e8b5f9d056d475955a54723cd906ffa5619/fixtures/dom/src/components/TestCase.js#L103

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return (
<section className={cn('test-case', complete && 'test-case--complete')}>
Expand All @@ -60,6 +62,15 @@ class TestCase extends React.Component {
</h2>

<dl className="test-case__details">
{introducedIn && <dt>First broken in: </dt>}
{introducedIn &&
<dd>
<a
href={'https://github.com/facebook/react/tag/v' + introducedIn}>
<code>{introducedIn}</code>
</a>
</dd>}

{resolvedIn && <dt>First supported in: </dt>}
{resolvedIn &&
<dd>
Expand Down
58 changes: 58 additions & 0 deletions fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js
View file
Open in desktop
@@ -0,0 +1,58 @@
import React from 'react';

import Fixture from '../../Fixture';

class RadioGroupFixture extends React.Component {
constructor(props, context) {
super(props, context);

this.state = {
changeCount: 0,
};
}

handleChange = () => {
this.setState(({changeCount}) => {
return {
changeCount: changeCount + 1,
};
});
};

handleReset = () => {
this.setState({
changeCount: 0,
});
};

render() {
const {changeCount} = this.state;
const color = changeCount === 2 ? 'green' : 'red';

return (
<Fixture>
<label>
<input
defaultChecked
name="foo"
type="radio"
onChange={this.handleChange}
/>
Radio 1
</label>
<label>
<input name="foo" type="radio" onChange={this.handleChange} />
Radio 2
</label>

{' '}
<p style={{color}}>
<code>onChange</code>{' calls: '}<strong>{changeCount}</strong>
</p>
<button onClick={this.handleReset}>Reset count</button>
</Fixture>
);
}
}

export default RadioGroupFixture;
19 changes: 19 additions & 0 deletions fixtures/dom/src/components/fixtures/input-change-events/index.js
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';
import RangeKeyboardFixture from './RangeKeyboardFixture';
import RadioClickFixture from './RadioClickFixture';
import RadioGroupFixture from './RadioGroupFixture';
import InputPlaceholderFixture from './InputPlaceholderFixture';

class InputChangeEvents extends React.Component {
Expand Down Expand Up @@ -47,6 +48,24 @@ class InputChangeEvents extends React.Component {

<RadioClickFixture />
</TestCase>
<TestCase
title="Uncontrolled radio groups"
description={`
Radio inputs should fire change events when the value moved to
another named input
`}
introducedIn="15.6.0">
<TestCase.Steps>
<li>Click on the "Radio 2"</li>
<li>Click back to "Radio 1"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The <code>onChange</code> call count should equal 2
</TestCase.ExpectedResult>

<RadioGroupFixture />
</TestCase>

<TestCase
title="Inputs with placeholders"
Expand Down
12 changes: 12 additions & 0 deletions fixtures/dom/yarn.lock
View file
Open in desktop
Expand Up @@ -2199,6 +2199,18 @@ fbjs@^0.8.1, fbjs@^0.8.4:
setimmediate "^1.0.5"
ua-parser-js "^0.7.9"

fbjs@^0.8.9:
version "0.8.12"
resolved "https://registry.yarnpkg.com/fbjs/-/fbjs-0.8.12.tgz#10b5d92f76d45575fd63a217d4ea02bea2f8ed04"
dependencies:
core-js "^1.0.0"
isomorphic-fetch "^2.1.1"
loose-envify "^1.0.0"
object-assign "^4.1.0"
promise "^7.1.1"
setimmediate "^1.0.5"
ua-parser-js "^0.7.9"

figures@^1.3.5:
version "1.7.0"
resolved "https://registry.yarnpkg.com/figures/-/figures-1.7.0.tgz#cbe1e3affcf1cd44b80cadfed28dc793a9701d2e"
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Expand Up @@ -754,6 +754,10 @@ var ReactDOMFiberComponent = {
// happen after `updateDOMProperties`. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
ReactDOMFiberInput.updateWrapper(domElement, nextRawProps);

// 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((domElement: any));
break;
case 'textarea':
ReactDOMFiberTextarea.updateWrapper(domElement, nextRawProps);
Expand Down
8 changes: 2 additions & 6 deletions src/renderers/dom/shared/__tests__/inputValueTracking-test.js
View file
Open in desktop
Expand Up @@ -145,15 +145,11 @@ describe('inputValueTracking', () => {
it('should stop tracking', () => {
inputValueTracking.track(mockComponent);

expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
true,
);
expect(mockComponent._wrapperState.valueTracker).not.toEqual(null);

inputValueTracking.stopTracking(mockComponent);

expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
false,
);
expect(mockComponent._wrapperState.valueTracker).toEqual(null);

expect(input.hasOwnProperty('value')).toBe(false);
});
Expand Down
46 changes: 26 additions & 20 deletions src/renderers/dom/shared/inputValueTracking.js
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@

'use strict';

var {ELEMENT_NODE} = require('HTMLNodeType');
import type {Fiber} from 'ReactFiber';
import type {ReactInstance} from 'ReactInstanceType';

Expand All @@ -23,6 +24,9 @@ type ValueTracker = {
type WrapperState = {_wrapperState: {valueTracker: ?ValueTracker}};
type ElementWithWrapperState = Element & WrapperState;
type InstanceWithWrapperState = ReactInstance & WrapperState;
type SubjectWithWrapperState =
| InstanceWithWrapperState
| ElementWithWrapperState;

var ReactDOMComponentTree = require('ReactDOMComponentTree');

Expand All @@ -43,15 +47,11 @@ function getTracker(inst: any) {
return inst._wrapperState.valueTracker;
}

function attachTracker(inst: InstanceWithWrapperState, tracker: ?ValueTracker) {
inst._wrapperState.valueTracker = tracker;
function detachTracker(subject: SubjectWithWrapperState) {
subject._wrapperState.valueTracker = null;
}

function detachTracker(inst: InstanceWithWrapperState) {
delete inst._wrapperState.valueTracker;
}

function getValueFromNode(node) {
function getValueFromNode(node: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var value;
if (node) {
value = isCheckable(node) ? '' + node.checked : node.value;
Expand Down Expand Up @@ -113,40 +113,46 @@ var inputValueTracking = {
return getTracker(ReactDOMComponentTree.getInstanceFromNode(node));
},

trackNode: function(node: ElementWithWrapperState) {
if (node._wrapperState.valueTracker) {
trackNode(node: ElementWithWrapperState) {
if (getTracker(node)) {
return;
}
node._wrapperState.valueTracker = trackValueOnNode(node, node);
},

track: function(inst: InstanceWithWrapperState) {
track(inst: InstanceWithWrapperState) {
if (getTracker(inst)) {
return;
}
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo I think I understand where the bug is

attachTracker(inst, trackValueOnNode(node, inst));
inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
},

updateValueIfChanged(inst: InstanceWithWrapperState | Fiber) {
if (!inst) {
updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jquense jquense Jul 17, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check that, thanks.

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

if (!tracker) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (typeof (inst: any).tag === 'number') {
inputValueTracking.trackNode((inst: any).stateNode);
if (typeof (subject: any).tag === 'number') {
inputValueTracking.trackNode((subject: any).stateNode);
} else {
inputValueTracking.track((inst: any));
inputValueTracking.track((subject: any));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 true;
}

var lastValue = tracker.getValue();
var nextValue = getValueFromNode(
ReactDOMComponentTree.getNodeFromInstance(inst),
);

var node = subject;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm no I didn’t misread 😛

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

var nextValue = getValueFromNode(node);

if (nextValue !== lastValue) {
tracker.setValue(nextValue);
Expand Down
3 changes: 3 additions & 0 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
View file
Open in desktop
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix

break;
case 'textarea':
ReactDOMTextarea.updateWrapper(this);
Expand Down