diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js index 04814bd869d..8bd91685607 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/client/wrappers/ReactDOMInput.js @@ -29,7 +29,7 @@ var didWarnUncontrolledToControlled = false; function forceUpdateIfMounted() { if (this._rootNodeID) { // DOM component is still mounted; update - ReactDOMInput.updateWrapper(this); + ReactDOMInput.enforceControlledInputValue(this); } } @@ -59,20 +59,7 @@ var ReactDOMInput = { var value = LinkedValueUtils.getValue(props); var checked = LinkedValueUtils.getChecked(props); - var hostProps = Object.assign({ - // Make sure we set .type before any other properties (setting .value - // before .type means .value is lost in IE11 and below) - type: undefined, - // Make sure we set .step before .value (setting .value before .step - // means .value is rounded on mount, based upon step precision) - step: undefined, - // Make sure we set .min & .max before .value (to ensure proper order - // in corner cases such as min or max deriving from value, e.g. Issue #7170) - min: undefined, - max: undefined, - }, props, { - defaultChecked: undefined, - defaultValue: undefined, + var hostProps = Object.assign({}, props, { value: value != null ? value : inst._wrapperState.initialValue, checked: checked != null ? checked : inst._wrapperState.initialChecked, onChange: inst._wrapperState.onChange, @@ -188,6 +175,14 @@ var ReactDOMInput = { didWarnControlledToUncontrolled = true; } } + }, + + // Ensure that there is no disconnect between an input's property + // value and component state. This should run during `onChange`. + enforceControlledInputValue: function(inst) { + var props = inst._currentElement.props; + + ReactDOMInput.updateWrapper(inst); // TODO: Shouldn't this be getChecked(props)? var checked = props.checked; @@ -199,75 +194,13 @@ 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. - 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) { - var props = inst._currentElement.props; - - // 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); - - // Detach value from defaultValue. We won't do anything if we're working on - // submit or reset inputs as those values & defaultValues are linked. They - // are not resetable nodes so this operation doesn't matter and actually - // removes browser-default values (eg "Submit Query") when no value is - // provided. - - switch (props.type) { - case 'submit': - case 'reset': - break; - case 'color': - case 'date': - case 'datetime': - case 'datetime-local': - case 'month': - case 'time': - case 'week': - // This fixes the no-show issue on iOS Safari and Android Chrome: - // https://github.com/facebook/react/issues/7233 - node.value = ''; - node.value = node.defaultValue; - break; - default: - node.value = node.value; - break; - } - - // 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; - if (name !== '') { - node.name = ''; - } - node.defaultChecked = !node.defaultChecked; - node.defaultChecked = !node.defaultChecked; - if (name !== '') { - node.name = name; + DOMPropertyOperations.setValueForProperty( + ReactDOMComponentTree.getNodeFromInstance(inst), + 'value', + value + ); } }, }; diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js index 04a8b3a1f4d..5c9958e5ba3 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js @@ -47,7 +47,6 @@ describe('ReactDOMInput', () => { stub = ReactTestUtils.renderIntoDocument(stub); var node = ReactDOM.findDOMNode(stub); - expect(node.getAttribute('value')).toBe('0'); expect(node.value).toBe('0'); }); @@ -825,7 +824,7 @@ describe('ReactDOMInput', () => { }); it('sets type, step, min, max before value always', () => { - if (!ReactDOMFeatureFlags.useCreateElement) { + if (!ReactDOMFeatureFlags.us3eCreateElement) { return; } var log = []; @@ -853,10 +852,8 @@ describe('ReactDOMInput', () => { 'set step', 'set min', 'set max', - 'set value', - 'set value', - 'set checked', - 'set checked', + 'set value', // attribute + 'set value', // property ]); }); @@ -885,43 +882,4 @@ describe('ReactDOMInput', () => { input.setState({ type: 'text', value: 'Test' }); expect(node.value).toEqual('Test'); }); - - it('resets value of date/time input to fix bugs in iOS Safari', () => { - // https://github.com/facebook/react/issues/7233 - if (!ReactDOMFeatureFlags.useCreateElement) { - return; - } - - function strify(x) { - return JSON.stringify(x, null, 2); - } - - var log = []; - var originalCreateElement = document.createElement; - spyOn(document, 'createElement').and.callFake(function(type) { - var el = originalCreateElement.apply(this, arguments); - if (type === 'input') { - Object.defineProperty(el, 'value', { - set: function(val) { - log.push(`node.value = ${strify(val)}`); - }, - }); - spyOn(el, 'setAttribute').and.callFake(function(name, val) { - log.push(`node.setAttribute(${strify(name)}, ${strify(val)})`); - }); - } - return el; - }); - - ReactTestUtils.renderIntoDocument(); - expect(log).toEqual([ - 'node.setAttribute("data-reactroot", "")', - 'node.setAttribute("type", "date")', - 'node.setAttribute("value", "1980-01-01")', - 'node.value = ""', - 'node.value = ""', - 'node.setAttribute("checked", "")', - 'node.setAttribute("checked", "")', - ]); - }); }); diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 068638ad6b7..208cf731ea2 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -23,6 +23,7 @@ var DOMPropertyInjection = { * specifies how the associated DOM property should be accessed or rendered. */ MUST_USE_PROPERTY: 0x1, + NO_MARKUP: 0x2, HAS_BOOLEAN_VALUE: 0x4, HAS_NUMERIC_VALUE: 0x8, HAS_POSITIVE_NUMERIC_VALUE: 0x10 | 0x8, @@ -70,6 +71,9 @@ var DOMPropertyInjection = { ); } + // Some tags must assign properties in a specific order. Assign that here: + Object.assign(DOMProperty.order, domPropertyConfig.Order); + for (var propName in Properties) { invariant( !DOMProperty.properties.hasOwnProperty(propName), @@ -90,6 +94,7 @@ var DOMPropertyInjection = { mutationMethod: null, mustUseProperty: checkMask(propConfig, Injection.MUST_USE_PROPERTY), + noMarkup: checkMask(propConfig, Injection.NO_MARKUP), hasBooleanValue: checkMask(propConfig, Injection.HAS_BOOLEAN_VALUE), hasNumericValue: checkMask(propConfig, Injection.HAS_NUMERIC_VALUE), hasPositiveNumericValue: @@ -175,6 +180,8 @@ var DOMProperty = { * initial render. * mustUseProperty: * Whether the property must be accessed and mutated as an object property. + * noMarkup: + * Whether the property will generate HTML when rendered to a string. * hasBooleanValue: * Whether the property should be removed when set to a falsey value. * hasNumericValue: @@ -190,6 +197,12 @@ var DOMProperty = { */ properties: {}, + /** + * Some elements need specific attribute insertion order. This property + * stores that configuration. + */ + order: {}, + /** * Mapping from lowercase property names to the properly cased version, used * to warn in the case of missing properties. Available only in __DEV__. diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 895a7ec71ed..f68a08c3ecf 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -91,7 +91,7 @@ var DOMPropertyOperations = { var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { - if (shouldIgnoreValue(propertyInfo, value)) { + if (propertyInfo.noMarkup || shouldIgnoreValue(propertyInfo, value)) { return ''; } var attributeName = propertyInfo.attributeName; diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index ec288be9eae..a9102c7a25a 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -14,6 +14,7 @@ var DOMProperty = require('DOMProperty'); var MUST_USE_PROPERTY = DOMProperty.injection.MUST_USE_PROPERTY; +var NO_MARKUP = DOMProperty.injection.NO_MARKUP; var HAS_BOOLEAN_VALUE = DOMProperty.injection.HAS_BOOLEAN_VALUE; var HAS_NUMERIC_VALUE = DOMProperty.injection.HAS_NUMERIC_VALUE; var HAS_POSITIVE_NUMERIC_VALUE = @@ -25,6 +26,24 @@ var HTMLDOMPropertyConfig = { isCustomAttribute: RegExp.prototype.test.bind( new RegExp('^(data|aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$') ), + + Order: { + input: [ + // Make sure we set .type before any other form properties (setting .value + // before .type means .value is lost in IE11 and below) + 'type', + // Make sure we set .step before .value (setting .value before .step + // means .value is rounded on mount: based upon step precision) + 'step', + // Fix bug in range inputs initial render + // https://github.com/facebook/react/issues/7170 + 'min', + 'max', + 'value', + 'checked', + ], + }, + Properties: { /** * Standard Properties @@ -36,8 +55,6 @@ var HTMLDOMPropertyConfig = { allowFullScreen: HAS_BOOLEAN_VALUE, allowTransparency: 0, alt: 0, - // specifies target context for links with `preload` type - as: 0, async: HAS_BOOLEAN_VALUE, autoComplete: 0, // autoFocus is polyfilled/normalized by AutoFocusUtils @@ -49,6 +66,7 @@ var HTMLDOMPropertyConfig = { charSet: 0, challenge: 0, checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, + defaultChecked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE | NO_MARKUP, cite: 0, classID: 0, className: 0, @@ -117,7 +135,6 @@ var HTMLDOMPropertyConfig = { optimum: 0, pattern: 0, placeholder: 0, - playsInline: HAS_BOOLEAN_VALUE, poster: 0, preload: 0, profile: 0, @@ -155,7 +172,8 @@ var HTMLDOMPropertyConfig = { // Setting .type throws on non- tags type: 0, useMap: 0, - value: 0, + value: MUST_USE_PROPERTY, + defaultValue: MUST_USE_PROPERTY | NO_MARKUP, width: 0, wmode: 0, wrap: 0, @@ -211,6 +229,60 @@ var HTMLDOMPropertyConfig = { }, DOMPropertyNames: { }, + DOMMutationMethods: { + value: function(node, next) { + if (next == null) { + node.removeAttribute('value'); + } else { + node.setAttribute('value', '' + next); + } + + if (next == null) { + next = ''; + } else if (next === 0) { + // Since we use loose type checking below, zero is + // falsy, so we need to manually cover it + next = '0'; + } + + if (node.value != next) { // eslint-disable-line eqeqeq + // Set value directly cause it has already been modified + node.value = next; + } + }, + + defaultValue: function(node, next) { + // If a value is present, intentially re-assign it to detatch it + // from defaultValue. Values derived from server-rendered markup + // will not had a prior changes to assign value as a property. + // + // Make an exception for multi-selects + if (!node.multiple && node.value !== '') { + node.value = node.value; + } + + node.defaultValue = next; + }, + + // Chrome ~50 does not properly detatch defaultChecked, this mutation method + // is a work around to mitigate a bug where setting defaultChecked changes + // the value of checked, even after detachment: + // Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=608416 + defaultChecked: function(node, next) { + // The property priority list mandates that `checked` be assigned + // before `defaultChecked`. Additionally, ReactDOMInput ensures that + // `checked` assigns `defaultChecked` if it is not otherwise specified. + // This means that we can always force checked to be the original without + // any influence from `defaultChecked`. + var checked = node.checked; + node.defaultChecked = next; + + // No need to touch the DOM again if the property is the same + if (checked !== next) { + node.checked = next; + } + }, + }, }; module.exports = HTMLDOMPropertyConfig; diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index c9bae5a8026..c49fd7c2078 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -236,11 +236,6 @@ function putListener() { ); } -function inputPostMount() { - var inst = this; - ReactDOMInput.postMountWrapper(inst); -} - function textareaPostMount() { var inst = this; ReactDOMTextarea.postMountWrapper(inst); @@ -654,10 +649,6 @@ ReactDOMComponent.Mixin = { switch (this._tag) { case 'input': - transaction.getReactMountReady().enqueue( - inputPostMount, - this - ); if (props.autoFocus) { transaction.getReactMountReady().enqueue( AutoFocusUtils.focusDOMComponent, @@ -946,6 +937,8 @@ ReactDOMComponent.Mixin = { */ _updateDOMProperties: function(lastProps, nextProps, transaction) { var propKey; + var nextProp; + var lastProp; var styleName; var styleUpdates; for (propKey in lastProps) { @@ -983,9 +976,34 @@ ReactDOMComponent.Mixin = { DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey); } } + + var ordered = DOMProperty.order[this._tag]; + if (ordered != null) { + for (var i = 0, len = ordered.length; i < len; i++) { + propKey = ordered[i]; + + if (nextProps.hasOwnProperty(propKey)) { + lastProp = lastProps ? lastProps[propKey] : undefined; + nextProp = nextProps[propKey]; + + if (lastProp !== nextProp) { + DOMPropertyOperations.setValueForProperty( + getNode(this), + propKey, + nextProp + ); + } + } + } + } + for (propKey in nextProps) { - var nextProp = nextProps[propKey]; - var lastProp = + if (ordered != null && ordered.indexOf(propKey) >= 0) { + continue; + } + + nextProp = nextProps[propKey]; + lastProp = propKey === STYLE ? this._previousStyleCopy : lastProps != null ? lastProps[propKey] : undefined; if (!nextProps.hasOwnProperty(propKey) || diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 972ef0b71b5..23d27ba8e50 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -260,7 +260,7 @@ describe('DOMPropertyOperations', () => { expect(stubNode.hasAttribute('data-foo')).toBe(false); }); - it('should use mutation method where applicable', () => { + it('should use mutation method where applicable', function() { var foobarSetter = jest.fn(); // inject foobar DOM property DOMProperty.injection.injectDOMPropertyConfig({ @@ -381,6 +381,19 @@ describe('DOMPropertyOperations', () => { expect(stubNode.value).toBe(''); }); + 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); + }); + it('should not leave all options selected when deleting multiple', () => { stubNode = document.createElement('select'); ReactDOMComponentTree.precacheNode(stubInstance, stubNode); @@ -400,6 +413,27 @@ describe('DOMPropertyOperations', () => { stubNode.options[1].selected ).toBe(false); }); + + it('should not update numeric values when the input.value is loosely the same', function() { + DOMPropertyOperations.setValueForProperty(stubNode, 'type', 'number'); + DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30); + + Object.defineProperty(stubNode, 'value', { + get() { + return this._value; + }, + set(value) { + if (value == this._value) { // eslint-disable-line eqeqeq + throw new Error('Should not have overriden value ' + this._value + ' with ' + value); + } + + this._value = value; + }, + }); + + DOMPropertyOperations.setValueForProperty(stubNode, 'value', 3e1); + DOMPropertyOperations.setValueForProperty(stubNode, 'value', '3e1'); + }); }); describe('injectDOMPropertyConfig', () => { diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 0c09b11baf5..475b7f58c7e 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -502,31 +502,31 @@ describe('ReactDOMComponent', () => { it('should not incur unnecessary DOM mutations for string properties', () => { var container = document.createElement('div'); - ReactDOM.render(
, container); + ReactDOM.render(, container); var node = container.firstChild; var nodeValueSetter = jest.genMockFn(); var oldSetAttribute = node.setAttribute.bind(node); - node.setAttribute = function(key, value) { - oldSetAttribute(key, value); - nodeValueSetter(key, value); + node.setAttribute = function(key, itemProp) { + oldSetAttribute(key, itemProp); + nodeValueSetter(key, itemProp); }; - ReactDOM.render(, container); + ReactDOM.render(, container); expect(nodeValueSetter.mock.calls.length).toBe(1); - ReactDOM.render(, container); + ReactDOM.render(, container); expect(nodeValueSetter.mock.calls.length).toBe(1); ReactDOM.render(, container); expect(nodeValueSetter.mock.calls.length).toBe(1); - ReactDOM.render(, container); + ReactDOM.render(, container); expect(nodeValueSetter.mock.calls.length).toBe(1); - ReactDOM.render(, container); + ReactDOM.render(, container); expect(nodeValueSetter.mock.calls.length).toBe(2); ReactDOM.render(, container); @@ -616,6 +616,83 @@ describe('ReactDOMComponent', () => { expect(container.textContent).toBe('BADC'); }); + + describe('DOM mutations for value property', function() { + var container, node, nodeValueSetter; + + beforeEach(function() { + container = document.createElement('div'); + ReactDOM.render(, container); + nodeValueSetter = jest.genMockFn(); + node = container.firstChild; + + var value = null; + Object.defineProperty(node, 'value', { + get() { + return value; + }, + set(next) { + value = next; + nodeValueSetter(value); + }, + }); + }); + + it('should assign new properties', function() { + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + }); + + it('should not assign if nothing has changed', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + }); + + it('should remove if the property is undefined', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(2); + }); + + it('should remove if the property is null', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(2); + }); + + it('should not reassign null from undefined', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + }); + + it('should not reassign an empty string from null', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + }); + + it('should not reassign an undefined from an empty string', function() { + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + }); + + it('should reassign zero from an empty string', function() { + ReactDOM.render(, container); + + expect(nodeValueSetter.mock.calls.length).toBe(1); + + // extra check here to verify the correct value was set + expect(node.value).toEqual('0'); + }); + + it('should reassign an empty string from zero', function() { + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(nodeValueSetter.mock.calls.length).toBe(2); + }); + }); }); describe('createOpenTagMarkup', () => {