Skip to content

Commit

Permalink
Fix uncontrolled radios (#10156)
Browse files Browse the repository at this point in the history
* Add fixture

* Fix Uncontrolled radio groups

* address feedback

* fix tests; prettier

* Update TestCase.js
  • Loading branch information
jquense authored and nhunzaker committed Jul 13, 2017
1 parent 2dcdc3c commit 999df3e
Show file tree
Hide file tree
Showing 9 changed files with 140 additions and 32 deletions.
5 changes: 2 additions & 3 deletions fixtures/dom/public/react-loader.js
Original file line number Diff line number Diff line change
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';
}

document.write('<script src="' + REACT_PATH + '"></script>');
Expand Down
17 changes: 14 additions & 3 deletions fixtures/dom/src/components/TestCase.js
Original file line number Diff line number Diff line change
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;

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 Expand Up @@ -89,7 +100,7 @@ class TestCase extends React.Component {
</p>

<div className="test-case__body">
{!isTestRelevant &&
{!isTestFixed &&
<p className="test-case__invalid-version">
<strong>Note:</strong>
{' '}
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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) {
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);
attachTracker(inst, trackValueOnNode(node, inst));
inst._wrapperState.valueTracker = trackValueOnNode(node, inst);
},

updateValueIfChanged(inst: InstanceWithWrapperState | Fiber) {
if (!inst) {
updateValueIfChanged(subject: SubjectWithWrapperState | Fiber) {
if (!subject) {
return false;
}
var tracker = getTracker(inst);
var tracker = getTracker(subject);

if (!tracker) {
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));
}
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);

This comment has been minimized.

Copy link
@gaearon

gaearon Jul 15, 2017

Collaborator

This seems to be causing issues in production for us. This function throws with invalid argument. Do you have any thoughts on how this could happen?

This comment has been minimized.

Copy link
@trueadm

trueadm Jul 15, 2017

Contributor

This could be because we're going into this function now from a new route, 999df3e#diff-9756d35a2f408667aa5e1cfb065b4fdcR760. Which could be why we're now seeing this happen.

This comment has been minimized.

Copy link
@jquense

jquense Jul 15, 2017

Author Contributor

Is there any chance the value here is some sort of DOM node that isn't an ELEMENT_NODE ? Apart from that i'm not sure how you could get something off here?

}

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
Original file line number Diff line number Diff line change
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);
break;
case 'textarea':
ReactDOMTextarea.updateWrapper(this);
Expand Down

0 comments on commit 999df3e

Please sign in to comment.