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

Controlled inputs do not synchronize value or checked attribute #10150

Closed
Closed
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
10 changes: 2 additions & 8 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -717,8 +717,6 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
* should set className to empty string instead of null
* should remove property properly for boolean properties
* should remove property properly even with different name
* should update an empty attribute to zero
* should always assign the value attribute for non-inputs
* should remove attributes for normal properties
* should not remove attributes for special properties
* should not leave all options selected when deleting multiple
Expand Down Expand Up @@ -1805,7 +1803,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
* should control values in reentrant events with different targets
* does change the number 2 to "2.0" with no change handler
* does change the string "2" to "2.0" with no change handler
* changes the number 2 to "2.0" using a change handler
* changes the value property from number 2 to "2.0" using a change handler
* does change the string ".98" to "0.98" with no change handler
* distinguishes precision for extra zeroes in string number values
* should display `defaultValue` of number 0
Expand Down Expand Up @@ -1867,11 +1865,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
* sets value properly with type coming later in props
* does not raise a validation warning when it switches types
* resets value of date/time input to fix bugs in iOS Safari
* always sets the attribute when values change on text inputs
* does not set the value attribute on number inputs if focused
* sets the value attribute on number inputs on blur
* an uncontrolled number input will not update the value attribute on blur
* an uncontrolled text input will not update the value attribute on blur
* does not set the attribute when values change on text inputs

src/renderers/dom/shared/wrappers/__tests__/ReactDOMOption-test.js
* should flatten children to a string
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -846,8 +846,10 @@ var ReactDOMFiberComponent = {
// Controlled attributes are not validated
// TODO: Only ignore them on controlled tags.
case 'value':
case 'defaultValue':
break;
case 'checked':
case 'defaultChecked':
break;
case 'selected':
break;
Expand Down Expand Up @@ -901,7 +903,9 @@ var ReactDOMFiberComponent = {
// Controlled attributes are not validated
// TODO: Only ignore them on controlled tags.
propKey === 'value' ||
propKey === 'defaultValue' ||
propKey === 'checked' ||
propKey === 'defaultChecked' ||
propKey === 'selected'
) {
// Noop
Expand Down
12 changes: 8 additions & 4 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,14 @@ var ReactDOMInput = {
},
props,
{
defaultChecked: undefined,
defaultValue: undefined,
value: value != null ? value : node._wrapperState.initialValue,
checked: checked != null ? checked : node._wrapperState.initialChecked,
defaultValue: value == null
? props.defaultValue
: node._wrapperState.initialValue,
defaultChecked: checked == null
? props.defaultChecked
: node._wrapperState.initialChecked,
value: undefined,
checked: undefined,
},
);

Expand Down
1 change: 1 addition & 0 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ var ReactDOMSelect = {
getHostProps: function(element: Element, props: Object) {
return Object.assign({}, props, {
value: undefined,
defaultValue: undefined,
});
},

Expand Down
33 changes: 5 additions & 28 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ var HTMLDOMPropertyConfig = {
charSet: 0,
challenge: 0,
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
defaultChecked: HAS_BOOLEAN_VALUE,
cite: 0,
classID: 0,
className: 0,
Expand Down Expand Up @@ -159,6 +160,7 @@ var HTMLDOMPropertyConfig = {
type: 0,
useMap: 0,
value: 0,
defaultValue: 0,
width: 0,
wmode: 0,
wrap: 0,
Expand Down Expand Up @@ -211,36 +213,11 @@ var HTMLDOMPropertyConfig = {
className: 'class',
htmlFor: 'for',
httpEquiv: 'http-equiv',
defaultValue: 'value',
defaultChecked: 'checked',
},
DOMPropertyNames: {},
DOMMutationMethods: {
value: function(node, value) {
if (value == null) {
return node.removeAttribute('value');
}

// Number inputs get special treatment due to some edge cases in
// Chrome. Let everything else assign the value attribute as normal.
// https://github.com/facebook/react/issues/7253#issuecomment-236074326
if (node.type !== 'number' || node.hasAttribute('value') === false) {
node.setAttribute('value', '' + value);
} else if (
node.validity &&
!node.validity.badInput &&
node.ownerDocument.activeElement !== node
) {
// Don't assign an attribute if validation reports bad
// input. Chrome will clear the value. Additionally, don't
// operate on inputs that have focus, otherwise Chrome might
// strip off trailing decimal places and cause the user's
// cursor position to jump to the beginning of the input.
//
// In ReactDOMInput, we have an onBlur event that will trigger
// this function again when focus is lost.
node.setAttribute('value', '' + value);
}
},
},
DOMMutationMethods: {},
};

module.exports = HTMLDOMPropertyConfig;
29 changes: 0 additions & 29 deletions src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,35 +297,6 @@ describe('DOMPropertyOperations', () => {
});
});

describe('value mutation method', function() {
it('should update an empty attribute to zero', function() {
var stubNode = document.createElement('input');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

stubNode.setAttribute('type', 'radio');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
spyOn(stubNode, 'setAttribute');
DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0);

expect(stubNode.setAttribute.calls.count()).toBe(1);
});

it('should always assign the value attribute for non-inputs', function() {
var stubNode = document.createElement('progress');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

spyOn(stubNode, 'setAttribute');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30);
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30');

expect(stubNode.setAttribute.calls.count()).toBe(2);
});
});

describe('deleteValueForProperty', () => {
var stubNode;
var stubInstance;
Expand Down
25 changes: 0 additions & 25 deletions src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,26 +226,6 @@ function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) {
}
}

function handleControlledInputBlur(inst, node) {
// TODO: In IE, inst is occasionally null. Why?
if (inst == null) {
return;
}

// Fiber and ReactDOM keep wrapper state in separate places
let state = inst._wrapperState || node._wrapperState;

if (!state || !state.controlled || node.type !== 'number') {
return;
}

// If controlled, assign the value attribute to the current value on blur
let value = '' + node.value;
if (node.getAttribute('value') !== value) {
node.setAttribute('value', value);
}
}

/**
* This plugin creates an `onChange` event that normalizes change events
* across form elements. This event fires at a time when it's possible to
Expand Down Expand Up @@ -300,11 +280,6 @@ var ChangeEventPlugin = {
if (handleEventFunc) {
handleEventFunc(topLevelType, targetNode, targetInst);
}

// When blurring, set the value attribute for number inputs
if (topLevelType === 'topBlur') {
handleControlledInputBlur(targetInst, targetNode);
}
},
};

Expand Down
61 changes: 4 additions & 57 deletions src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ describe('ReactDOMInput', () => {
expect(node.value).toBe('2');
});

it('changes the number 2 to "2.0" using a change handler', () => {
it('changes the value property from number 2 to "2.0" using a change handler', () => {
class Stub extends React.Component {
state = {
value: 2,
Expand All @@ -229,7 +229,7 @@ describe('ReactDOMInput', () => {

ReactTestUtils.Simulate.change(node);

expect(node.getAttribute('value')).toBe('2.0');
expect(node.getAttribute('value')).toBe('2');
expect(node.value).toBe('2.0');
});
});
Expand Down Expand Up @@ -1283,67 +1283,14 @@ describe('ReactDOMInput', () => {
};
}

it('always sets the attribute when values change on text inputs', function() {
it('does not set the attribute when values change on text inputs', function() {
var Input = getTestInput();
var stub = ReactTestUtils.renderIntoDocument(<Input type="text" />);
var node = ReactDOM.findDOMNode(stub);

ReactTestUtils.Simulate.change(node, {target: {value: '2'}});

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

it('does not set the value attribute on number inputs if focused', () => {
var Input = getTestInput();
var stub = ReactTestUtils.renderIntoDocument(
<Input type="number" value="1" />,
);
var node = ReactDOM.findDOMNode(stub);

node.focus();

ReactTestUtils.Simulate.change(node, {target: {value: '2'}});

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

it('sets the value attribute on number inputs on blur', () => {
var Input = getTestInput();
var stub = ReactTestUtils.renderIntoDocument(
<Input type="number" value="1" />,
);
var node = ReactDOM.findDOMNode(stub);

ReactTestUtils.Simulate.change(node, {target: {value: '2'}});
ReactTestUtils.SimulateNative.blur(node);

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

it('an uncontrolled number input will not update the value attribute on blur', () => {
var stub = ReactTestUtils.renderIntoDocument(
<input type="number" defaultValue="1" />,
);
var node = ReactDOM.findDOMNode(stub);

node.value = 4;

ReactTestUtils.SimulateNative.blur(node);

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

it('an uncontrolled text input will not update the value attribute on blur', () => {
var stub = ReactTestUtils.renderIntoDocument(
<input type="text" defaultValue="1" />,
);
var node = ReactDOM.findDOMNode(stub);

node.value = 4;

ReactTestUtils.SimulateNative.blur(node);

expect(node.getAttribute('value')).toBe('1');
expect(node.getAttribute('value')).toBe('');
});
});
});
12 changes: 8 additions & 4 deletions src/renderers/dom/stack/client/wrappers/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,14 @@ var ReactDOMInput = {
},
props,
{
defaultChecked: undefined,
defaultValue: undefined,
value: value != null ? value : inst._wrapperState.initialValue,
checked: checked != null ? checked : inst._wrapperState.initialChecked,
defaultValue: value == null
? props.defaultValue
: inst._wrapperState.initialValue,
defaultChecked: checked == null
? props.defaultChecked
: inst._wrapperState.initialChecked,
value: undefined,
checked: undefined,
},
);

Expand Down
1 change: 1 addition & 0 deletions src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ function updateOptions(inst, multiple, propValue) {
var ReactDOMSelect = {
getHostProps: function(inst, props) {
return Object.assign({}, props, {
defaultValue: undefined,
value: undefined,
});
},
Expand Down
2 changes: 2 additions & 0 deletions src/renderers/shared/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ class ReactDOMServerRenderer {

props = Object.assign({}, props, {
value: undefined,
defaultValue: undefined,
children: '' + initialValue,
});
} else if (tag === 'select') {
Expand Down Expand Up @@ -590,6 +591,7 @@ class ReactDOMServerRenderer {
: props.defaultValue;
props = Object.assign({}, props, {
value: undefined,
defaultValue: undefined,
});
} else if (tag === 'option') {
var selected = null;
Expand Down