Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fix on input radio #11227

Merged
merged 9 commits into from Nov 19, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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 (
<div>
<label>
<input
type="radio"
name={radioName}
onChange={noop}
checked={updated === true}
/>
First Radio
</label>

<label>
<input
type="radio"
name={radioName}
onChange={noop}
checked={updated === false}
/>
Second Radio
</label>

<div>
<button type="button" onClick={this.onClick}>Toggle</button>
</div>
</div>
);
}
}

export default RadioNameChangeFixture;
19 changes: 19 additions & 0 deletions fixtures/dom/src/components/fixtures/input-change-events/index.js
Expand Up @@ -5,6 +5,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';

class InputChangeEvents extends React.Component {
Expand Down Expand Up @@ -87,6 +88,24 @@ class InputChangeEvents extends React.Component {

<InputPlaceholderFixture />
</TestCase>
<TestCase
title="Radio button groups with name changes"
description={`
A radio button group should have correct checked value when
the names changes
`}
resolvedBy="#11227"
affectedBrowsers="IE9+">
<TestCase.Steps>
<li>Click the toggle button</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The checked radio button should switch between the first and second radio button
</TestCase.ExpectedResult>

<RadioNameChangeFixture />
</TestCase>
</FixtureSet>
);
}
Expand Down
39 changes: 39 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Expand Up @@ -669,6 +669,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 (
<div>
<button type="button" onClick={this.onClick} />
<input
type="radio"
name={radioName}
onChange={emptyFunction}
checked={updated === true}
/>
<input
type="radio"
name={radioName}
onChange={emptyFunction}
checked={updated === false}
/>
</div>
);
}
}

var stub = ReactTestUtils.renderIntoDocument(<App />);
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');
Expand Down
7 changes: 7 additions & 0 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
Expand Up @@ -771,6 +771,13 @@ 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') {
ReactDOMFiberInput.updateChecked(domElement, nextRawProps, 'radio');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to figure out why this is necessary (but it is definitely effective).

updateNamedCousins in ReactDOMInput should take care of this, but it looks like it doesn't run in the test case you've provided.

@jquense updateNamedCousins gets called in restoreControlledState, but for what ever reason it only fires on the button, which I guess is clicked and responsible for the last trigger. The radio input never gets targeted by restoreControlledState.

It feels like there's something deeper here that we need to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updateNamedCousins is called only on the button because of this code ReactTestUtils.Simulate.click(buttonNode);
I checked that updateNamedCousins is not called when I run your fixture.


var wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
var isCustomComponentTag = isCustomComponent(tag, nextRawProps);
// Apply the diff.
Expand Down
23 changes: 15 additions & 8 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
Expand Up @@ -144,6 +144,20 @@ export function initWrapperState(element: Element, props: Object) {
};
}

export function updateChecked(
element: Element,
props: Object,
targetType: ?string,
) {
var node = ((element: any): InputWithWrapperState);
if (!targetType || targetType === node.type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In updateNamedCousins, we check for radio buttons like:

function updateNamedCousins(rootNode, props) {
  var name = props.name;
  if (props.type === 'radio' && name != null) {
    // ...
  }
  // ...
}

Can you use that check here? That would allow us to remove the targetType argument and reading from the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the code in updateProperties because updateWrapper also calls updateChecked

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__) {
Expand Down Expand Up @@ -183,14 +197,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) {
Expand Down