Fix uncontrolled radios (#10156)
* Add fixture * Fix Uncontrolled radio groups * address feedback * fix tests; prettier * Update TestCase.js
Jason Quense committed
Jul 13, 2017 at 16:02 UTC
999df3e7776247ddd78eda891c269196c6dd9992
9 files changed
+140
-32
fixtures/dom/public/react-loader.js
+2
-3
@@ -28,9 +28,8 @@ var query = parseQuery(window.location.search);
28
var version = query.version || 'local';
29
30
if (version !== 'local') {
31
- REACT_PATH = 'https://unpkg.com/react@' + version + '/dist/react.min.js';
32
- DOM_PATH =
33
- 'https://unpkg.com/react-dom@' + version + '/dist/react-dom.min.js';
31
+ REACT_PATH = 'https://unpkg.com/react@' + version + '/dist/react.js';
32
+ DOM_PATH = 'https://unpkg.com/react-dom@' + version + '/dist/react-dom.js';
33
}
34
35
document.write('<script src="' + REACT_PATH + '"></script>');
fixtures/dom/src/components/TestCase.js
+14
-3
@@ -9,6 +9,7 @@ const propTypes = {
9
children: PropTypes.node.isRequired,
10
title: PropTypes.node.isRequired,
11
resolvedIn: semverString,
12
+ introducedIn: semverString,
13
resolvedBy: PropTypes.string,
14
};
15
@@ -31,6 +32,7 @@ class TestCase extends React.Component {
32
const {
33
title,
34
description,
35
+ introducedIn,
36
resolvedIn,
37
resolvedBy,
38
affectedBrowsers,
@@ -40,10 +42,10 @@ class TestCase extends React.Component {
42
let {complete} = this.state;
43
44
const {version} = parse(window.location.search);
43
- const isTestRelevant =
45
+ const isTestFixed =
46
!version || !resolvedIn || semver.gte(version, resolvedIn);
47
46
- complete = !isTestRelevant || complete;
48
+ complete = !isTestFixed || complete;
49
50
return (
51
<section className={cn('test-case', complete && 'test-case--complete')}>
@@ -60,6 +62,15 @@ class TestCase extends React.Component {
62
</h2>
63
64
<dl className="test-case__details">
65
+ {introducedIn && <dt>First broken in: </dt>}
66
+ {introducedIn &&
67
+ <dd>
68
+ <a
69
+ href={'https://github.com/facebook/react/tag/v' + introducedIn}>
70
+ <code>{introducedIn}</code>
71
+ </a>
72
+ </dd>}
73
+
74
{resolvedIn && <dt>First supported in: </dt>}
75
{resolvedIn &&
76
<dd>
@@ -89,7 +100,7 @@ class TestCase extends React.Component {
100
</p>
101
102
<div className="test-case__body">
92
- {!isTestRelevant &&
103
+ {!isTestFixed &&
104
<p className="test-case__invalid-version">
105
<strong>Note:</strong>
106
{' '}
fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js
new
+58
@@ -0,0 +1,58 @@
1
+import React from 'react';
2
+
3
+import Fixture from '../../Fixture';
4
+
5
+class RadioGroupFixture extends React.Component {
6
+ constructor(props, context) {
7
+ super(props, context);
8
+
9
+ this.state = {
10
+ changeCount: 0,
11
+ };
12
+ }
13
+
14
+ handleChange = () => {
15
+ this.setState(({changeCount}) => {
16
+ return {
17
+ changeCount: changeCount + 1,
18
+ };
19
+ });
20
+ };
21
+
22
+ handleReset = () => {
23
+ this.setState({
24
+ changeCount: 0,
25
+ });
26
+ };
27
+
28
+ render() {
29
+ const {changeCount} = this.state;
30
+ const color = changeCount === 2 ? 'green' : 'red';
31
+
32
+ return (
33
+ <Fixture>
34
+ <label>
35
+ <input
36
+ defaultChecked
37
+ name="foo"
38
+ type="radio"
39
+ onChange={this.handleChange}
40
+ />
41
+ Radio 1
42
+ </label>
43
+ <label>
44
+ <input name="foo" type="radio" onChange={this.handleChange} />
45
+ Radio 2
46
+ </label>
47
+
48
+ {' '}
49
+ <p style={{color}}>
50
+ <code>onChange</code>{' calls: '}<strong>{changeCount}</strong>
51
+ </p>
52
+ <button onClick={this.handleReset}>Reset count</button>
53
+ </Fixture>
54
+ );
55
+ }
56
+}
57
+
58
+export default RadioGroupFixture;
fixtures/dom/src/components/fixtures/input-change-events/index.js
+19
@@ -4,6 +4,7 @@ import FixtureSet from '../../FixtureSet';
4
import TestCase from '../../TestCase';
5
import RangeKeyboardFixture from './RangeKeyboardFixture';
6
import RadioClickFixture from './RadioClickFixture';
7
+import RadioGroupFixture from './RadioGroupFixture';
8
import InputPlaceholderFixture from './InputPlaceholderFixture';
9
10
class InputChangeEvents extends React.Component {
@@ -47,6 +48,24 @@ class InputChangeEvents extends React.Component {
48
49
<RadioClickFixture />
50
</TestCase>
51
+ <TestCase
52
+ title="Uncontrolled radio groups"
53
+ description={`
54
+ Radio inputs should fire change events when the value moved to
55
+ another named input
56
+ `}
57
+ introducedIn="15.6.0">
58
+ <TestCase.Steps>
59
+ <li>Click on the "Radio 2"</li>
60
+ <li>Click back to "Radio 1"</li>
61
+ </TestCase.Steps>
62
+
63
+ <TestCase.ExpectedResult>
64
+ The <code>onChange</code> call count should equal 2
65
+ </TestCase.ExpectedResult>
66
+
67
+ <RadioGroupFixture />
68
+ </TestCase>
69
70
<TestCase
71
title="Inputs with placeholders"
fixtures/dom/yarn.lock
+12
@@ -2199,6 +2199,18 @@ fbjs@^0.8.1, fbjs@^0.8.4:
2199
setimmediate "^1.0.5"
2200
ua-parser-js "^0.7.9"
2201
2202
+fbjs@^0.8.9:
2203
+ version "0.8.12"
2204
+ resolved "https://registry.yarnpkg.com/fbjs/-/fbjs-0.8.12.tgz#10b5d92f76d45575fd63a217d4ea02bea2f8ed04"
2205
+ dependencies:
2206
+ core-js "^1.0.0"
2207
+ isomorphic-fetch "^2.1.1"
2208
+ loose-envify "^1.0.0"
2209
+ object-assign "^4.1.0"
2210
+ promise "^7.1.1"
2211
+ setimmediate "^1.0.5"
2212
+ ua-parser-js "^0.7.9"
2213
+
2214
figures@^1.3.5:
2215
version "1.7.0"
2216
resolved "https://registry.yarnpkg.com/figures/-/figures-1.7.0.tgz#cbe1e3affcf1cd44b80cadfed28dc793a9701d2e"
src/renderers/dom/fiber/ReactDOMFiberComponent.js
+4
@@ -754,6 +754,10 @@ var ReactDOMFiberComponent = {
754
// happen after `updateDOMProperties`. Otherwise HTML5 input validations
755
// raise warnings and prevent the new value from being assigned.
756
ReactDOMFiberInput.updateWrapper(domElement, nextRawProps);
757
+
758
+ // We also check that we haven't missed a value update, such as a
759
+ // Radio group shifting the checked value to another named radio input.
760
+ inputValueTracking.updateValueIfChanged((domElement: any));
761
break;
762
case 'textarea':
763
ReactDOMFiberTextarea.updateWrapper(domElement, nextRawProps);
src/renderers/dom/shared/__tests__/inputValueTracking-test.js
+2
-6
@@ -145,15 +145,11 @@ describe('inputValueTracking', () => {
145
it('should stop tracking', () => {
146
inputValueTracking.track(mockComponent);
147
148
- expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
149
- true,
150
- );
148
+ expect(mockComponent._wrapperState.valueTracker).not.toEqual(null);
149
150
inputValueTracking.stopTracking(mockComponent);
151
154
- expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
155
- false,
156
- );
152
+ expect(mockComponent._wrapperState.valueTracker).toEqual(null);
153
154
expect(input.hasOwnProperty('value')).toBe(false);
155
});
src/renderers/dom/shared/inputValueTracking.js
+26
-20
@@ -12,6 +12,7 @@
12
13
'use strict';
14
15
+var {ELEMENT_NODE} = require('HTMLNodeType');
16
import type {Fiber} from 'ReactFiber';
17
import type {ReactInstance} from 'ReactInstanceType';
18
@@ -23,6 +24,9 @@ type ValueTracker = {
24
type WrapperState = {_wrapperState: {valueTracker: ?ValueTracker}};
25
type ElementWithWrapperState = Element & WrapperState;
26
type InstanceWithWrapperState = ReactInstance & WrapperState;
27
+type SubjectWithWrapperState =
28
+ | InstanceWithWrapperState
29
+ | ElementWithWrapperState;
30
31
var ReactDOMComponentTree = require('ReactDOMComponentTree');
32
@@ -43,15 +47,11 @@ function getTracker(inst: any) {
47
return inst._wrapperState.valueTracker;
48
}
49
46
-function attachTracker(inst: InstanceWithWrapperState, tracker: ?ValueTracker) {
47
- inst._wrapperState.valueTracker = tracker;
50
+function detachTracker(subject: SubjectWithWrapperState) {
51
+ subject._wrapperState.valueTracker = null;
52
}
53
50
-function detachTracker(inst: InstanceWithWrapperState) {
51
- delete inst._wrapperState.valueTracker;
52
-}
53
-
54
-function getValueFromNode(node) {
54
+function getValueFromNode(node: any) {
55
var value;
56
if (node) {
57
value = isCheckable(node) ? '' + node.checked : node.value;
@@ -113,40 +113,46 @@ var inputValueTracking = {
113
return getTracker(ReactDOMComponentTree.getInstanceFromNode(node));
114
},
115
116
- trackNode: function(node: ElementWithWrapperState) {
117
- if (node._wrapperState.valueTracker) {
116
+ trackNode(node: ElementWithWrapperState) {
117
+ if (getTracker(node)) {
118
return;
119
}
120
node._wrapperState.valueTracker = trackValueOnNode(node, node);
121
},
122
123
- track: function(inst: InstanceWithWrapperState) {
123
+ track(inst: InstanceWithWrapperState) {
124
if (getTracker(inst)) {
125
return;
126
}
127
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
128
- attachTracker(inst, trackValueOnNode(node, inst));
128
+ inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
129
},
130
131
- updateValueIfChanged(inst: InstanceWithWrapperState | Fiber) {
132
- if (!inst) {
131
+ updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
132
+ if (!subject) {
133
return false;
134
}
135
- var tracker = getTracker(inst);
135
+ var tracker = getTracker(subject);
136
137
if (!tracker) {
138
- if (typeof (inst: any).tag === 'number') {
139
- inputValueTracking.trackNode((inst: any).stateNode);
138
+ if (typeof (subject: any).tag === 'number') {
139
+ inputValueTracking.trackNode((subject: any).stateNode);
140
} else {
141
- inputValueTracking.track((inst: any));
141
+ inputValueTracking.track((subject: any));
142
}
143
return true;
144
}
145
146
var lastValue = tracker.getValue();
147
- var nextValue = getValueFromNode(
148
- ReactDOMComponentTree.getNodeFromInstance(inst),
149
- );
147
+
148
+ var node = subject;
149
+
150
+ // TODO: remove check when the Stack renderer is retired
151
+ if ((subject: any).nodeType !== ELEMENT_NODE) {
152
+ node = ReactDOMComponentTree.getNodeFromInstance(subject);
153
+ }
154
+
155
+ var nextValue = getValueFromNode(node);
156
157
if (nextValue !== lastValue) {
158
tracker.setValue(nextValue);
src/renderers/dom/stack/client/ReactDOMComponent.js
+3
@@ -860,6 +860,9 @@ ReactDOMComponent.Mixin = {
860
// happen after `_updateDOMProperties`. Otherwise HTML5 input validations
861
// raise warnings and prevent the new value from being assigned.
862
ReactDOMInput.updateWrapper(this);
863
+ // We also check that we haven't missed a value update, such as a
864
+ // Radio group shifting the checked value to another named radio input.
865
+ inputValueTracking.updateValueIfChanged(this);
866
break;
867
case 'textarea':
868
ReactDOMTextarea.updateWrapper(this);