From ce70f13629f8044b7df42c728095f514bf02dba3 Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Wed, 12 Jul 2017 11:50:38 -0400 Subject: [PATCH 1/5] Add fixture --- fixtures/dom/public/react-loader.js | 4 +- fixtures/dom/src/components/TestCase.js | 20 ++++-- .../input-change-events/RadioGroupFixture.js | 62 +++++++++++++++++++ .../fixtures/input-change-events/index.js | 20 ++++++ fixtures/dom/yarn.lock | 12 ++++ 5 files changed, 112 insertions(+), 6 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 af8fe8874e7b3..083d6f6e96a96 100644 --- a/fixtures/dom/public/react-loader.js +++ b/fixtures/dom/public/react-loader.js @@ -28,8 +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 053eac9467884..167fa6eadb276 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,13 +42,13 @@ 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 (
+ {introducedIn && ( +
First broken in:
)} + {introducedIn && ( +
+ + {introducedIn} + +
+ )} + {resolvedIn && (
First supported in:
)} {resolvedIn && ( @@ -100,12 +112,12 @@ class TestCase extends React.Component {

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

Note: This test case was fixed in a later version of React. This test is not expected to pass for the selected version, and that's ok!

- )} + )} {children}
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..3ec6cf1fd3a4a --- /dev/null +++ b/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js @@ -0,0 +1,62 @@ +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 703585ed43795..9637dce0b3368 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 { @@ -50,6 +51,25 @@ class InputChangeEvents extends React.Component { + + +
  • Click on the "Radio 2"
    • +
  • Click back to "Radio 1"
    • +     + + + The onChange call count should equal 2 + + + +     Date: Wed, 12 Jul 2017 11:50:55 -0400 Subject: [PATCH 2/5] Fix Uncontrolled radio groups --- .../dom/fiber/ReactDOMFiberComponent.js | 4 +++ .../dom/shared/inputValueTracking.js | 33 ++++++++++--------- .../dom/stack/client/ReactDOMComponent.js | 3 ++ 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 9bc7e30a618e8..5df79697c94d7 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -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); diff --git a/src/renderers/dom/shared/inputValueTracking.js b/src/renderers/dom/shared/inputValueTracking.js index 9f0d50ecd07a5..e091e0deb86af 100644 --- a/src/renderers/dom/shared/inputValueTracking.js +++ b/src/renderers/dom/shared/inputValueTracking.js @@ -23,6 +23,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 +46,11 @@ function getTracker(inst: any) { return inst._wrapperState.valueTracker; } -function attachTracker(inst: InstanceWithWrapperState, tracker: ?ValueTracker) { - inst._wrapperState.valueTracker = tracker; +function detachTracker(subject: SubjectWithWrapperState) { + delete subject._wrapperState.valueTracker; } -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,22 +112,22 @@ 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) { + updateValueIfChanged(inst: SubjectWithWrapperState | Fiber) { if (!inst) { return false; } @@ -144,9 +143,13 @@ var inputValueTracking = { } var lastValue = tracker.getValue(); - var nextValue = getValueFromNode( - ReactDOMComponentTree.getNodeFromInstance(inst), - ); + + var node = inst; + if (!(inst: any).nodeName) { + node = ReactDOMComponentTree.getNodeFromInstance(inst); + } + + 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); From 84bda9788468e6f0a0d4dad4e112fcdc26584938 Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Wed, 12 Jul 2017 12:29:19 -0400 Subject: [PATCH 3/5] address feedback --- .../dom/shared/inputValueTracking.js | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/renderers/dom/shared/inputValueTracking.js b/src/renderers/dom/shared/inputValueTracking.js index e091e0deb86af..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'; @@ -47,7 +48,7 @@ function getTracker(inst: any) { } function detachTracker(subject: SubjectWithWrapperState) { - delete subject._wrapperState.valueTracker; + subject._wrapperState.valueTracker = null; } function getValueFromNode(node: any) { @@ -127,26 +128,28 @@ var inputValueTracking = { inst._wrapperState.valueTracker = trackValueOnNode(node, inst); }, - updateValueIfChanged(inst: SubjectWithWrapperState | 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 node = inst; - if (!(inst: any).nodeName) { - node = 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); From 15228e8b5f9d056d475955a54723cd906ffa5619 Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Wed, 12 Jul 2017 13:45:16 -0400 Subject: [PATCH 4/5] fix tests; prettier --- fixtures/dom/src/components/TestCase.js | 3 +- .../input-change-events/RadioGroupFixture.js | 28 ++++++++----------- .../fixtures/input-change-events/index.js | 3 +- .../__tests__/inputValueTracking-test.js | 8 ++---- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/fixtures/dom/src/components/TestCase.js b/fixtures/dom/src/components/TestCase.js index 067d042b81352..27757557495a2 100644 --- a/fixtures/dom/src/components/TestCase.js +++ b/fixtures/dom/src/components/TestCase.js @@ -65,7 +65,8 @@ class TestCase extends React.Component { {introducedIn &&
    First broken in:
    } {introducedIn &&
    - + {introducedIn}
    } diff --git a/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js b/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js index 3ec6cf1fd3a4a..8f4495976b9d4 100644 --- a/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js +++ b/fixtures/dom/src/components/fixtures/input-change-events/RadioGroupFixture.js @@ -12,21 +12,21 @@ class RadioGroupFixture extends React.Component { } handleChange = () => { - this.setState(({ changeCount }) => { + this.setState(({changeCount}) => { return { - changeCount: changeCount + 1 - } - }) - } + changeCount: changeCount + 1, + }; + }); + }; handleReset = () => { this.setState({ changeCount: 0, - }) - } + }); + }; render() { - const { changeCount } = this.state; + const {changeCount} = this.state; const color = changeCount === 2 ? 'green' : 'red'; return ( @@ -35,27 +35,23 @@ class RadioGroupFixture extends React.Component { Radio 1 {' '} -

    +

    onChange{' calls: '}{changeCount}

    - ) + ); } } 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 b181a14b723f6..41920e5802403 100644 --- a/fixtures/dom/src/components/fixtures/input-change-events/index.js +++ b/fixtures/dom/src/components/fixtures/input-change-events/index.js @@ -54,8 +54,7 @@ class InputChangeEvents extends React.Component { Radio inputs should fire change events when the value moved to another named input `} - introducedIn="15.6.0" - > + introducedIn="15.6.0">
  • Click on the "Radio 2"
  • Click back to "Radio 1"
    • diff --git a/src/renderers/dom/shared/__tests__/inputValueTracking-test.js b/src/renderers/dom/shared/__tests__/inputValueTracking-test.js index f2bab94f3b413..dbec64a15697c 100644 --- a/src/renderers/dom/shared/__tests__/inputValueTracking-test.js +++ b/src/renderers/dom/shared/__tests__/inputValueTracking-test.js @@ -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); }); From dede4b614485fb2d968c5fac02b48c0f3b4bcbda Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Thu, 13 Jul 2017 10:42:46 -0400 Subject: [PATCH 5/5] Update TestCase.js --- fixtures/dom/src/components/TestCase.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fixtures/dom/src/components/TestCase.js b/fixtures/dom/src/components/TestCase.js index 27757557495a2..e1e6321e269a8 100644 --- a/fixtures/dom/src/components/TestCase.js +++ b/fixtures/dom/src/components/TestCase.js @@ -100,7 +100,7 @@ class TestCase extends React.Component {

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

    Note: {' '}