Skip to content

Fix controlled radio button behavior #281

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

Merged
merged 1 commit into from
Sep 7, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/core/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var nodeCache = {};
var $ = require('$');

/** Mapping from reactRootID to React component instance. */
var instanceByReactRootID = {};
var instancesByReactRootID = {};
Copy link
Member

Choose a reason for hiding this comment

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

Can you rebase & update the other new place that uses this? It's in the same file. Also, @sebmarkbage, heads up about that.

Actually, why the rename, you still get a single instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did it for consistency with containersByReactRootID -- plural sounded better to my ear but I can switch both to singular if you prefer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zpao and I decided on IRC to stick with the plural; I just rebased.


/** Mapping from reactRootID to `container` nodes. */
var containersByReactRootID = {};
Expand Down Expand Up @@ -212,7 +212,7 @@ var ReactMount = {
useTouchEvents: false,

/** Exposed for debugging purposes **/
_instanceByReactRootID: instanceByReactRootID,
_instancesByReactRootID: instancesByReactRootID,

/**
* This is a hook provided to support rendering React components while
Expand Down Expand Up @@ -272,7 +272,7 @@ var ReactMount = {
ReactMount.prepareEnvironmentForDOM();

var reactRootID = ReactMount.registerContainer(container);
instanceByReactRootID[reactRootID] = nextComponent;
instancesByReactRootID[reactRootID] = nextComponent;
return reactRootID;
},

Expand Down Expand Up @@ -316,7 +316,7 @@ var ReactMount = {
* @return {ReactComponent} Component instance rendered in `container`.
*/
renderComponent: function(nextComponent, container, callback) {
var registeredComponent = instanceByReactRootID[getReactRootID(container)];
var registeredComponent = instancesByReactRootID[getReactRootID(container)];

if (registeredComponent) {
if (registeredComponent.constructor === nextComponent.constructor) {
Expand Down Expand Up @@ -403,12 +403,12 @@ var ReactMount = {
*/
unmountAndReleaseReactRootNode: function(container) {
var reactRootID = getReactRootID(container);
var component = instanceByReactRootID[reactRootID];
var component = instancesByReactRootID[reactRootID];
if (!component) {
return false;
}
ReactMount.unmountComponentFromNode(component, container);
delete instanceByReactRootID[reactRootID];
delete instancesByReactRootID[reactRootID];
delete containersByReactRootID[reactRootID];
if (__DEV__) {
delete rootElementsByReactRootID[reactRootID];
Expand Down
1 change: 1 addition & 0 deletions src/dom/DefaultDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ var DefaultDOMPropertyConfig = {
disabled: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
draggable: null,
encType: null,
form: MUST_USE_ATTRIBUTE,
frameBorder: MUST_USE_ATTRIBUTE,
height: MUST_USE_ATTRIBUTE,
hidden: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
Expand Down
55 changes: 55 additions & 0 deletions src/dom/components/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,16 @@
var DOMPropertyOperations = require('DOMPropertyOperations');
var ReactCompositeComponent = require('ReactCompositeComponent');
var ReactDOM = require('ReactDOM');
var ReactMount = require('ReactMount');

var invariant = require('invariant');
var merge = require('merge');

// Store a reference to the <input> `ReactNativeComponent`.
var input = ReactDOM.input;

var instancesByReactID = {};

/**
* Implements an <input> native component that allows setting these optional
* props: `checked`, `value`, `defaultChecked`, and `defaultValue`.
Expand Down Expand Up @@ -71,6 +75,17 @@ var ReactDOMInput = ReactCompositeComponent.createClass({
return input(props, this.props.children);
},

componentDidMount: function(rootNode) {
var id = ReactMount.getID(rootNode);
instancesByReactID[id] = this;
},

componentWillUnmount: function() {
var rootNode = this.getDOMNode();
var id = ReactMount.getID(rootNode);
delete instancesByReactID[id];
},

componentDidUpdate: function(prevProps, prevState, rootNode) {
if (this.props.checked != null) {
DOMPropertyOperations.setValueForProperty(
Expand Down Expand Up @@ -101,6 +116,46 @@ var ReactDOMInput = ReactCompositeComponent.createClass({
checked: event.target.checked,
value: event.target.value
});

var name = this.props.name;
if (this.props.type === 'radio' && name != null) {
var rootNode = this.getDOMNode();
// If `rootNode.form` was non-null, then we could try `form.elements`,
// but that sometimes behaves strangely in IE8. We could also try using
// `form.getElementsByName`, but that will only return direct children
// and won't include inputs that use the HTML5 `form=` attribute. Since
// the input might not even be in a form, let's just use the global
// `getElementsByName` to ensure we don't miss anything.
var group = document.getElementsByName(name);
for (var i = 0; i < group.length; i++) {
var otherNode = group[i];
if (otherNode === rootNode ||
otherNode.nodeName !== 'INPUT' || otherNode.type !== 'radio' ||
otherNode.form !== rootNode.form) {
continue;
}
var otherID = ReactMount.getID(otherNode);
invariant(
otherID,
'ReactDOMInput: Mixing React and non-React radio inputs with the ' +
'same `name` is not supported.'
);
var otherInstance = instancesByReactID[otherID];
invariant(
otherInstance,
'ReactDOMInput: Unknown radio button ID %s.',
otherID
);
// In some cases, this will actually change the `checked` state value.
// In other cases, there's no change but this forces a reconcile upon
// which componentDidUpdate will reset the DOM property to whatever it
// should be.
otherInstance.setState({
checked: false
});
}
}

return returnValue;
}

Expand Down
43 changes: 43 additions & 0 deletions src/dom/components/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,47 @@ describe('ReactDOMInput', function() {
expect(node.value).toBe('0');
});

it('should control radio buttons', function() {
var RadioGroup = React.createClass({
render: function() {
return (
<div>
<input ref="a" type="radio" name="fruit" checked={true} />A
<input ref="b" type="radio" name="fruit" />B

<form>
<input ref="c" type="radio" name="fruit" form="pluto"
defaultChecked={true} />
</form>
</div>
);
}
});

var stub = ReactTestUtils.renderIntoDocument(<RadioGroup />);
var aNode = stub.refs.a.getDOMNode();
var bNode = stub.refs.b.getDOMNode();
var cNode = stub.refs.c.getDOMNode();

expect(aNode.checked).toBe(true);
expect(bNode.checked).toBe(false);
// c is in a separate form and shouldn't be affected at all here
expect(cNode.checked).toBe(true);

bNode.checked = true;
// This next line isn't necessary in a proper browser environment, but
// jsdom doesn't uncheck the others in a group (which makes this whole test
// a little less effective)
aNode.checked = false;
expect(cNode.checked).toBe(true);

// Now let's run the actual ReactDOMInput change event handler (on radio
// inputs, ChangeEventPlugin listens for the `click` event so trigger that)
ReactTestUtils.Simulate.click(bNode);

// The original state should have been restored
expect(aNode.checked).toBe(true);
expect(cNode.checked).toBe(true);
});

});