From 73deff0d5162160c0aafa5cd0b87e11143fe9938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 30 Mar 2023 14:30:57 -0400 Subject: [PATCH] Refactor DOMProperty and CSSProperty (#26513) This is a step towards getting rid of the meta programming in DOMProperty and CSSProperty. This moves isAttributeNameSafe and isUnitlessNumber to a separate shared modules. isUnitlessNumber is now a single switch instead of meta-programming. There is a slight behavior change here in that I hard code a specific set of vendor-prefixed attributes instead of prefixing all the unitless properties. I based this list on what getComputedStyle returns in current browsers. I removed Opera prefixes because they were [removed in Opera](https://dev.opera.com/blog/css-vendor-prefixes-in-opera-12-50-snapshots/) itself. I included the ms ones mentioned [in the original PR](https://github.com/facebook/react/commit/5abcce534382d85887f3d33475e8e54e3b5d8457). These shouldn't really be used anymore anyway so should be pretty safe. Worst case, they'll fallback to the other property if you specify both. Finally I inline the mustUseProperty special cases - which are also the only thing that uses propertyName. These are really all controlled components and all booleans. I'm making a small breaking change here by treating `checked` and `selected` specially only on the `input` and `option` tags instead of all tags. That's because those are the only DOM nodes that actually have those properties but we used to set them as expandos instead of attributes before. That's why one of the tests is updated to now use `input` instead of testing an expando on a `div` which isn't a real use case. Interestingly this also uncovered that we update checked twice for some reason but keeping that logic for now. Ideally `multiple` and `muted` should move into `select` and `audio`/`video` respectively for the same reason. No change to the attribute-behavior fixture. --- .../src/client/CSSPropertyOperations.js | 12 +- .../src/client/DOMPropertyOperations.js | 236 ++++++++---------- .../src/client/ReactDOMComponent.js | 81 +++++- .../src/server/ReactDOMServerFormatConfig.js | 27 +- .../src/shared/CSSProperty.js | 82 ------ .../src/shared/DOMProperty.js | 94 ------- .../src/shared/ReactDOMInvalidARIAHook.js | 2 +- .../src/shared/ReactDOMUnknownPropertyHook.js | 143 ++++++----- .../src/shared/isAttributeNameSafe.js | 42 ++++ .../src/shared/isUnitlessNumber.js | 90 +++++++ .../src/__tests__/ReactDOMComponent-test.js | 40 ++- 11 files changed, 456 insertions(+), 393 deletions(-) delete mode 100644 packages/react-dom-bindings/src/shared/CSSProperty.js create mode 100644 packages/react-dom-bindings/src/shared/isAttributeNameSafe.js create mode 100644 packages/react-dom-bindings/src/shared/isUnitlessNumber.js diff --git a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js index c2abd6f4f5674..3f101ae8282c6 100644 --- a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js @@ -9,7 +9,7 @@ import {shorthandToLonghand} from './CSSShorthandProperty'; import hyphenateStyleName from '../shared/hyphenateStyleName'; import warnValidStyle from '../shared/warnValidStyle'; -import {isUnitlessNumber} from '../shared/CSSProperty'; +import isUnitlessNumber from '../shared/isUnitlessNumber'; import {checkCSSPropertyStringCoercion} from 'shared/CheckStringCoercion'; /** @@ -42,10 +42,7 @@ export function createDangerousStringForStyles(styles) { if ( typeof value === 'number' && value !== 0 && - !( - isUnitlessNumber.hasOwnProperty(styleName) && - isUnitlessNumber[styleName] - ) + !isUnitlessNumber(styleName) ) { serialized += delimiter + hyphenateStyleName(styleName) + ':' + value + 'px'; @@ -101,10 +98,7 @@ export function setValueForStyles(node, styles) { } else if ( typeof value === 'number' && value !== 0 && - !( - isUnitlessNumber.hasOwnProperty(styleName) && - isUnitlessNumber[styleName] - ) + !isUnitlessNumber(styleName) ) { style[styleName] = value + 'px'; // Presumes implicit 'px' suffix for unitless numbers } else { diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js index c27066b0cc31d..f0649b97522b4 100644 --- a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js @@ -8,13 +8,13 @@ */ import { - getPropertyInfo, - isAttributeNameSafe, BOOLEAN, OVERLOADED_BOOLEAN, NUMERIC, POSITIVE_NUMERIC, } from '../shared/DOMProperty'; + +import isAttributeNameSafe from '../shared/isAttributeNameSafe'; import sanitizeURL from '../shared/sanitizeURL'; import { enableTrustedTypesIntegration, @@ -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)) { @@ -287,152 +282,137 @@ export function getValueForAttributeOnCustomComponent( * @param {string} name * @param {*} value */ -export function setValueForProperty(node: Element, name: string, value: mixed) { - if ( - // shouldIgnoreAttribute - // We have already filtered out reserved words. - name.length > 2 && - (name[0] === 'o' || name[0] === 'O') && - (name[1] === 'n' || name[1] === 'N') - ) { +export function setValueForProperty( + node: Element, + propertyInfo: PropertyInfo, + value: mixed, +) { + const attributeName = propertyInfo.attributeName; + + if (value === null) { + node.removeAttribute(attributeName); return; } - const propertyInfo = getPropertyInfo(name); - if (propertyInfo !== null) { - 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'; + // shouldRemoveAttribute + switch (typeof value) { + case 'undefined': + case 'function': + case 'symbol': // eslint-disable-line + node.removeAttribute(attributeName); return; + case 'boolean': { + if (!propertyInfo.acceptsBooleans) { + node.removeAttribute(attributeName); + return; + } } - - // The rest are treated as attributes with special cases. - - const attributeName = propertyInfo.attributeName; - - if (value === null) { + } + if (enableFilterEmptyStringAttributesDOM) { + if (propertyInfo.removeEmptyString && value === '') { + if (__DEV__) { + if (attributeName === 'src') { + console.error( + 'An empty string ("") was passed to the %s attribute. ' + + 'This may cause the browser to download the whole page again over the network. ' + + 'To fix this, either do not render the element at all ' + + 'or pass null to %s instead of an empty string.', + attributeName, + attributeName, + ); + } else { + console.error( + 'An empty string ("") was passed to the %s attribute. ' + + 'To fix this, either do not render the element at all ' + + 'or pass null to %s instead of an empty string.', + attributeName, + attributeName, + ); + } + } node.removeAttribute(attributeName); return; } + } - // shouldRemoveAttribute - switch (typeof value) { - case 'undefined': - case 'function': - case 'symbol': // eslint-disable-line + switch (propertyInfo.type) { + case BOOLEAN: + if (value) { + node.setAttribute(attributeName, ''); + } else { node.removeAttribute(attributeName); return; - case 'boolean': { - if (!propertyInfo.acceptsBooleans) { - node.removeAttribute(attributeName); - return; + } + break; + case OVERLOADED_BOOLEAN: + if (value === true) { + node.setAttribute(attributeName, ''); + } else if (value === false) { + node.removeAttribute(attributeName); + } else { + if (__DEV__) { + checkAttributeStringCoercion(value, attributeName); } + node.setAttribute(attributeName, (value: any)); } - } - if (enableFilterEmptyStringAttributesDOM) { - if (propertyInfo.removeEmptyString && value === '') { + return; + case NUMERIC: + if (!isNaN(value)) { if (__DEV__) { - if (name === 'src') { - console.error( - 'An empty string ("") was passed to the %s attribute. ' + - 'This may cause the browser to download the whole page again over the network. ' + - 'To fix this, either do not render the element at all ' + - 'or pass null to %s instead of an empty string.', - name, - name, - ); - } else { - console.error( - 'An empty string ("") was passed to the %s attribute. ' + - 'To fix this, either do not render the element at all ' + - 'or pass null to %s instead of an empty string.', - name, - name, - ); - } + checkAttributeStringCoercion(value, attributeName); } + node.setAttribute(attributeName, (value: any)); + } else { node.removeAttribute(attributeName); - return; } - } - - switch (propertyInfo.type) { - case BOOLEAN: - if (value) { - node.setAttribute(attributeName, ''); - } else { - node.removeAttribute(attributeName); - return; - } - break; - case OVERLOADED_BOOLEAN: - if (value === true) { - node.setAttribute(attributeName, ''); - } else if (value === false) { - node.removeAttribute(attributeName); - } else { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); - } - node.setAttribute(attributeName, (value: any)); - } - return; - case NUMERIC: - if (!isNaN(value)) { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); - } - node.setAttribute(attributeName, (value: any)); - } else { - node.removeAttribute(attributeName); - } - break; - case POSITIVE_NUMERIC: - if (!isNaN(value) && (value: any) >= 1) { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); - } - node.setAttribute(attributeName, (value: any)); - } else { - node.removeAttribute(attributeName); - } - break; - default: { + break; + case POSITIVE_NUMERIC: + if (!isNaN(value) && (value: any) >= 1) { if (__DEV__) { checkAttributeStringCoercion(value, attributeName); } - let attributeValue; - // `setAttribute` with objects becomes only `[object]` in IE8/9, - // ('' + value) makes it output the correct toString()-value. - if (enableTrustedTypesIntegration) { - if (propertyInfo.sanitizeURL) { - attributeValue = (sanitizeURL(value): any); - } else { - attributeValue = (value: any); - } + node.setAttribute(attributeName, (value: any)); + } else { + node.removeAttribute(attributeName); + } + break; + default: { + if (__DEV__) { + checkAttributeStringCoercion(value, attributeName); + } + let attributeValue; + // `setAttribute` with objects becomes only `[object]` in IE8/9, + // ('' + value) makes it output the correct toString()-value. + if (enableTrustedTypesIntegration) { + if (propertyInfo.sanitizeURL) { + attributeValue = (sanitizeURL(value): any); } else { - // We have already verified this above. - // eslint-disable-next-line react-internal/safe-string-coercion - attributeValue = '' + (value: any); - if (propertyInfo.sanitizeURL) { - attributeValue = sanitizeURL(attributeValue); - } + attributeValue = (value: any); } - const attributeNamespace = propertyInfo.attributeNamespace; - if (attributeNamespace) { - node.setAttributeNS( - attributeNamespace, - attributeName, - attributeValue, - ); - } else { - node.setAttribute(attributeName, attributeValue); + } else { + // We have already verified this above. + // eslint-disable-next-line react-internal/safe-string-coercion + attributeValue = '' + (value: any); + if (propertyInfo.sanitizeURL) { + attributeValue = sanitizeURL(attributeValue); } } + const attributeNamespace = propertyInfo.attributeNamespace; + if (attributeNamespace) { + node.setAttributeNS(attributeNamespace, attributeName, attributeValue); + } else { + node.setAttribute(attributeName, attributeValue); + } } - } else if (isAttributeNameSafe(name)) { + } +} + +export function setValueForAttribute( + node: Element, + name: string, + value: mixed, +) { + if (isAttributeNameSafe(name)) { // If the prop isn't in the special list, treat it as a simple attribute. // shouldRemoveAttribute if (value === null) { diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index c1f5ed366ae2a..93686a54b8b7e 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -24,6 +24,7 @@ import { getValueForProperty, setValueForProperty, setValueForPropertyOnCustomComponent, + setValueForAttribute, } from './DOMPropertyOperations'; import { initWrapperState as ReactDOMInputInitWrapperState, @@ -378,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 @@ -408,7 +421,22 @@ function setProp( if (isCustomComponentTag) { setValueForPropertyOnCustomComponent(domElement, key, value); } else { - setValueForProperty(domElement, key, value); + if ( + // shouldIgnoreAttribute + // We have already filtered out reserved words. + key.length > 2 && + (key[0] === 'o' || key[0] === 'O') && + (key[1] === 'n' || key[1] === 'N') + ) { + return; + } + + const propertyInfo = getPropertyInfo(key); + if (propertyInfo !== null) { + setValueForProperty(domElement, propertyInfo, value); + } else { + setValueForAttribute(domElement, key, value); + } } } } @@ -687,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; @@ -1002,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': @@ -1233,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 543fa064ea388..4ede9b9ddba4d 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -38,15 +38,15 @@ import { clonePrecomputedChunk, } from 'react-server/src/ReactServerStreamConfig'; +import isAttributeNameSafe from '../shared/isAttributeNameSafe'; import { getPropertyInfo, - isAttributeNameSafe, BOOLEAN, OVERLOADED_BOOLEAN, NUMERIC, POSITIVE_NUMERIC, } from '../shared/DOMProperty'; -import {isUnitlessNumber} from '../shared/CSSProperty'; +import isUnitlessNumber from '../shared/isUnitlessNumber'; import {checkControlledValueProps} from '../shared/ReactControlledValuePropTypes'; import {validateProperties as validateARIAProperties} from '../shared/ReactDOMInvalidARIAHook'; @@ -579,10 +579,7 @@ function pushStyleAttribute( nameChunk = processStyleName(styleName); if (typeof styleValue === 'number') { - if ( - styleValue !== 0 && - !hasOwnProperty.call(isUnitlessNumber, styleName) - ) { + if (styleValue !== 0 && !isUnitlessNumber(styleName)) { valueChunk = stringToChunk(styleValue + 'px'); // Presumes implicit 'px' suffix for unitless numbers } else { valueChunk = stringToChunk('' + styleValue); @@ -614,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, @@ -631,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 @@ -1115,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/CSSProperty.js b/packages/react-dom-bindings/src/shared/CSSProperty.js deleted file mode 100644 index 5952aca573fa1..0000000000000 --- a/packages/react-dom-bindings/src/shared/CSSProperty.js +++ /dev/null @@ -1,82 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -/** - * CSS properties which accept numbers but are not in units of "px". - */ -export const isUnitlessNumber = { - animationIterationCount: true, - aspectRatio: true, - borderImageOutset: true, - borderImageSlice: true, - borderImageWidth: true, - boxFlex: true, - boxFlexGroup: true, - boxOrdinalGroup: true, - columnCount: true, - columns: true, - flex: true, - flexGrow: true, - flexPositive: true, - flexShrink: true, - flexNegative: true, - flexOrder: true, - gridArea: true, - gridRow: true, - gridRowEnd: true, - gridRowSpan: true, - gridRowStart: true, - gridColumn: true, - gridColumnEnd: true, - gridColumnSpan: true, - gridColumnStart: true, - fontWeight: true, - lineClamp: true, - lineHeight: true, - opacity: true, - order: true, - orphans: true, - scale: true, - tabSize: true, - widows: true, - zIndex: true, - zoom: true, - - // SVG-related properties - fillOpacity: true, - floodOpacity: true, - stopOpacity: true, - strokeDasharray: true, - strokeDashoffset: true, - strokeMiterlimit: true, - strokeOpacity: true, - strokeWidth: true, -}; - -/** - * @param {string} prefix vendor-specific prefix, eg: Webkit - * @param {string} key style name, eg: transitionDuration - * @return {string} style name prefixed with `prefix`, properly camelCased, eg: - * WebkitTransitionDuration - */ -function prefixKey(prefix, key) { - return prefix + key.charAt(0).toUpperCase() + key.substring(1); -} - -/** - * Support style names that may come passed in prefixed by adding permutations - * of vendor prefixes. - */ -const prefixes = ['Webkit', 'ms', 'Moz', 'O']; - -// Using Object.keys here, or else the vanilla for-in loop makes IE8 go into an -// infinite loop, because it iterates over the newly added props too. -Object.keys(isUnitlessNumber).forEach(function (prop) { - prefixes.forEach(function (prefix) { - isUnitlessNumber[prefixKey(prefix, prop)] = isUnitlessNumber[prop]; - }); -}); diff --git a/packages/react-dom-bindings/src/shared/DOMProperty.js b/packages/react-dom-bindings/src/shared/DOMProperty.js index 6b3178418551c..8bd96110f3a50 100644 --- a/packages/react-dom-bindings/src/shared/DOMProperty.js +++ b/packages/react-dom-bindings/src/shared/DOMProperty.js @@ -7,8 +7,6 @@ * @flow */ -import hasOwnProperty from 'shared/hasOwnProperty'; - type PropertyType = 0 | 1 | 2 | 3 | 4 | 5 | 6; // A simple string attribute. @@ -44,54 +42,18 @@ export type PropertyInfo = { +acceptsBooleans: boolean, +attributeName: string, +attributeNamespace: string | null, - +mustUseProperty: boolean, - +propertyName: string, +type: PropertyType, +sanitizeURL: boolean, +removeEmptyString: boolean, }; -/* eslint-disable max-len */ -export const ATTRIBUTE_NAME_START_CHAR = - ':A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6\\u00F8-\\u02FF\\u0370-\\u037D\\u037F-\\u1FFF\\u200C-\\u200D\\u2070-\\u218F\\u2C00-\\u2FEF\\u3001-\\uD7FF\\uF900-\\uFDCF\\uFDF0-\\uFFFD'; -/* eslint-enable max-len */ -export const ATTRIBUTE_NAME_CHAR: string = - ATTRIBUTE_NAME_START_CHAR + '\\-.0-9\\u00B7\\u0300-\\u036F\\u203F-\\u2040'; - -export const VALID_ATTRIBUTE_NAME_REGEX: RegExp = new RegExp( - '^[' + ATTRIBUTE_NAME_START_CHAR + '][' + ATTRIBUTE_NAME_CHAR + ']*$', -); - -const illegalAttributeNameCache: {[string]: boolean} = {}; -const validatedAttributeNameCache: {[string]: boolean} = {}; - -export function isAttributeNameSafe(attributeName: string): boolean { - if (hasOwnProperty.call(validatedAttributeNameCache, attributeName)) { - return true; - } - if (hasOwnProperty.call(illegalAttributeNameCache, attributeName)) { - return false; - } - if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) { - validatedAttributeNameCache[attributeName] = true; - return true; - } - illegalAttributeNameCache[attributeName] = true; - if (__DEV__) { - console.error('Invalid attribute name: `%s`', attributeName); - } - return false; -} - export function getPropertyInfo(name: string): PropertyInfo | null { return properties.hasOwnProperty(name) ? properties[name] : null; } // $FlowFixMe[missing-this-annot] function PropertyInfoRecord( - name: string, type: PropertyType, - mustUseProperty: boolean, attributeName: string, attributeNamespace: string | null, sanitizeURL: boolean, @@ -103,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; @@ -125,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 @@ -141,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 @@ -163,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 @@ -204,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 @@ -214,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. [ @@ -252,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 @@ -275,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 @@ -289,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 @@ -390,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 @@ -416,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 @@ -439,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 @@ -455,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 @@ -470,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 @@ -482,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 @@ -494,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/ReactDOMInvalidARIAHook.js b/packages/react-dom-bindings/src/shared/ReactDOMInvalidARIAHook.js index 2eab4ffdfb643..1546c32ed965e 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMInvalidARIAHook.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMInvalidARIAHook.js @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import {ATTRIBUTE_NAME_CHAR} from './DOMProperty'; +import {ATTRIBUTE_NAME_CHAR} from './isAttributeNameSafe'; import isCustomComponent from './isCustomComponent'; import validAriaProperties from './validAriaProperties'; import hasOwnProperty from 'shared/hasOwnProperty'; diff --git a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js index 91ac166d13760..ca469bc792690 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js @@ -5,7 +5,8 @@ * LICENSE file in the root directory of this source tree. */ -import {ATTRIBUTE_NAME_CHAR, BOOLEAN, getPropertyInfo} from './DOMProperty'; +import {BOOLEAN, getPropertyInfo} from './DOMProperty'; +import {ATTRIBUTE_NAME_CHAR} from './isAttributeNameSafe'; import isCustomComponent from './isCustomComponent'; import possibleStandardNames from './possibleStandardNames'; import hasOwnProperty from 'shared/hasOwnProperty'; @@ -180,75 +181,95 @@ function validateProperty(tagName, name, value, eventRegistry) { } } - if (typeof value === '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 { + switch (typeof value) { + case 'boolean': { + 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; + } + } + } + case 'function': + case 'symbol': // eslint-disable-line + // Warn when a known attribute is a bad type + warnedProperties[name] = true; + return false; + case 'string': { + // Warn when passing the strings 'false' or 'true' into a boolean prop + 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 `%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.', + 'Received the string `%s` for the boolean attribute `%s`. ' + + '%s ' + + 'Did you mean %s={%s}?', value, name, + value === 'false' + ? 'The browser will interpret it as a truthy value.' + : 'Although this works, it will not work as expected if you pass the string "false".', name, value, - name, - name, - name, ); + warnedProperties[name] = true; + return true; } } - warnedProperties[name] = true; - return true; - } - - // Warn when a known attribute is a bad type - switch (typeof value) { - case 'function': - case 'symbol': // eslint-disable-line - warnedProperties[name] = true; - return false; } - - // Warn when passing the strings 'false' or 'true' into a boolean prop - if ( - (value === 'false' || value === 'true') && - propertyInfo !== null && - propertyInfo.type === BOOLEAN - ) { - console.error( - 'Received the string `%s` for the boolean attribute `%s`. ' + - '%s ' + - 'Did you mean %s={%s}?', - value, - name, - value === 'false' - ? 'The browser will interpret it as a truthy value.' - : 'Although this works, it will not work as expected if you pass the string "false".', - name, - value, - ); - warnedProperties[name] = true; - return true; - } - return true; } } diff --git a/packages/react-dom-bindings/src/shared/isAttributeNameSafe.js b/packages/react-dom-bindings/src/shared/isAttributeNameSafe.js new file mode 100644 index 0000000000000..d1378a64de37c --- /dev/null +++ b/packages/react-dom-bindings/src/shared/isAttributeNameSafe.js @@ -0,0 +1,42 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import hasOwnProperty from 'shared/hasOwnProperty'; + +/* eslint-disable max-len */ +const ATTRIBUTE_NAME_START_CHAR = + ':A-Z_a-z\\u00C0-\\u00D6\\u00D8-\\u00F6\\u00F8-\\u02FF\\u0370-\\u037D\\u037F-\\u1FFF\\u200C-\\u200D\\u2070-\\u218F\\u2C00-\\u2FEF\\u3001-\\uD7FF\\uF900-\\uFDCF\\uFDF0-\\uFFFD'; +/* eslint-enable max-len */ +export const ATTRIBUTE_NAME_CHAR: string = + ATTRIBUTE_NAME_START_CHAR + '\\-.0-9\\u00B7\\u0300-\\u036F\\u203F-\\u2040'; + +const VALID_ATTRIBUTE_NAME_REGEX: RegExp = new RegExp( + '^[' + ATTRIBUTE_NAME_START_CHAR + '][' + ATTRIBUTE_NAME_CHAR + ']*$', +); + +const illegalAttributeNameCache: {[string]: boolean} = {}; +const validatedAttributeNameCache: {[string]: boolean} = {}; + +export default function isAttributeNameSafe(attributeName: string): boolean { + if (hasOwnProperty.call(validatedAttributeNameCache, attributeName)) { + return true; + } + if (hasOwnProperty.call(illegalAttributeNameCache, attributeName)) { + return false; + } + if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) { + validatedAttributeNameCache[attributeName] = true; + return true; + } + illegalAttributeNameCache[attributeName] = true; + if (__DEV__) { + console.error('Invalid attribute name: `%s`', attributeName); + } + return false; +} diff --git a/packages/react-dom-bindings/src/shared/isUnitlessNumber.js b/packages/react-dom-bindings/src/shared/isUnitlessNumber.js new file mode 100644 index 0000000000000..334111fa7a385 --- /dev/null +++ b/packages/react-dom-bindings/src/shared/isUnitlessNumber.js @@ -0,0 +1,90 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +/** + * CSS properties which accept numbers but are not in units of "px". + */ +export default function (name: string): boolean { + switch (name) { + case 'animationIterationCount': + case 'aspectRatio': + case 'borderImageOutset': + case 'borderImageSlice': + case 'borderImageWidth': + case 'boxFlex': + case 'boxFlexGroup': + case 'boxOrdinalGroup': + case 'columnCount': + case 'columns': + case 'flex': + case 'flexGrow': + case 'flexPositive': + case 'flexShrink': + case 'flexNegative': + case 'flexOrder': + case 'gridArea': + case 'gridRow': + case 'gridRowEnd': + case 'gridRowSpan': + case 'gridRowStart': + case 'gridColumn': + case 'gridColumnEnd': + case 'gridColumnSpan': + case 'gridColumnStart': + case 'fontWeight': + case 'lineClamp': + case 'lineHeight': + case 'opacity': + case 'order': + case 'orphans': + case 'scale': + case 'tabSize': + case 'widows': + case 'zIndex': + case 'zoom': + case 'fillOpacity': // SVG-related properties + case 'floodOpacity': + case 'stopOpacity': + case 'strokeDasharray': + case 'strokeDashoffset': + case 'strokeMiterlimit': + case 'strokeOpacity': + case 'strokeWidth': + case 'MozAnimationIterationCount': // Known Prefixed Properties + case 'MozBoxFlex': // TODO: Remove these since they shouldn't be used in modern code + case 'MozBoxFlexGroup': + case 'MozLineClamp': + case 'msAnimationIterationCount': + case 'msFlex': + case 'msZoom': + case 'msFlexGrow': + case 'msFlexNegative': + case 'msFlexOrder': + case 'msFlexPositive': + case 'msFlexShrink': + case 'msGridColumn': + case 'msGridColumnSpan': + case 'msGridRow': + case 'msGridRowSpan': + case 'WebkitAnimationIterationCount': + case 'WebkitBoxFlex': + case 'WebKitBoxFlexGroup': + case 'WebkitBoxOrdinalGroup': + case 'WebkitColumnCount': + case 'WebkitColumns': + case 'WebkitFlex': + case 'WebkitFlexGrow': + case 'WebkitFlexPositive': + case 'WebkitFlexShrink': + case 'WebkitLineClamp': + return true; + default: + return false; + } +} 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', () => {