From 999df3e7776247ddd78eda891c269196c6dd9992 Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Thu, 13 Jul 2017 16:02:31 -0400 Subject: [PATCH] Fix uncontrolled radios (#10156) * Add fixture * Fix Uncontrolled radio groups * address feedback * fix tests; prettier * Update TestCase.js --- fixtures/dom/public/react-loader.js | 5 +- fixtures/dom/src/components/TestCase.js | 17 +++++- .../input-change-events/RadioGroupFixture.js | 58 +++++++++++++++++++ .../fixtures/input-change-events/index.js | 19 ++++++ fixtures/dom/yarn.lock | 12 ++++ .../dom/fiber/ReactDOMFiberComponent.js | 4 ++ .../__tests__/inputValueTracking-test.js | 8 +-- .../dom/shared/inputValueTracking.js | 46 ++++++++------- .../dom/stack/client/ReactDOMComponent.js | 3 + 9 files changed, 140 insertions(+), 32 deletions(-) create mode 100644 fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js diff --git a/fixtures/dom/public/react-loader.js b/fixtures/dom/public/react-loader.js index 945905e38875c..083d6f6e96a96 100644 --- a/fixtures/dom/public/react-loader.js +++ b/fixtures/dom/public/react-loader.js @@ -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(''); diff --git a/fixtures/dom/src/components/TestCase.js b/fixtures/dom/src/components/TestCase.js index 84fa28c954668..e1e6321e269a8 100644 --- a/fixtures/dom/src/components/TestCase.js +++ b/fixtures/dom/src/components/TestCase.js @@ -9,6 +9,7 @@ const propTypes = { children: PropTypes.node.isRequired, title: PropTypes.node.isRequired, resolvedIn: semverString, + introducedIn: semverString, resolvedBy: PropTypes.string, }; @@ -31,6 +32,7 @@ class TestCase extends React.Component { const { title, description, + introducedIn, resolvedIn, resolvedBy, affectedBrowsers, @@ -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 (
@@ -60,6 +62,15 @@ class TestCase extends React.Component {
+ {introducedIn &&
First broken in:
} + {introducedIn && +
+ + {introducedIn} + +
} + {resolvedIn &&
First supported in:
} {resolvedIn &&
@@ -89,7 +100,7 @@ class TestCase extends React.Component {

- {!isTestRelevant && + {!isTestFixed &&

Note: {' '} diff --git a/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js b/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js new file mode 100644 index 0000000000000..8f4495976b9d4 --- /dev/null +++ b/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js @@ -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 ( + + + + + {' '} +

+ onChange{' calls: '}{changeCount} +

+ + + ); + } +} + +export default RadioGroupFixture; diff --git a/fixtures/dom/src/components/fixtures/input-change-events/index.js b/fixtures/dom/src/components/fixtures/input-change-events/index.js index 022c72254ba70..41920e5802403 100644 --- a/fixtures/dom/src/components/fixtures/input-change-events/index.js +++ b/fixtures/dom/src/components/fixtures/input-change-events/index.js @@ -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 { @@ -47,6 +48,24 @@ class InputChangeEvents extends React.Component { + + +
  • Click on the "Radio 2"
    • +
  • Click back to "Radio 1"
    • +     + + + The onChange call count should equal 2 + + + +     { 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); }); diff --git a/src/renderers/dom/shared/inputValueTracking.js b/src/renderers/dom/shared/inputValueTracking.js index 9f0d50ecd07a5..05b5b5de3659b 100644 --- a/src/renderers/dom/shared/inputValueTracking.js +++ b/src/renderers/dom/shared/inputValueTracking.js @@ -12,6 +12,7 @@ 'use strict'; +var {ELEMENT_NODE} = require('HTMLNodeType'); import type {Fiber} from 'ReactFiber'; import type {ReactInstance} from 'ReactInstanceType'; @@ -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'); @@ -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; @@ -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); + } + + var nextValue = getValueFromNode(node); if (nextValue !== lastValue) { tracker.setValue(nextValue); diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index bc9c5fa4e3a8c..9e87868d61d0b 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -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);