Skip to content

fix textarea value of number 0 #274

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.

Sign up for GitHub

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
zpao merged 1 commit into from
Sep 11, 2013
+55 −6
Merged

fix textarea value of number 0 #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 8 additions & 5 deletions src/dom/components/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ var instancesByReactID = {};
var ReactDOMInput = ReactCompositeComponent.createClass({

getInitialState: function() {
var defaultValue = this.props.defaultValue;
return {
checked: this.props.defaultChecked || false,
value: this.props.defaultValue != null ? this.props.defaultValue : ''
value: defaultValue != null && defaultValue !== false ? defaultValue : ''
};
},

Expand All @@ -69,9 +70,11 @@ var ReactDOMInput = ReactCompositeComponent.createClass({
props.defaultValue = null;
props.checked =
this.props.checked != null ? this.props.checked : this.state.checked;
// Cast `this.props.value` to a string so equality checks pass.
props.value =
this.props.value != null ? '' + this.props.value : this.state.value;

props.value = this.props.value != null && this.props.value !== false
? '' + this.props.value
: this.state.value;

props.onChange = this._handleChange;

return input(props, this.props.children);
Expand Down Expand Up @@ -102,7 +105,7 @@ var ReactDOMInput = ReactCompositeComponent.createClass({
DOMPropertyOperations.setValueForProperty(
Copy link
Collaborator

@sophiebits sophiebits Aug 20, 2013

Choose a reason for hiding this comment

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

Obviously the old code here differs from textarea already, but perhaps this was intentional so that false becomes ''; generally in React you can write flag && <Component /> where nothing is inserted if flag is false so here perhaps we want to support <input value={flag && val} /> here too.

rootNode,
'value',
'' + this.props.value || ''
this.props.value !== false ? '' + this.props.value : ''
);
}
},
Expand Down
2 changes: 1 addition & 1 deletion src/dom/components/ReactDOMTextarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ var ReactDOMTextarea = ReactCompositeComponent.createClass({
DOMPropertyOperations.setValueForProperty(
rootNode,
'value',
this.props.value || ''
this.props.value !== false ? '' + this.props.value : ''
);
}
},
Expand Down
23 changes: 23 additions & 0 deletions src/dom/components/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,36 @@ describe('ReactDOMInput', function() {
expect(node.value).toBe('0');
});

it('should display "" for `defaultValue` of `false`', function() {
var stub = <input type="text" defaultValue={false} />;
var node = renderTextInput(stub);

expect(node.value).toBe('');
});

it('should display `value` of number 0', function() {
var stub = <input type="text" value={0} />;
var node = renderTextInput(stub);

expect(node.value).toBe('0');
});

it('should display "" for `value` of `false`', function() {
var stub = <input type="text" value={false} />;
var node = renderTextInput(stub);

expect(node.value).toBe('');
});

it('should properly control a value of number `0`', function() {
var stub = <input type="text" value={0} />;
var node = renderTextInput(stub);

node.value = 'giraffe';
ReactTestUtils.Simulate.input(node);
expect(node.value).toBe('0');
Copy link
Contributor

@benjamn benjamn Oct 1, 2013

Choose a reason for hiding this comment

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

Curious why the node.value = 'giraffe' is ignored here.

Copy link
Collaborator

@sophiebits sophiebits Oct 1, 2013

Choose a reason for hiding this comment

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

It's a controlled component with no onChange handler… (note I'm changing the actual value, not the value prop).

Copy link
Contributor

@benjamn benjamn Oct 1, 2013

Choose a reason for hiding this comment

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

So ReactTestUtils.Simulate.input(node) should cause node.value to be reset? Do you happen to know where that happens?

Copy link
Member

@zpao zpao Oct 1, 2013

Choose a reason for hiding this comment

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

https://github.com/facebook/react/blob/master/src/dom/components/ReactDOMInput.js#L81

Copy link
Collaborator

@sophiebits sophiebits Oct 1, 2013

Choose a reason for hiding this comment

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

Yeah, the input event triggers the onChange handler in ReactDOMInput which sets value on state to the current node value giraffe, causing a rerender after which componentDidUpdate sets node.value to match props.value. Something not working for you?

Copy link
Contributor

@benjamn benjamn Oct 1, 2013

Choose a reason for hiding this comment

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

I'm trying to upgrade the jsdom module that we use for our internal test environment. It's a big jump (v0.2.15 to v0.8.6), so no surprise there are problems, but one of them happens to be that node.value remains "giraffe" after this input event. I'll investigate some more. Thanks for confirming this is the expected behavior.

});

it('should control radio buttons', function() {
var RadioGroup = React.createClass({
render: function() {
Expand Down
23 changes: 23 additions & 0 deletions src/dom/components/__tests__/ReactDOMTextarea-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ describe('ReactDOMTextarea', function() {
expect(node.value).toBe('0');
});

it('should display "" for `defaultValue` of `false`', function() {
var stub = <textarea type="text" defaultValue={false} />;
var node = renderTextarea(stub);

expect(node.value).toBe('');
});

it('should allow setting `value`', function() {
var stub = <textarea value="giraffe" />;
var node = renderTextarea(stub);
Expand All @@ -75,6 +82,22 @@ describe('ReactDOMTextarea', function() {
expect(node.value).toBe('0');
});

it('should display "" for `value` of `false`', function() {
var stub = <textarea type="text" value={false} />;
var node = renderTextarea(stub);

expect(node.value).toBe('');
});

it('should properly control a value of number `0`', function() {
var stub = <textarea value={0} />;
var node = renderTextarea(stub);

node.value = 'giraffe';
ReactTestUtils.Simulate.input(node);
expect(node.value).toBe('0');
});

it('should treat children like `defaultValue`', function() {
spyOn(console, 'warn');

Expand Down