From 70abda5b92b8aae4dc0255a8ca00c2025ce05c3d Mon Sep 17 00:00:00 2001 From: landvibe Date: Mon, 20 Nov 2017 00:44:35 +0900 Subject: [PATCH] Switching the name property preserves radio selection Fixes a case where changing the name and checked value of a radio button in the same update would lead to checking the wrong radio input. Also adds a DOM test fixture for related issue. Related issues: https://github.com/facebook/react/issues/7630 --- .../RadioNameChangeFixture.js | 46 +++++++++++++++++++ .../fixtures/input-change-events/index.js | 19 ++++++++ .../src/__tests__/ReactDOMInput-test.js | 39 ++++++++++++++++ .../src/client/ReactDOMFiberComponent.js | 11 +++++ .../src/client/ReactDOMFiberInput.js | 17 +++---- 5 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 fixtures/dom/src/components/fixtures/input-change-events/RadioNameChangeFixture.js diff --git a/fixtures/dom/src/components/fixtures/input-change-events/RadioNameChangeFixture.js b/fixtures/dom/src/components/fixtures/input-change-events/RadioNameChangeFixture.js new file mode 100644 index 0000000000..1169c61c2d --- /dev/null +++ b/fixtures/dom/src/components/fixtures/input-change-events/RadioNameChangeFixture.js @@ -0,0 +1,46 @@ +const React = window.React; +const noop = n => n; + +class RadioNameChangeFixture extends React.Component { + state = { + updated: false, + }; + onClick = () => { + this.setState(state => { + return {updated: !state.updated}; + }); + }; + render() { + const {updated} = this.state; + const radioName = updated ? 'firstName' : 'secondName'; + return ( +
+ + + + +
+ +
+
+ ); + } +} + +export default RadioNameChangeFixture; 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 8390b080a4..9c904b7f61 100644 --- a/fixtures/dom/src/components/fixtures/input-change-events/index.js +++ b/fixtures/dom/src/components/fixtures/input-change-events/index.js @@ -3,6 +3,7 @@ import TestCase from '../../TestCase'; import RangeKeyboardFixture from './RangeKeyboardFixture'; import RadioClickFixture from './RadioClickFixture'; import RadioGroupFixture from './RadioGroupFixture'; +import RadioNameChangeFixture from './RadioNameChangeFixture'; import InputPlaceholderFixture from './InputPlaceholderFixture'; const React = window.React; @@ -88,6 +89,24 @@ class InputChangeEvents extends React.Component { + + +
  • Click the toggle button
    • +     + + + The checked radio button should switch between the first and second radio button + + + +     ); } diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index b75c18c986..36d8add971 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -668,6 +668,45 @@ describe('ReactDOMInput', () => { expect(cNode.checked).toBe(true); }); + it('should check the correct radio when the selected name moves', () => { + class App extends React.Component { + state = { + updated: false, + }; + onClick = () => { + this.setState({updated: true}); + }; + render() { + const {updated} = this.state; + const radioName = updated ? 'secondName' : 'firstName'; + return ( +
    +
    + ); + } + } + + var stub = ReactTestUtils.renderIntoDocument(); + var buttonNode = ReactDOM.findDOMNode(stub).childNodes[0]; + var firstRadioNode = ReactDOM.findDOMNode(stub).childNodes[1]; + expect(firstRadioNode.checked).toBe(false); + ReactTestUtils.Simulate.click(buttonNode); + expect(firstRadioNode.checked).toBe(true); + }); + it('should control radio buttons if the tree updates during render', () => { var sharedParent = document.createElement('div'); var container1 = document.createElement('div'); diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js index e49a3c3d24..e6ff7e7c92 100644 --- a/packages/react-dom/src/client/ReactDOMFiberComponent.js +++ b/packages/react-dom/src/client/ReactDOMFiberComponent.js @@ -764,6 +764,17 @@ export function updateProperties( lastRawProps: Object, nextRawProps: Object, ): void { + // Update checked *before* name. + // In the middle of an update, it is possible to have multiple checked. + // When a checked radio tries to change name, browser makes another radio's checked false. + if ( + tag === 'input' && + nextRawProps.type === 'radio' && + nextRawProps.name != null + ) { + ReactDOMFiberInput.updateChecked(domElement, nextRawProps); + } + var wasCustomComponentTag = isCustomComponent(tag, lastRawProps); var isCustomComponentTag = isCustomComponent(tag, nextRawProps); // Apply the diff. diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 7c4b044490..118718fd4f 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -142,6 +142,14 @@ export function initWrapperState(element: Element, props: Object) { }; } +export function updateChecked(element: Element, props: Object) { + var node = ((element: any): InputWithWrapperState); + var checked = props.checked; + if (checked != null) { + DOMPropertyOperations.setValueForProperty(node, 'checked', checked); + } +} + export function updateWrapper(element: Element, props: Object) { var node = ((element: any): InputWithWrapperState); if (__DEV__) { @@ -181,14 +189,7 @@ export function updateWrapper(element: Element, props: Object) { } } - var checked = props.checked; - if (checked != null) { - DOMPropertyOperations.setValueForProperty( - node, - 'checked', - checked || false, - ); - } + updateChecked(element, props); var value = props.value; if (value != null) {