diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js index fae87f3d1c0f9..f0649b97522b4 100644 --- a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js @@ -38,11 +38,6 @@ export function getValueForProperty( propertyInfo: PropertyInfo, ): mixed { if (__DEV__) { - if (propertyInfo.mustUseProperty) { - const {propertyName} = propertyInfo; - return (node: any)[propertyName]; - } - const attributeName = propertyInfo.attributeName; if (!node.hasAttribute(attributeName)) { @@ -292,16 +287,6 @@ export function setValueForProperty( propertyInfo: PropertyInfo, value: mixed, ) { - if (propertyInfo.mustUseProperty) { - // We assume mustUseProperty are of BOOLEAN type because that's the only way we use it - // right now. - (node: any)[propertyInfo.propertyName] = - value && typeof value !== 'function' && typeof value !== 'symbol'; - return; - } - - // The rest are treated as attributes with special cases. - const attributeName = propertyInfo.attributeName; if (value === null) { diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index bf527f3d3659a..93686a54b8b7e 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -379,6 +379,18 @@ function setProp( } break; } + // Note: `option.selected` is not updated if `select.multiple` is + // disabled with `removeAttribute`. We have special logic for handling this. + case 'multiple': { + (domElement: any).multiple = + value && typeof value !== 'function' && typeof value !== 'symbol'; + break; + } + case 'muted': { + (domElement: any).muted = + value && typeof value !== 'function' && typeof value !== 'symbol'; + break; + } case 'suppressContentEditableWarning': case 'suppressHydrationWarning': case 'defaultValue': // Reserved @@ -703,7 +715,19 @@ export function setInitialProperties( if (propValue == null) { continue; } - setProp(domElement, tag, propKey, propValue, false, props); + switch (propKey) { + case 'selected': { + // TODO: Remove support for selected on option. + (domElement: any).selected = + propValue && + typeof propValue !== 'function' && + typeof propValue !== 'symbol'; + break; + } + default: { + setProp(domElement, tag, propKey, propValue, false, props); + } + } } ReactDOMOptionPostMountWrapper(domElement, props); return; @@ -1018,6 +1042,26 @@ export function updateProperties( ReactDOMTextareaUpdateWrapper(domElement, nextProps); return; } + case 'option': { + for (let i = 0; i < updatePayload.length; i += 2) { + const propKey = updatePayload[i]; + const propValue = updatePayload[i + 1]; + switch (propKey) { + case 'selected': { + // TODO: Remove support for selected on option. + (domElement: any).selected = + propValue && + typeof propValue !== 'function' && + typeof propValue !== 'symbol'; + break; + } + default: { + setProp(domElement, tag, propKey, propValue, false, nextProps); + } + } + } + return; + } case 'img': case 'link': case 'area': @@ -1249,7 +1293,22 @@ function diffHydratedGenericElement( extraAttributeNames.delete(propKey); diffHydratedStyles(domElement, nextProp); continue; - // eslint-disable-next-line no-fallthrough + case 'multiple': { + extraAttributeNames.delete(propKey); + const serverValue = (domElement: any).multiple; + if (nextProp !== serverValue) { + warnForPropDifference('multiple', serverValue, nextProp); + } + continue; + } + case 'muted': { + extraAttributeNames.delete(propKey); + const serverValue = (domElement: any).muted; + if (nextProp !== serverValue) { + warnForPropDifference('muted', serverValue, nextProp); + } + continue; + } default: if ( // shouldIgnoreAttribute diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index b24bfdf75bac9..ff6e83d9ae970 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -611,6 +611,16 @@ const attributeAssign = stringToPrecomputedChunk('="'); const attributeEnd = stringToPrecomputedChunk('"'); const attributeEmptyString = stringToPrecomputedChunk('=""'); +function pushBooleanAttribute( + target: Array, + name: string, + value: string | boolean | number | Function | Object, // not null or undefined +): void { + if (value && typeof value !== 'function' && typeof value !== 'symbol') { + target.push(attributeSeparator, stringToChunk(name), attributeEmptyString); + } +} + function pushAttribute( target: Array, name: string, @@ -628,6 +638,10 @@ function pushAttribute( case 'suppressHydrationWarning': // Ignored. These are built-in to React on the client. return; + case 'multiple': + case 'muted': + pushBooleanAttribute(target, name, value); + return; } if ( // shouldIgnoreAttribute @@ -1112,9 +1126,9 @@ function pushInput( } if (checked !== null) { - pushAttribute(target, 'checked', checked); + pushBooleanAttribute(target, 'checked', checked); } else if (defaultChecked !== null) { - pushAttribute(target, 'checked', defaultChecked); + pushBooleanAttribute(target, 'checked', defaultChecked); } if (value !== null) { pushAttribute(target, 'value', value); diff --git a/packages/react-dom-bindings/src/shared/DOMProperty.js b/packages/react-dom-bindings/src/shared/DOMProperty.js index 52d197fac5575..8bd96110f3a50 100644 --- a/packages/react-dom-bindings/src/shared/DOMProperty.js +++ b/packages/react-dom-bindings/src/shared/DOMProperty.js @@ -42,8 +42,6 @@ export type PropertyInfo = { +acceptsBooleans: boolean, +attributeName: string, +attributeNamespace: string | null, - +mustUseProperty: boolean, - +propertyName: string, +type: PropertyType, +sanitizeURL: boolean, +removeEmptyString: boolean, @@ -55,9 +53,7 @@ export function getPropertyInfo(name: string): PropertyInfo | null { // $FlowFixMe[missing-this-annot] function PropertyInfoRecord( - name: string, type: PropertyType, - mustUseProperty: boolean, attributeName: string, attributeNamespace: string | null, sanitizeURL: boolean, @@ -69,8 +65,6 @@ function PropertyInfoRecord( type === OVERLOADED_BOOLEAN; this.attributeName = attributeName; this.attributeNamespace = attributeNamespace; - this.mustUseProperty = mustUseProperty; - this.propertyName = name; this.type = type; this.sanitizeURL = sanitizeURL; this.removeEmptyString = removeEmptyString; @@ -91,9 +85,7 @@ const properties: {[string]: $FlowFixMe} = {}; ].forEach(([name, attributeName]) => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, STRING, - false, // mustUseProperty attributeName, // attributeName null, // attributeNamespace false, // sanitizeURL @@ -107,9 +99,7 @@ const properties: {[string]: $FlowFixMe} = {}; ['contentEditable', 'draggable', 'spellCheck', 'value'].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, BOOLEANISH_STRING, - false, // mustUseProperty name.toLowerCase(), // attributeName null, // attributeNamespace false, // sanitizeURL @@ -129,9 +119,7 @@ const properties: {[string]: $FlowFixMe} = {}; ].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, BOOLEANISH_STRING, - false, // mustUseProperty name, // attributeName null, // attributeNamespace false, // sanitizeURL @@ -170,9 +158,7 @@ const properties: {[string]: $FlowFixMe} = {}; ].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, BOOLEAN, - false, // mustUseProperty name.toLowerCase(), // attributeName null, // attributeNamespace false, // sanitizeURL @@ -180,32 +166,6 @@ const properties: {[string]: $FlowFixMe} = {}; ); }); -// These are the few React props that we set as DOM properties -// rather than attributes. These are all booleans. -[ - 'checked', - // Note: `option.selected` is not updated if `select.multiple` is - // disabled with `removeAttribute`. We have special logic for handling this. - 'multiple', - 'muted', - 'selected', - - // NOTE: if you add a camelCased prop to this list, - // you'll need to set attributeName to name.toLowerCase() - // instead in the assignment below. -].forEach(name => { - // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions - properties[name] = new PropertyInfoRecord( - name, - BOOLEAN, - true, // mustUseProperty - name, // attributeName - null, // attributeNamespace - false, // sanitizeURL - false, // removeEmptyString - ); -}); - // These are HTML attributes that are "overloaded booleans": they behave like // booleans, but can also accept a string value. [ @@ -218,9 +178,7 @@ const properties: {[string]: $FlowFixMe} = {}; ].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, OVERLOADED_BOOLEAN, - false, // mustUseProperty name, // attributeName null, // attributeNamespace false, // sanitizeURL @@ -241,9 +199,7 @@ const properties: {[string]: $FlowFixMe} = {}; ].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, POSITIVE_NUMERIC, - false, // mustUseProperty name, // attributeName null, // attributeNamespace false, // sanitizeURL @@ -255,9 +211,7 @@ const properties: {[string]: $FlowFixMe} = {}; ['rowSpan', 'start'].forEach(name => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, NUMERIC, - false, // mustUseProperty name.toLowerCase(), // attributeName null, // attributeNamespace false, // sanitizeURL @@ -356,9 +310,7 @@ const capitalize = (token: string) => token[1].toUpperCase(); const name = attributeName.replace(CAMELIZE, capitalize); // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, STRING, - false, // mustUseProperty attributeName, null, // attributeNamespace false, // sanitizeURL @@ -382,9 +334,7 @@ const capitalize = (token: string) => token[1].toUpperCase(); const name = attributeName.replace(CAMELIZE, capitalize); // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, STRING, - false, // mustUseProperty attributeName, 'http://www.w3.org/1999/xlink', false, // sanitizeURL @@ -405,9 +355,7 @@ const capitalize = (token: string) => token[1].toUpperCase(); const name = attributeName.replace(CAMELIZE, capitalize); // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[name] = new PropertyInfoRecord( - name, STRING, - false, // mustUseProperty attributeName, 'http://www.w3.org/XML/1998/namespace', false, // sanitizeURL @@ -421,9 +369,7 @@ const capitalize = (token: string) => token[1].toUpperCase(); ['tabIndex', 'crossOrigin'].forEach(attributeName => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[attributeName] = new PropertyInfoRecord( - attributeName, STRING, - false, // mustUseProperty attributeName.toLowerCase(), // attributeName null, // attributeNamespace false, // sanitizeURL @@ -436,9 +382,7 @@ const capitalize = (token: string) => token[1].toUpperCase(); const xlinkHref = 'xlinkHref'; // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[xlinkHref] = new PropertyInfoRecord( - 'xlinkHref', STRING, - false, // mustUseProperty 'xlink:href', 'http://www.w3.org/1999/xlink', true, // sanitizeURL @@ -448,9 +392,7 @@ properties[xlinkHref] = new PropertyInfoRecord( const formAction = 'formAction'; // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[formAction] = new PropertyInfoRecord( - 'formAction', STRING, - false, // mustUseProperty 'formaction', // attributeName null, // attributeNamespace true, // sanitizeURL @@ -460,9 +402,7 @@ properties[formAction] = new PropertyInfoRecord( ['src', 'href', 'action'].forEach(attributeName => { // $FlowFixMe[invalid-constructor] Flow no longer supports calling new on functions properties[attributeName] = new PropertyInfoRecord( - attributeName, STRING, - false, // mustUseProperty attributeName.toLowerCase(), // attributeName null, // attributeNamespace true, // sanitizeURL diff --git a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js index 4bf1f8aebed7a..ca469bc792690 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js @@ -183,56 +183,76 @@ function validateProperty(tagName, name, value, eventRegistry) { switch (typeof value) { case 'boolean': { - const prefix = name.toLowerCase().slice(0, 5); - const acceptsBooleans = - propertyInfo !== null - ? propertyInfo.acceptsBooleans - : prefix === 'data-' || prefix === 'aria-'; - if (!acceptsBooleans) { - if (value) { - console.error( - 'Received `%s` for a non-boolean attribute `%s`.\n\n' + - 'If you want to write it to the DOM, pass a string instead: ' + - '%s="%s" or %s={value.toString()}.', - value, - name, - name, - value, - name, - ); - } else { - console.error( - 'Received `%s` for a non-boolean attribute `%s`.\n\n' + - 'If you want to write it to the DOM, pass a string instead: ' + - '%s="%s" or %s={value.toString()}.\n\n' + - 'If you used to conditionally omit it with %s={condition && value}, ' + - 'pass %s={condition ? value : undefined} instead.', - value, - name, - name, - value, - name, - name, - name, - ); + switch (name) { + case 'checked': + case 'selected': + case 'multiple': + case 'muted': { + // Boolean properties can accept boolean values + return true; + } + default: { + if (propertyInfo === null) { + const prefix = name.toLowerCase().slice(0, 5); + if (prefix === 'data-' || prefix === 'aria-') { + return true; + } + } else if (propertyInfo.acceptsBooleans) { + return true; + } + if (value) { + console.error( + 'Received `%s` for a non-boolean attribute `%s`.\n\n' + + 'If you want to write it to the DOM, pass a string instead: ' + + '%s="%s" or %s={value.toString()}.', + value, + name, + name, + value, + name, + ); + } else { + console.error( + 'Received `%s` for a non-boolean attribute `%s`.\n\n' + + 'If you want to write it to the DOM, pass a string instead: ' + + '%s="%s" or %s={value.toString()}.\n\n' + + 'If you used to conditionally omit it with %s={condition && value}, ' + + 'pass %s={condition ? value : undefined} instead.', + value, + name, + name, + value, + name, + name, + name, + ); + } + warnedProperties[name] = true; + return true; } - warnedProperties[name] = true; - return true; } - return true; } case 'function': case 'symbol': // eslint-disable-line // Warn when a known attribute is a bad type warnedProperties[name] = true; return false; - case 'string': + case 'string': { // Warn when passing the strings 'false' or 'true' into a boolean prop - if ( - (value === 'false' || value === 'true') && - propertyInfo !== null && - propertyInfo.type === BOOLEAN - ) { + if (value === 'false' || value === 'true') { + switch (name) { + case 'checked': + case 'selected': + case 'multiple': + case 'muted': { + break; + } + default: { + if (propertyInfo === null || propertyInfo.type !== BOOLEAN) { + return true; + } + } + } console.error( 'Received the string `%s` for the boolean attribute `%s`. ' + '%s ' + @@ -248,6 +268,7 @@ function validateProperty(tagName, name, value, eventRegistry) { warnedProperties[name] = true; return true; } + } } return true; } diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js index 7343ff3bbad7b..f373af9335ea1 100644 --- a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js @@ -1045,7 +1045,13 @@ describe('ReactDOMComponent', () => { it('should not incur unnecessary DOM mutations for boolean properties', () => { const container = document.createElement('div'); - ReactDOM.render(
, container); + function onChange() { + // noop + } + ReactDOM.render( + , + container, + ); const node = container.firstChild; let nodeValue = true; @@ -1059,17 +1065,37 @@ describe('ReactDOMComponent', () => { }), }); - ReactDOM.render(
, container); + ReactDOM.render( + , + container, + ); expect(nodeValueSetter).toHaveBeenCalledTimes(0); - ReactDOM.render(
, container); + expect(() => { + ReactDOM.render( + , + container, + ); + }).toErrorDev( + 'A component is changing a controlled input to be uncontrolled. This is likely caused by ' + + 'the value changing from a defined to undefined, which should not happen. Decide between ' + + 'using a controlled or uncontrolled input element for the lifetime of the component.', + ); expect(nodeValueSetter).toHaveBeenCalledTimes(1); - ReactDOM.render(
, container); - expect(nodeValueSetter).toHaveBeenCalledTimes(2); - - ReactDOM.render(
, container); + ReactDOM.render( + , + container, + ); + // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed. expect(nodeValueSetter).toHaveBeenCalledTimes(3); + + ReactDOM.render( + , + container, + ); + // TODO: Non-null values are updated twice on inputs. This is should ideally be fixed. + expect(nodeValueSetter).toHaveBeenCalledTimes(5); }); it('should ignore attribute list for elements with the "is" attribute', () => {