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.

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

Jump to bottom

Properly set value and defaultValue for input and textarea #6406

Merged
merged 2 commits into from
May 25, 2016
Merged

Properly set value and defaultValue for input and textarea #6406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
459cd81
Have `defaultValue` reach DOM node for html input box for #4618
hayesgm Dec 15, 2015
079b16a
Cleanup and bug fixes for merge.
jimfb Apr 4, 2016
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
45 changes: 36 additions & 9 deletions src/renderers/dom/client/wrappers/ReactDOMInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ var ReactDOMInput = {

var defaultValue = props.defaultValue;
inst._wrapperState = {
initialChecked: props.defaultChecked || false,
initialValue: defaultValue != null ? defaultValue : null,
initialChecked: props.checked != null ? props.checked : props.defaultChecked,
initialValue: props.value != null ? props.value : defaultValue,
listeners: null,
onChange: _handleChange.bind(inst),
};
Expand All @@ -166,13 +166,12 @@ var ReactDOMInput = {
if (__DEV__) {
warnIfValueIsNull(props);

var initialValue = inst._wrapperState.initialChecked || inst._wrapperState.initialValue;
var defaultValue = props.defaultChecked || props.defaultValue;
var controlled = props.checked !== undefined || props.value !== undefined;
var owner = inst._currentElement._owner;

if (
(initialValue || !inst._wrapperState.controlled) &&
!inst._wrapperState.controlled &&
controlled && !didWarnUncontrolledToControlled
) {
warning(
Expand Down Expand Up @@ -214,17 +213,45 @@ var ReactDOMInput = {
);
}

var node = ReactDOMComponentTree.getNodeFromInstance(inst);
var value = LinkedValueUtils.getValue(props);
if (value != null) {

// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
DOMPropertyOperations.setValueForProperty(
ReactDOMComponentTree.getNodeFromInstance(inst),
'value',
'' + value
);
var newValue = '' + value;

// To avoid side effects (such as losing text selection), only set value if changed
if (newValue !== node.value) {
node.value = newValue;
}
} else {
if (props.value == null && props.defaultValue != null) {
node.defaultValue = '' + props.defaultValue;
}
if (props.checked == null && props.defaultChecked != null) {
node.defaultChecked = !!props.defaultChecked;
}
}
},

postMountWrapper: function(inst) {
// This is in postMount because we need access to the DOM node, which is not
// available until after the component has mounted.
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
node.value = node.value; // Detach value from defaultValue
Copy link

Choose a reason for hiding this comment

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

I'm trying to understand the logic here. Could somebody tell what "detach" means here?

Copy link

Choose a reason for hiding this comment

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

If I recall from way back when, once you call the setter for value, it then ignores changes to the defaultValue. In this case, we set the value to the same value, which has this effect.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation


// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
// this is needed to work around a chrome bug where setting defaultChecked
// will sometimes influence the value of checked (even after detachment).
// Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=608416
// We need to temporarily unset name to avoid disrupting radio button groups.
var name = node.name;
node.name = undefined;
node.defaultChecked = !node.defaultChecked;
node.defaultChecked = !node.defaultChecked;
node.name = name;
},
};

function _handleChange(event) {
Expand Down
99 changes: 60 additions & 39 deletions src/renderers/dom/client/wrappers/ReactDOMTextarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'use strict';

var DisabledInputUtils = require('DisabledInputUtils');
var DOMPropertyOperations = require('DOMPropertyOperations');
var LinkedValueUtils = require('LinkedValueUtils');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactUpdates = require('ReactUpdates');
Expand Down Expand Up @@ -67,11 +66,14 @@ var ReactDOMTextarea = {
);

// Always set children to the same thing. In IE9, the selection range will
// get reset if `textContent` is mutated.
// get reset if `textContent` is mutated. We could add a check in setTextContent
// to only set the value if/when the value differs from the node value (which would
Copy link
Member

Choose a reason for hiding this comment

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

Is this still applicable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into it now. If not, I'll remove the comment as part of my rebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked into it. Appears that IE9 does indeed get upset if you set textContent to the same value it already had. But this was totally the wrong place for a comment to be talking about that; applied a hack in setTextContent.js which is a much more appropriate place to deal with this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's only relevant for textareas, isn't it?

Copy link
Contributor Author

@jimfb jimfb May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure. What happens if you have a contenteditable div with text in it? I wouldn't be surprised if there were another edge condition where this applied for React reconciliation, independent of text areas.

But regardless, it's the wrong place for the comment. The behavior here shouldn't be dictated by a chain of assumptions where getNativeProps returns a particular value BECAUSE it knows that the reconciler will use the textContent property (as oppose to any other perfectly-valid method) to set the children of the textarea DOM component. Correct solution is for the reconciler to properly handle this edge case by virtue of the fact that setTextContent properly handles this edge case.

Copy link
Collaborator

@sophiebits sophiebits May 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is that no matter how you might update the children, IE loses cursor position in textareas only. Elements with contenteditable should be handled by our ReactInputSelection wrapper on the transaction. I'd like this logic to be maintained as it was, where the children are always passed down as the same value.

// completely solve this IE9 bug), but Sebastian+Ben seemed to like this solution.
// The value can be a boolean or object so that's why it's forced to be a string.
var hostProps = Object.assign({}, DisabledInputUtils.getHostProps(inst, props), {
defaultValue: undefined,
value: undefined,
children: inst._wrapperState.initialValue,
defaultValue: undefined,
children: '' + inst._wrapperState.initialValue,
onChange: inst._wrapperState.onChange,
});

Expand Down Expand Up @@ -110,41 +112,45 @@ var ReactDOMTextarea = {
warnIfValueIsNull(props);
}

var defaultValue = props.defaultValue;
// TODO (yungsters): Remove support for children content in <textarea>.
var children = props.children;
if (children != null) {
if (__DEV__) {
warning(
false,
'Use the `defaultValue` or `value` props instead of setting ' +
'children on <textarea>.'
);
}
invariant(
defaultValue == null,
'If you supply `defaultValue` on a <textarea>, do not pass children.'
);
if (Array.isArray(children)) {

var value = LinkedValueUtils.getValue(props);
var initialValue = value;

// Only bother fetching default value if we're going to use it
if (value == null) {
var defaultValue = props.defaultValue;
// TODO (yungsters): Remove support for children content in <textarea>.
var children = props.children;
if (children != null) {
if (__DEV__) {
warning(
false,
'Use the `defaultValue` or `value` props instead of setting ' +
'children on <textarea>.'
);
}
invariant(
children.length <= 1,
'<textarea> can only have at most one child.'
defaultValue == null,
'If you supply `defaultValue` on a <textarea>, do not pass children.'
);
children = children[0];
if (Array.isArray(children)) {
invariant(
children.length <= 1,
'<textarea> can only have at most one child.'
);
children = children[0];
}

defaultValue = '' + children;
}

defaultValue = '' + children;
}
if (defaultValue == null) {
defaultValue = '';
if (defaultValue == null) {
defaultValue = '';
}
initialValue = defaultValue;
}
var value = LinkedValueUtils.getValue(props);

inst._wrapperState = {
// We save the initial value so that `ReactDOMComponent` doesn't update
// `textContent` (unnecessary since we update value).
// The initial value can be a boolean or object so that's why it's
// forced to be a string.
initialValue: '' + (value != null ? value : defaultValue),
initialValue: '' + initialValue,
listeners: null,
onChange: _handleChange.bind(inst),
};
Expand All @@ -157,17 +163,32 @@ var ReactDOMTextarea = {
warnIfValueIsNull(props);
}

var node = ReactDOMComponentTree.getNodeFromInstance(inst);
var value = LinkedValueUtils.getValue(props);
if (value != null) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
DOMPropertyOperations.setValueForProperty(
ReactDOMComponentTree.getNodeFromInstance(inst),
'value',
'' + value
);
var newValue = '' + value;

// To avoid side effects (such as losing text selection), only set value if changed
if (newValue !== node.value) {
node.value = newValue;
}
if (props.defaultValue == null) {
node.defaultValue = newValue;
}
}
if (props.defaultValue != null) {
node.defaultValue = props.defaultValue;
}
},

postMountWrapper: function(inst) {
// This is in postMount because we need access to the DOM node, which is not
// available until after the component has mounted.
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
node.value = node.value; // Detach value from defaultValue
},
};

function _handleChange(event) {
Expand Down
89 changes: 83 additions & 6 deletions src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ describe('ReactDOMInput', function() {
var EventConstants;
var React;
var ReactDOM;
var ReactDOMServer;
var ReactDOMFeatureFlags;
var ReactLink;
var ReactTestUtils;
Expand All @@ -35,6 +36,7 @@ describe('ReactDOMInput', function() {
EventConstants = require('EventConstants');
React = require('React');
ReactDOM = require('ReactDOM');
ReactDOMServer = require('ReactDOMServer');
ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
ReactLink = require('ReactLink');
ReactTestUtils = require('ReactTestUtils');
Expand All @@ -47,6 +49,7 @@ describe('ReactDOMInput', function() {
stub = ReactTestUtils.renderIntoDocument(stub);
var node = ReactDOM.findDOMNode(stub);

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

Expand All @@ -66,6 +69,48 @@ describe('ReactDOMInput', function() {
expect(node.value).toBe('false');
});

it('should update `defaultValue` for uncontrolled input', function() {
var container = document.createElement('div');

var node = ReactDOM.render(<input type="text" defaultValue="0" />, container);

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

ReactDOM.render(<input type="text" defaultValue="1" />, container);

expect(node.value).toBe('0');
Copy link
Collaborator

@sophiebits sophiebits May 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want to assert that node.defaultValue is '1' here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

expect(node.defaultValue).toBe('1');
});

it('should take `defaultValue` when changing to uncontrolled input', function() {
var container = document.createElement('div');

var node = ReactDOM.render(<input type="text" value="0" readOnly="true" />, container);

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

ReactDOM.render(<input type="text" defaultValue="1" />, container);

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

it('should render defaultValue for SSR', function() {
var markup = ReactDOMServer.renderToString(<input type="text" defaultValue="1" />);
var div = document.createElement('div');
div.innerHTML = markup;
expect(div.firstChild.getAttribute('value')).toBe('1');
expect(div.firstChild.getAttribute('defaultValue')).toBe(null);
});

it('should render value for SSR', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a SSR test where we render with value (not defaultValue)? If not, can you add one?

var element = <input type="text" value="1" onChange={function() {}} />;
var markup = ReactDOMServer.renderToString(element);
var div = document.createElement('div');
div.innerHTML = markup;
expect(div.firstChild.getAttribute('value')).toBe('1');
expect(div.firstChild.getAttribute('defaultValue')).toBe(null);
});

it('should display "foobar" for `defaultValue` of `objToString`', function() {
var objToString = {
toString: function() {
Expand All @@ -91,8 +136,7 @@ describe('ReactDOMInput', function() {
it('should allow setting `value` to `true`', function() {
var container = document.createElement('div');
var stub = <input type="text" value="yolo" onChange={emptyFunction} />;
stub = ReactDOM.render(stub, container);
var node = ReactDOM.findDOMNode(stub);
var node = ReactDOM.render(stub, container);

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

Expand All @@ -106,8 +150,7 @@ describe('ReactDOMInput', function() {
it('should allow setting `value` to `false`', function() {
var container = document.createElement('div');
var stub = <input type="text" value="yolo" onChange={emptyFunction} />;
stub = ReactDOM.render(stub, container);
var node = ReactDOM.findDOMNode(stub);
var node = ReactDOM.render(stub, container);

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

Expand All @@ -121,8 +164,7 @@ describe('ReactDOMInput', function() {
it('should allow setting `value` to `objToString`', function() {
var container = document.createElement('div');
var stub = <input type="text" value="foo" onChange={emptyFunction} />;
stub = ReactDOM.render(stub, container);
var node = ReactDOM.findDOMNode(stub);
var node = ReactDOM.render(stub, container);

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

Expand All @@ -138,6 +180,29 @@ describe('ReactDOMInput', function() {
expect(node.value).toEqual('foobar');
});

it('should not incur unnecessary DOM mutations', function() {
var container = document.createElement('div');
ReactDOM.render(<input value="a" />, container);

var node = container.firstChild;
var nodeValue = 'a';
var nodeValueSetter = jest.genMockFn();
Object.defineProperty(node, 'value', {
get: function() {
return nodeValue;
},
set: nodeValueSetter.mockImplementation(function(newValue) {
nodeValue = newValue;
}),
});

ReactDOM.render(<input value="a" />, container);
expect(nodeValueSetter.mock.calls.length).toBe(0);

ReactDOM.render(<input value="b"/>, container);
expect(nodeValueSetter.mock.calls.length).toBe(1);
});

it('should properly control a value of number `0`', function() {
var stub = <input type="text" value={0} onChange={emptyFunction} />;
stub = ReactTestUtils.renderIntoDocument(stub);
Expand Down Expand Up @@ -399,6 +464,13 @@ describe('ReactDOMInput', function() {

});

it('should update defaultValue to empty string', function() {
var container = document.createElement('div');
ReactDOM.render(<input type="text" defaultValue={'foo'} />, container);
ReactDOM.render(<input type="text" defaultValue={''} />, container);
expect(container.firstChild.defaultValue).toBe('');
});

it('should throw if both checkedLink and valueLink are provided', function() {
var node = document.createElement('div');
var link = new ReactLink(true, jest.fn());
Expand Down Expand Up @@ -618,6 +690,11 @@ describe('ReactDOMInput', function() {
'set data-reactroot',
'set type',
'set value',
'set value',
'set name',
'set checked',
'set checked',
'set name',
]);
});

Expand Down
Loading