From 3302df3bfc413bb07899395281603308c1e1902b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sun, 12 Nov 2017 13:01:50 -0500 Subject: [PATCH] Use defaultValue instead of setAttribute('value') This commit replaces the method of synchronizing an input's value attribute from using setAttribute to assigning defaultValue. This has several benefits: - Fixes issue where IE10+ and Edge password icon disappears (#7328) - Fixes issue where toggling input types hides display value on dates in Safari (unreported) - Removes mutationMethod behaviors from DOMPropertyOperations --- .../fixtures/number-inputs/index.js | 6 +- .../src/__tests__/ReactDOMInput-test.js | 67 ++++++++--- .../src/client/DOMPropertyOperations.js | 13 +-- .../src/client/ReactDOMFiberInput.js | 104 ++++++++---------- packages/react-dom/src/shared/DOMProperty.js | 12 -- .../src/shared/HTMLDOMPropertyConfig.js | 28 ----- 6 files changed, 102 insertions(+), 128 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/number-inputs/index.js b/fixtures/dom/src/components/fixtures/number-inputs/index.js index 99b737912a68b..46b4edb6f6468 100644 --- a/fixtures/dom/src/components/fixtures/number-inputs/index.js +++ b/fixtures/dom/src/components/fixtures/number-inputs/index.js @@ -27,9 +27,9 @@ function NumberInputs() {

- Notes: Chrome and Safari clear trailing decimals on blur. React - makes this concession so that the value attribute remains in sync with - the value property. + Notes: Modern Chrome and Safari {'<='} 6 clear trailing + decimals on blur. React makes this concession so that the value + attribute remains in sync with the value property.

diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index c851792d6ba8f..a30388b9fb1c0 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1249,15 +1249,20 @@ describe('ReactDOMInput', () => { var originalCreateElement = document.createElement; spyOnDevAndProd(document, 'createElement').and.callFake(function(type) { var el = originalCreateElement.apply(this, arguments); + var value = ''; + if (type === 'input') { Object.defineProperty(el, 'value', { - get: function() {}, - set: function() { - log.push('set value'); + get: function() { + return value; + }, + set: function(val) { + value = '' + val; + log.push('set property value'); }, }); spyOnDevAndProd(el, 'setAttribute').and.callFake(function(name, value) { - log.push('set ' + name); + log.push('set attribute ' + name); }); } return el; @@ -1267,14 +1272,14 @@ describe('ReactDOMInput', () => { , ); expect(log).toEqual([ - 'set type', - 'set step', - 'set min', - 'set max', - 'set value', - 'set value', - 'set checked', - 'set checked', + 'set attribute type', + 'set attribute min', + 'set attribute max', + 'set attribute step', + 'set attribute value', + 'set property value', + 'set attribute checked', + 'set attribute checked', ]); }); @@ -1313,9 +1318,14 @@ describe('ReactDOMInput', () => { var originalCreateElement = document.createElement; spyOnDevAndProd(document, 'createElement').and.callFake(function(type) { var el = originalCreateElement.apply(this, arguments); + var value = ''; if (type === 'input') { Object.defineProperty(el, 'value', { + get: function() { + return value; + }, set: function(val) { + value = '' + val; log.push(`node.value = ${strify(val)}`); }, }); @@ -1332,8 +1342,7 @@ describe('ReactDOMInput', () => { expect(log).toEqual([ 'node.setAttribute("type", "date")', 'node.setAttribute("value", "1980-01-01")', - 'node.value = ""', - 'node.value = ""', + 'node.value = "1980-01-01"', 'node.setAttribute("checked", "")', 'node.setAttribute("checked", "")', ]); @@ -1367,6 +1376,36 @@ describe('ReactDOMInput', () => { expect(node.getAttribute('value')).toBe('2'); }); + it('initially sets the value attribute on mount', () => { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + expect(node.getAttribute('value')).toBe('1'); + }); + + it('initially sets the value attribute for submit on mount', () => { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + expect(node.getAttribute('value')).toBe('1'); + }); + + it('initially sets the value attribute for reset on mount', () => { + var Input = getTestInput(); + var stub = ReactTestUtils.renderIntoDocument( + , + ); + var node = ReactDOM.findDOMNode(stub); + + expect(node.getAttribute('value')).toBe('1'); + }); + it('does not set the value attribute on number inputs if focused', () => { var Input = getTestInput(); var stub = ReactTestUtils.renderIntoDocument( diff --git a/packages/react-dom/src/client/DOMPropertyOperations.js b/packages/react-dom/src/client/DOMPropertyOperations.js index 309ef65350be9..51ba953d2dfae 100644 --- a/packages/react-dom/src/client/DOMPropertyOperations.js +++ b/packages/react-dom/src/client/DOMPropertyOperations.js @@ -73,8 +73,7 @@ export function getValueForProperty(node, name, expected) { if (__DEV__) { var propertyInfo = getPropertyInfo(name); if (propertyInfo) { - var mutationMethod = propertyInfo.mutationMethod; - if (mutationMethod || propertyInfo.mustUseProperty) { + if (propertyInfo.mustUseProperty) { return node[propertyInfo.propertyName]; } else { var attributeName = propertyInfo.attributeName; @@ -157,10 +156,7 @@ export function setValueForProperty(node, name, value) { var propertyInfo = getPropertyInfo(name); if (propertyInfo && shouldSetAttribute(name, value)) { - var mutationMethod = propertyInfo.mutationMethod; - if (mutationMethod) { - mutationMethod(node, value); - } else if (shouldIgnoreValue(propertyInfo, value)) { + if (shouldIgnoreValue(propertyInfo, value)) { deleteValueForProperty(node, name); return; } else if (propertyInfo.mustUseProperty) { @@ -233,10 +229,7 @@ export function deleteValueForAttribute(node, name) { export function deleteValueForProperty(node, name) { var propertyInfo = getPropertyInfo(name); if (propertyInfo) { - var mutationMethod = propertyInfo.mutationMethod; - if (mutationMethod) { - mutationMethod(node, undefined); - } else if (propertyInfo.mustUseProperty) { + if (propertyInfo.mustUseProperty) { var propName = propertyInfo.propertyName; if (propertyInfo.hasBooleanValue) { node[propName] = false; diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 118718fd4f507..605220e9b1337 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -58,30 +58,14 @@ function isControlled(props) { export function getHostProps(element: Element, props: Object) { var node = ((element: any): InputWithWrapperState); - var value = props.value; var checked = props.checked; - 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, - value: value != null ? value : node._wrapperState.initialValue, - checked: checked != null ? checked : node._wrapperState.initialChecked, - }, - ); + var hostProps = Object.assign({}, props, { + defaultChecked: undefined, + defaultValue: undefined, + value: undefined, + checked: checked != null ? checked : node._wrapperState.initialChecked, + }); return hostProps; } @@ -132,7 +116,7 @@ export function initWrapperState(element: Element, props: Object) { } } - var defaultValue = props.defaultValue; + var defaultValue = props.defaultValue == null ? '' : props.defaultValue; var node = ((element: any): InputWithWrapperState); node._wrapperState = { initialChecked: @@ -192,6 +176,7 @@ export function updateWrapper(element: Element, props: Object) { updateChecked(element, props); var value = props.value; + var valueAsString = '' + props.value; if (value != null) { if (value === 0 && node.value === '') { node.value = '0'; @@ -208,26 +193,17 @@ export function updateWrapper(element: Element, props: Object) { ) { // Cast `value` to a string to ensure the value is set correctly. While // browsers typically do this as necessary, jsdom doesn't. - node.value = '' + value; + node.value = valueAsString; } - } else if (node.value !== '' + value) { + } else if (node.value !== valueAsString) { // Cast `value` to a string to ensure the value is set correctly. While // browsers typically do this as necessary, jsdom doesn't. - node.value = '' + value; + node.value = valueAsString; } + synchronizeDefaultValue(node, props.type, valueAsString); } else { if (props.value == null && props.defaultValue != null) { - // In Chrome, assigning defaultValue to certain input types triggers input validation. - // For number inputs, the display value loses trailing decimal points. For email inputs, - // Chrome raises "The specified value is not a valid email address". - // - // Here we check to see if the defaultValue has actually changed, avoiding these problems - // when the user is inputting text - // - // https://github.com/facebook/react/issues/7253 - if (node.defaultValue !== '' + props.defaultValue) { - node.defaultValue = '' + props.defaultValue; - } + synchronizeDefaultValue(node, props.type, '' + props.defaultValue); } if (props.checked == null && props.defaultChecked != null) { node.defaultChecked = !!props.defaultChecked; @@ -237,32 +213,20 @@ export function updateWrapper(element: Element, props: Object) { export function postMountWrapper(element: Element, props: Object) { var node = ((element: any): InputWithWrapperState); + var hasUserInput = node.value !== ''; + var value = node._wrapperState.initialValue; - // 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. + if (value !== '' || props.hasOwnProperty('value')) { + // Do not assign value if it is already set. This prevents user text input + // from being lost during SSR hydration. + if (!hasUserInput) { + node.value = value; + } - 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; + // value must be assigned before defaultValue. This fixes an issue where the + // visually displayed value of date inputs disappears on mobile Safari and Chrome: + // https://github.com/facebook/react/issues/7233 + node.defaultValue = value; } // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug @@ -334,3 +298,21 @@ function updateNamedCousins(rootNode, props) { } } } + +// In Chrome, assigning defaultValue to certain input types triggers input validation. +// For number inputs, the display value loses trailing decimal points. For email inputs, +// Chrome raises "The specified value is not a valid email address". +// +// Here we check to see if the defaultValue has actually changed, avoiding these problems +// when the user is inputting text +// +// https://github.com/facebook/react/issues/7253 +function synchronizeDefaultValue(node: Element, type: ?string, value: string) { + if ( + // Focused number inputs synchronize on blur. See ChangeEventPlugin.js + (type !== 'number' || node.ownerDocument.activeElement !== node) && + node.defaultValue !== value + ) { + node.defaultValue = value; + } +} diff --git a/packages/react-dom/src/shared/DOMProperty.js b/packages/react-dom/src/shared/DOMProperty.js index 5801f4428b1e2..18aae072a5eda 100644 --- a/packages/react-dom/src/shared/DOMProperty.js +++ b/packages/react-dom/src/shared/DOMProperty.js @@ -54,9 +54,6 @@ var DOMPropertyInjection = { * DOMPropertyNames: similar to DOMAttributeNames but for DOM properties. * Property names not specified use the normalized name. * - * DOMMutationMethods: Properties that require special mutation methods. If - * `value` is undefined, the mutation method should unset the property. - * * @param {object} domPropertyConfig the config as described above. */ injectDOMPropertyConfig: function(domPropertyConfig) { @@ -64,7 +61,6 @@ var DOMPropertyInjection = { var Properties = domPropertyConfig.Properties || {}; var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {}; var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {}; - var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {}; for (var propName in Properties) { invariant( @@ -83,7 +79,6 @@ var DOMPropertyInjection = { attributeName: lowerCased, attributeNamespace: null, propertyName: propName, - mutationMethod: null, mustUseProperty: checkMask(propConfig, Injection.MUST_USE_PROPERTY), hasBooleanValue: checkMask(propConfig, Injection.HAS_BOOLEAN_VALUE), @@ -121,10 +116,6 @@ var DOMPropertyInjection = { propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName]; } - if (DOMMutationMethods.hasOwnProperty(propName)) { - propertyInfo.mutationMethod = DOMMutationMethods[propName]; - } - // Downcase references to whitelist properties to check for membership // without case-sensitivity. This allows the whitelist to pick up // `allowfullscreen`, which should be written using the property configuration @@ -154,9 +145,6 @@ export const ROOT_ATTRIBUTE_NAME = 'data-reactroot'; * propertyName: * Used on DOM node instances. (This includes properties that mutate due to * external factors.) - * mutationMethod: - * If non-null, used instead of the property or `setAttribute()` after - * initial render. * mustUseProperty: * Whether the property must be accessed and mutated as an object property. * hasBooleanValue: diff --git a/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js b/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js index ef09460238496..195e75691c1f0 100644 --- a/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js +++ b/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js @@ -83,34 +83,6 @@ var HTMLDOMPropertyConfig = { htmlFor: 'for', httpEquiv: 'http-equiv', }, - 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); - } - }, - }, }; export default HTMLDOMPropertyConfig;