Skip to content

Commit

Permalink
Controlled inputs do not synchronize value or checked attribute
Browse files Browse the repository at this point in the history 
This commit is a follow up to prior work to resolve issues with number
inputs in React. Inputs keep their value/checked attribute in sync with the
value/checked property. This is a React behavior. Traditionally
browser DOM manipulation does not rely on keeping the value/checked
attribute in sync.

It's also very problematic for number inputs. After discussion, it was
decided to make a breaking change to no longer sync up the
value/checked attribute with it's assoicated property.

For this to work, I made the following changes:

- The value, defaultValue, checked and defaultChecked properties are
  now maintained within the HTML property config.
- This required adding a filter to strip out the value property on
  selects and textareas
- The logic to defer assignment of the value attribute has been
  removed form ChangeEventPlugin
- defaultValue and defaultChecked are aliased to `value` and `checked`
  so that uncontrolled input attribute assignment works as intended.
  • Loading branch information
@nhunzaker
nhunzaker committed Jul 11, 2017
1 parent 2e2f503 commit d25f3a7
Show file tree
Hide file tree
Showing 12 changed files with 95 additions and 214 deletions.
10 changes: 2 additions & 8 deletions scripts/fiber/tests-passing.txt
View file
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
118 changes: 59 additions & 59 deletions scripts/rollup/results.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,36 +25,36 @@
"gzip": 7214
},
"react-dom.development.js (UMD_DEV)": {
"size": 613141,
"gzip": 140395
"size": 621770,
"gzip": 142030
},
"react-dom.production.min.js (UMD_PROD)": {
"size": 126584,
"gzip": 39853
"size": 125762,
"gzip": 39791
},
"react-dom.development.js (NODE_DEV)": {
"size": 570841,
"gzip": 130520
"size": 580431,
"gzip": 132387
},
"react-dom.production.min.js (NODE_PROD)": {
"size": 122880,
"gzip": 38546
"size": 122127,
"gzip": 38480
},
"ReactDOMFiber-dev.js (FB_DEV)": {
"size": 570125,
"gzip": 130563
"size": 579715,
"gzip": 132422
},
"ReactDOMFiber-prod.js (FB_PROD)": {
"size": 428502,
"gzip": 96996
"size": 425463,
"gzip": 96331
},
"react-dom-test-utils.development.js (NODE_DEV)": {
"size": 53025,
"gzip": 13685
"size": 53430,
"gzip": 13782
},
"ReactTestUtils-dev.js (FB_DEV)": {
"size": 52904,
"gzip": 13646
"size": 53309,
"gzip": 13741
},
"ReactDOMServerStack-dev.js (FB_DEV)": {
"size": 460810,
Expand All @@ -65,20 +65,20 @@
"gzip": 81957
},
"react-dom-server.development.js (UMD_DEV)": {
"size": 308329,
"gzip": 77617
"size": 307012,
"gzip": 76850
},
"react-dom-server.production.min.js (UMD_PROD)": {
"size": 66111,
"gzip": 22613
"size": 65184,
"gzip": 22155
},
"react-dom-server.development.js (NODE_DEV)": {
"size": 266194,
"gzip": 67866
"size": 265858,
"gzip": 67373
},
"react-dom-server.production.min.js (NODE_PROD)": {
"size": 62380,
"gzip": 21260
"size": 61522,
"gzip": 20857
},
"ReactDOMServerStream-dev.js (FB_DEV)": {
"size": 264750,
Expand All @@ -89,64 +89,64 @@
"gzip": 51047
},
"react-art.development.js (UMD_DEV)": {
"size": 362062,
"gzip": 80236
"size": 359303,
"gzip": 79940
},
"react-art.production.min.js (UMD_PROD)": {
"size": 99126,
"gzip": 30132
"size": 97521,
"gzip": 29904
},
"react-art.development.js (NODE_DEV)": {
"size": 283458,
"gzip": 60201
"size": 280721,
"gzip": 59867
},
"react-art.production.min.js (NODE_PROD)": {
"size": 60504,
"gzip": 18189
"size": 58905,
"gzip": 17961
},
"ReactARTFiber-dev.js (FB_DEV)": {
"size": 282891,
"gzip": 60125
"size": 280154,
"gzip": 59786
},
"ReactARTFiber-prod.js (FB_PROD)": {
"size": 220185,
"gzip": 45704
"size": 215532,
"gzip": 44949
},
"ReactNativeStack-dev.js (RN_DEV)": {
"size": 197039,
"gzip": 36193
"gzip": 36189
},
"ReactNativeStack-prod.js (RN_PROD)": {
"size": 136606,
"gzip": 25990
},
"ReactNativeFiber-dev.js (RN_DEV)": {
"size": 301278,
"gzip": 51431
"size": 298654,
"gzip": 51338
},
"ReactNativeFiber-prod.js (RN_PROD)": {
"size": 221863,
"gzip": 38015
"size": 218380,
"gzip": 37833
},
"react-test-renderer.development.js (NODE_DEV)": {
"size": 280651,
"gzip": 59110
"size": 277864,
"gzip": 58787
},
"ReactTestRendererFiber-dev.js (FB_DEV)": {
"size": 280075,
"gzip": 59030
"size": 277288,
"gzip": 58707
},
"react-test-renderer-shallow.development.js (NODE_DEV)": {
"size": 8179,
"gzip": 2288
"size": 8437,
"gzip": 2308
},
"ReactShallowRenderer-dev.js (FB_DEV)": {
"size": 8080,
"gzip": 2237
"size": 8338,
"gzip": 2255
},
"react-noop-renderer.development.js (NODE_DEV)": {
"size": 274713,
"gzip": 57491
"size": 272012,
"gzip": 57213
},
"ReactHTMLString-dev.js (FB_DEV)": {
"size": 265654,
Expand Down Expand Up @@ -181,20 +181,20 @@
"gzip": 50920
},
"ReactDOMServer-dev.js (FB_DEV)": {
"size": 265645,
"gzip": 67788
"size": 265309,
"gzip": 67297
},
"ReactDOMServer-prod.js (FB_PROD)": {
"size": 197859,
"gzip": 51191
"size": 195492,
"gzip": 50325
},
"react-dom-node-stream.development.js (NODE_DEV)": {
"size": 265427,
"gzip": 67670
"size": 267552,
"gzip": 67871
},
"react-dom-node-stream.production.min.js (NODE_PROD)": {
"size": 62695,
"gzip": 21279
"size": 62459,
"gzip": 21187
},
"ReactDOMNodeStream-dev.js (FB_DEV)": {
"size": 264918,
Expand Down
5 changes: 5 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,7 @@ var ReactDOMFiberComponent = {
break;
case 'select':
ReactDOMFiberSelect.initWrapperState(domElement, rawProps);
rawProps = ReactDOMFiberSelect.getHostProps(domElement, rawProps);
trapBubbledEventsLocal(domElement, tag);
// For controlled components we always need to ensure we're listening
// to onChange. Even if there is no listener.
Expand Down Expand Up @@ -846,8 +847,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 +904,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
View file
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
View file
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
View file
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
View file
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
Loading

0 comments on commit d25f3a7

Please sign in to comment.