From af36bfa6ed80946b4e5ed4bf2aab763aeed756c0 Mon Sep 17 00:00:00 2001 From: Nate Hunzaker Date: Fri, 8 Apr 2016 11:07:50 -0400 Subject: [PATCH 01/49] Allow custom attributes. Add flag to toggle custom attribute behavior --- .../dom/fiber/ReactDOMFiberComponent.js | 51 +++++----- .../dom/shared/DOMMarkupOperations.js | 2 +- src/renderers/dom/shared/DOMProperty.js | 48 ++++++++++ .../dom/shared/DOMPropertyOperations.js | 4 +- .../dom/shared/ReactDOMFeatureFlags.js | 1 + .../__tests__/ReactDOMComponent-test.js | 92 +++++++++++++++---- .../ReactDOMServerIntegration-test.js | 20 +++- .../hooks/ReactDOMUnknownPropertyHook.js | 73 ++++++--------- .../dom/stack/client/ReactDOMComponent.js | 21 +---- 9 files changed, 194 insertions(+), 118 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index b31104d479c28..4487c2ae087b4 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -231,10 +231,7 @@ function setInitialDOMProperties( } } else if (isCustomComponentTag) { DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp); - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey) - ) { + } else if (DOMProperty.isWriteableAttribute(propKey)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -275,10 +272,7 @@ function updateDOMProperties( } else { DOMPropertyOperations.deleteValueForAttribute(domElement, propKey); } - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey) - ) { + } else if (DOMProperty.isWriteableAttribute(propKey)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -1013,28 +1007,27 @@ var ReactDOMFiberComponent = { if (expectedStyle !== serverValue) { warnForPropDifference(propKey, serverValue, expectedStyle); } - } else if ( - isCustomComponentTag || - DOMProperty.isCustomAttribute(propKey) - ) { - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey); - serverValue = DOMPropertyOperations.getValueForAttribute( - domElement, - propKey, - nextProp, - ); - if (nextProp !== serverValue) { - warnForPropDifference(propKey, serverValue, nextProp); + } else if (DOMProperty.isWriteableAttribute(propKey)) { + propertyInfo = DOMProperty.properties[propKey]; + + if (!isCustomComponentTag && propertyInfo) { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propertyInfo.attributeName); + serverValue = DOMPropertyOperations.getValueForProperty( + domElement, + propKey, + nextProp, + ); + } else { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propKey); + serverValue = DOMPropertyOperations.getValueForAttribute( + domElement, + propKey, + nextProp, + ); } - } else if ((propertyInfo = DOMProperty.properties[propKey])) { - // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propertyInfo.attributeName); - serverValue = DOMPropertyOperations.getValueForProperty( - domElement, - propKey, - nextProp, - ); + if (nextProp !== serverValue) { warnForPropDifference(propKey, serverValue, nextProp); } diff --git a/src/renderers/dom/shared/DOMMarkupOperations.js b/src/renderers/dom/shared/DOMMarkupOperations.js index 11b9d9be11a5c..ee34594ffec65 100644 --- a/src/renderers/dom/shared/DOMMarkupOperations.js +++ b/src/renderers/dom/shared/DOMMarkupOperations.js @@ -103,7 +103,7 @@ var DOMMarkupOperations = { return attributeName + '=""'; } return attributeName + '=' + quoteAttributeValueForBrowser(value); - } else if (DOMProperty.isCustomAttribute(name)) { + } else if (DOMProperty.isWriteableAttribute(name)) { if (value == null) { return ''; } diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 793df14dd773f..e831c21de31b2 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -11,8 +11,24 @@ 'use strict'; +var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var invariant = require('fbjs/lib/invariant'); +var RESERVED_PROPS = { + children: true, + dangerouslySetInnerHTML: true, + key: true, + ref: true, + + autoFocus: true, + defaultValue: true, + defaultChecked: true, + innerHTML: true, + suppressContentEditableWarning: true, + onFocusIn: true, + onFocusOut: true, +}; + function checkMask(value, bitmask) { return (value & bitmask) === bitmask; } @@ -226,6 +242,38 @@ var DOMProperty = { return false; }, + /** + * Checks whether a property name is a writeable attribute. + * @method + */ + isWriteableAttribute: function(attributeName) { + if (DOMProperty.isReservedProp(attributeName)) { + return false; + } + + if ( + ReactDOMFeatureFlags.allowCustomAttributes || + DOMProperty.properties[attributeName] + ) { + return true; + } + + return this.isCustomAttribute(attributeName); + }, + + /** + * Checks to see if a property name is within the list of properties + * reserved for internal React operations. These properties should + * not be set on an HTML element. + * + * @private + * @param {string} name + * @return {boolean} If the name is within reserved props + */ + isReservedProp(name) { + return RESERVED_PROPS.hasOwnProperty(name); + }, + injection: DOMPropertyInjection, }; diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 46846b3d3a868..c49fe716fa19b 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -194,7 +194,7 @@ var DOMPropertyOperations = { node.setAttribute(attributeName, '' + value); } } - } else if (DOMProperty.isCustomAttribute(name)) { + } else if (DOMProperty.isWriteableAttribute(name)) { DOMPropertyOperations.setValueForAttribute(node, name, value); return; } @@ -272,7 +272,7 @@ var DOMPropertyOperations = { } else { node.removeAttribute(propertyInfo.attributeName); } - } else if (DOMProperty.isCustomAttribute(name)) { + } else if (DOMProperty.isWriteableAttribute(name)) { node.removeAttribute(name); } diff --git a/src/renderers/dom/shared/ReactDOMFeatureFlags.js b/src/renderers/dom/shared/ReactDOMFeatureFlags.js index bb24b4a12fd7f..07f3d71bf2868 100644 --- a/src/renderers/dom/shared/ReactDOMFeatureFlags.js +++ b/src/renderers/dom/shared/ReactDOMFeatureFlags.js @@ -14,6 +14,7 @@ var ReactDOMFeatureFlags = { fiberAsyncScheduling: false, useFiber: true, + allowCustomAttributes: true, }; module.exports = ReactDOMFeatureFlags; diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index f07cfcc9d7309..bd5b5622968a0 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -142,27 +142,29 @@ describe('ReactDOMComponent', () => { expectDev(() => (style.position = 'absolute')).toThrow(); }); - it('should warn for unknown prop', () => { - spyOn(console, 'error'); - var container = document.createElement('div'); - ReactDOM.render(
, container); - expectDev(console.error.calls.count(0)).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown prop `foo` on
tag. Remove this prop from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', - ); - }); + if (ReactDOMFeatureFlags.allowCustomAttributes !== true) { + it('should warn for unknown prop', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + ReactDOM.render(
, container); + expectDev(console.error.calls.count(0)).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Unknown prop `foo` on
tag. Remove this prop from the element. ' + + 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + ); + }); - it('should group multiple unknown prop warnings together', () => { - spyOn(console, 'error'); - var container = document.createElement('div'); - ReactDOM.render(
, container); - expectDev(console.error.calls.count(0)).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', - ); - }); + it('should group multiple unknown prop warnings together', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + ReactDOM.render(
, container); + expectDev(console.error.calls.count(0)).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + + 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + ); + }); + } it('should warn for onDblClick prop', () => { spyOn(console, 'error'); @@ -1874,4 +1876,54 @@ describe('ReactDOMComponent', () => { expect(container.firstChild.innerHTML).toBe(html2); }); }); + + describe('allowCustomAttributes feature flag', function() { + const originalValue = ReactDOMFeatureFlags.allowCustomAttributes; + + afterEach(function() { + ReactDOMFeatureFlags.allowCustomAttributes = originalValue; + }); + + describe('when set to false', function() { + beforeEach(function() { + ReactDOMFeatureFlags.allowCustomAttributes = false; + }); + + it('does not allow assignment of custom attributes to elements', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('whatever')).toBe(false); + + expect(console.error).toHaveBeenCalledTimes(1); + }); + + it('allows data attributes', function() { + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('data-whatever')).toBe(true); + }); + }); + + describe('when set to true', function() { + beforeEach(function() { + ReactDOMFeatureFlags.allowCustomAttributes = true; + }); + + it('allows assignment of custom attributes to elements', function() { + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('whatever')).toBe(true); + }); + + it('warns on bad casing', function() { + spyOn(console, 'error'); + + ReactTestUtils.renderIntoDocument(
); + + expect(console.error).toHaveBeenCalledTimes(1); + }); + }); + }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 2f7c137cbb161..85ddfefdfdb5f 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -779,9 +779,14 @@ describe('ReactDOMServerIntegration', () => { }); describe('unknown attributes', function() { - itRenders('no unknown attributes', async render => { - const e = await render(
, 1); - expect(e.getAttribute('foo')).toBe(null); + itRenders('unknown attributes', async render => { + const e = await render( +
, + ReactDOMFeatureFlags.allowCustomAttributes ? 0 : 1, + ); + expect(e.getAttribute('foo')).toBe( + ReactDOMFeatureFlags.allowCustomAttributes ? 'bar' : null, + ); }); itRenders('unknown data- attributes', async render => { @@ -797,8 +802,13 @@ describe('ReactDOMServerIntegration', () => { itRenders( 'no unknown attributes for non-standard elements', async render => { - const e = await render(, 1); - expect(e.getAttribute('foo')).toBe(null); + const e = await render( + , + ReactDOMFeatureFlags.allowCustomAttributes ? 0 : 1, + ); + expect(e.getAttribute('foo')).toBe( + ReactDOMFeatureFlags.allowCustomAttributes ? 'bar' : null, + ); }, ); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index e68bf66d913fe..720870843862b 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -35,39 +35,22 @@ function getStackAddendum(debugID) { } if (__DEV__) { - var reactProps = { - children: true, - dangerouslySetInnerHTML: true, - key: true, - ref: true, - - autoFocus: true, - defaultValue: true, - defaultChecked: true, - innerHTML: true, - suppressContentEditableWarning: true, - onFocusIn: true, - onFocusOut: true, - }; var warnedProperties = {}; var EVENT_NAME_REGEX = /^on[A-Z]/; var validateProperty = function(tagName, name, debugID) { - if ( - DOMProperty.properties.hasOwnProperty(name) || - DOMProperty.isCustomAttribute(name) - ) { - return true; - } - if ( - (reactProps.hasOwnProperty(name) && reactProps[name]) || - (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) - ) { + var lowerCasedName = name.toLowerCase(); + + if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { return true; } + + warnedProperties[name] = true; + if (EventPluginRegistry.registrationNameModules.hasOwnProperty(name)) { return true; } + if ( EventPluginRegistry.plugins.length === 0 && EVENT_NAME_REGEX.test(name) @@ -76,15 +59,6 @@ if (__DEV__) { // Don't check events in this case. return true; } - warnedProperties[name] = true; - var lowerCasedName = name.toLowerCase(); - - // data-* attributes should be lowercase; suggest the lowercase version - var standardName = DOMProperty.isCustomAttribute(lowerCasedName) - ? lowerCasedName - : DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName) - ? DOMProperty.getPossibleStandardName[lowerCasedName] - : null; var registrationName = EventPluginRegistry.possibleRegistrationNames.hasOwnProperty( lowerCasedName, @@ -92,31 +66,40 @@ if (__DEV__) { ? EventPluginRegistry.possibleRegistrationNames[lowerCasedName] : null; - if (standardName != null) { + if (registrationName != null) { warning( false, - 'Unknown DOM property %s. Did you mean %s?%s', + 'Unknown event handler property %s. Did you mean `%s`?%s', name, - standardName, + registrationName, getStackAddendum(debugID), ); return true; - } else if (registrationName != null) { + } + + // data-* attributes should be lowercase; suggest the lowercase version + var standardName = DOMProperty.getPossibleStandardName.hasOwnProperty(name) + ? DOMProperty.getPossibleStandardName[name] + : null; + + var hasBadCasing = standardName != null && standardName !== name; + + if (DOMProperty.isWriteableAttribute(name) && !hasBadCasing) { + return true; + } + + if (standardName != null) { warning( false, - 'Unknown event handler property %s. Did you mean `%s`?%s', + 'Unknown DOM property %s. Did you mean %s?%s', name, - registrationName, + standardName, getStackAddendum(debugID), ); return true; - } else { - // We were unable to guess which prop the user intended. - // It is likely that the user was just blindly spreading/forwarding props - // Components should be careful to only render valid props/attributes. - // Warning will be invoked in warnUnknownProperties to allow grouping. - return false; } + + return DOMProperty.isReservedProp(name); }; } diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 5453df69b531e..93fce96de1e4a 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -57,11 +57,6 @@ var CONTENT_TYPES = {string: true, number: true}; var STYLE = 'style'; var HTML = '__html'; -var RESERVED_PROPS = { - children: null, - dangerouslySetInnerHTML: null, - suppressContentEditableWarning: null, -}; function getDeclarationErrorAddendum(internalInstance) { if (internalInstance) { @@ -710,7 +705,7 @@ ReactDOMComponent.Mixin = { } var markup = null; if (this._tag != null && isCustomComponent(this._tag, props)) { - if (!RESERVED_PROPS.hasOwnProperty(propKey)) { + if (!DOMProperty.isReservedProp(propKey)) { markup = DOMMarkupOperations.createMarkupForCustomAttribute( propKey, propValue, @@ -966,13 +961,10 @@ ReactDOMComponent.Mixin = { } else if (registrationNameModules.hasOwnProperty(propKey)) { // Do nothing for event names. } else if (isCustomComponent(this._tag, lastProps)) { - if (!RESERVED_PROPS.hasOwnProperty(propKey)) { + if (!DOMProperty.isReservedProp(propKey)) { DOMPropertyOperations.deleteValueForAttribute(getNode(this), propKey); } - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey) - ) { + } else if (DOMProperty.isWriteableAttribute(propKey)) { DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey); } } @@ -1022,17 +1014,14 @@ ReactDOMComponent.Mixin = { ensureListeningTo(this, propKey, transaction); } } else if (isCustomComponentTag) { - if (!RESERVED_PROPS.hasOwnProperty(propKey)) { + if (!DOMProperty.isReservedProp(propKey)) { DOMPropertyOperations.setValueForAttribute( getNode(this), propKey, nextProp, ); } - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey) - ) { + } else if (DOMProperty.isWriteableAttribute(propKey)) { var node = getNode(this); // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This From 601eabd683a72216ec8ba6e06f48ae4107f90b3d Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:05:26 -0400 Subject: [PATCH 02/49] Update custom attribute logic - Only allow string attributes - Remove custom attribute feature flag - Add additional tests for data, aria, and custom attributes --- .../dom/fiber/ReactDOMFiberComponent.js | 6 +- .../dom/shared/DOMMarkupOperations.js | 2 +- src/renderers/dom/shared/DOMProperty.js | 21 +-- .../dom/shared/DOMPropertyOperations.js | 4 +- .../dom/shared/ReactDOMFeatureFlags.js | 1 - .../__tests__/ReactDOMComponent-test.js | 145 ++++++++++++------ .../ReactDOMServerIntegration-test.js | 48 +----- .../hooks/ReactDOMUnknownPropertyHook.js | 6 +- .../dom/stack/client/ReactDOMComponent.js | 10 +- 9 files changed, 122 insertions(+), 121 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 4487c2ae087b4..6e34b41af2d70 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -231,7 +231,7 @@ function setInitialDOMProperties( } } else if (isCustomComponentTag) { DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp); - } else if (DOMProperty.isWriteableAttribute(propKey)) { + } else if (DOMProperty.isWriteable(propKey, nextProp)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -272,7 +272,7 @@ function updateDOMProperties( } else { DOMPropertyOperations.deleteValueForAttribute(domElement, propKey); } - } else if (DOMProperty.isWriteableAttribute(propKey)) { + } else if (DOMProperty.isWriteable(propKey, propValue)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -1007,7 +1007,7 @@ var ReactDOMFiberComponent = { if (expectedStyle !== serverValue) { warnForPropDifference(propKey, serverValue, expectedStyle); } - } else if (DOMProperty.isWriteableAttribute(propKey)) { + } else if (DOMProperty.isWriteable(propKey, nextProp)) { propertyInfo = DOMProperty.properties[propKey]; if (!isCustomComponentTag && propertyInfo) { diff --git a/src/renderers/dom/shared/DOMMarkupOperations.js b/src/renderers/dom/shared/DOMMarkupOperations.js index ee34594ffec65..280093d20001f 100644 --- a/src/renderers/dom/shared/DOMMarkupOperations.js +++ b/src/renderers/dom/shared/DOMMarkupOperations.js @@ -103,7 +103,7 @@ var DOMMarkupOperations = { return attributeName + '=""'; } return attributeName + '=' + quoteAttributeValueForBrowser(value); - } else if (DOMProperty.isWriteableAttribute(name)) { + } else if (DOMProperty.isWriteable(name, value)) { if (value == null) { return ''; } diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index e831c21de31b2..37078b5f81516 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -11,7 +11,6 @@ 'use strict'; -var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); var invariant = require('fbjs/lib/invariant'); var RESERVED_PROPS = { @@ -48,11 +47,6 @@ var DOMPropertyInjection = { * Inject some specialized knowledge about the DOM. This takes a config object * with the following properties: * - * isCustomAttribute: function that given an attribute name will return true - * if it can be inserted into the DOM verbatim. Useful for data-* or aria-* - * attributes where it's impossible to enumerate all of the possible - * attribute names, - * * Properties: object mapping DOM property name to one of the * DOMPropertyInjection constants or null. If your attribute isn't in here, * it won't get written to the DOM. @@ -246,19 +240,20 @@ var DOMProperty = { * Checks whether a property name is a writeable attribute. * @method */ - isWriteableAttribute: function(attributeName) { - if (DOMProperty.isReservedProp(attributeName)) { + isWriteable: function(name, value) { + if (DOMProperty.isReservedProp(name)) { return false; } - if ( - ReactDOMFeatureFlags.allowCustomAttributes || - DOMProperty.properties[attributeName] - ) { + if (value == null || DOMProperty.properties.hasOwnProperty(name)) { + return true; + } + + if (DOMProperty.isCustomAttribute(name)) { return true; } - return this.isCustomAttribute(attributeName); + return typeof value === 'string'; }, /** diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index c49fe716fa19b..4e1fb7132281e 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -194,7 +194,7 @@ var DOMPropertyOperations = { node.setAttribute(attributeName, '' + value); } } - } else if (DOMProperty.isWriteableAttribute(name)) { + } else if (DOMProperty.isWriteable(name, value)) { DOMPropertyOperations.setValueForAttribute(node, name, value); return; } @@ -272,7 +272,7 @@ var DOMPropertyOperations = { } else { node.removeAttribute(propertyInfo.attributeName); } - } else if (DOMProperty.isWriteableAttribute(name)) { + } else { node.removeAttribute(name); } diff --git a/src/renderers/dom/shared/ReactDOMFeatureFlags.js b/src/renderers/dom/shared/ReactDOMFeatureFlags.js index 07f3d71bf2868..bb24b4a12fd7f 100644 --- a/src/renderers/dom/shared/ReactDOMFeatureFlags.js +++ b/src/renderers/dom/shared/ReactDOMFeatureFlags.js @@ -14,7 +14,6 @@ var ReactDOMFeatureFlags = { fiberAsyncScheduling: false, useFiber: true, - allowCustomAttributes: true, }; module.exports = ReactDOMFeatureFlags; diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index bd5b5622968a0..94d550c1e2379 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -142,29 +142,27 @@ describe('ReactDOMComponent', () => { expectDev(() => (style.position = 'absolute')).toThrow(); }); - if (ReactDOMFeatureFlags.allowCustomAttributes !== true) { - it('should warn for unknown prop', () => { - spyOn(console, 'error'); - var container = document.createElement('div'); - ReactDOM.render(
, container); - expectDev(console.error.calls.count(0)).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown prop `foo` on
tag. Remove this prop from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', - ); - }); + it('should warn for unknown prop', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + ReactDOM.render(
{}} />, container); + expectDev(console.error.calls.count(0)).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Unknown prop `foo` on
tag. Remove this prop from the element. ' + + 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + ); + }); - it('should group multiple unknown prop warnings together', () => { - spyOn(console, 'error'); - var container = document.createElement('div'); - ReactDOM.render(
, container); - expectDev(console.error.calls.count(0)).toBe(1); - expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', - ); - }); - } + it('should group multiple unknown prop warnings together', () => { + spyOn(console, 'error'); + var container = document.createElement('div'); + ReactDOM.render(
{}} baz={() => {}} />, container); + expectDev(console.error.calls.count(0)).toBe(1); + expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + + 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + ); + }); it('should warn for onDblClick prop', () => { spyOn(console, 'error'); @@ -1877,52 +1875,101 @@ describe('ReactDOMComponent', () => { }); }); - describe('allowCustomAttributes feature flag', function() { - const originalValue = ReactDOMFeatureFlags.allowCustomAttributes; + describe('Custom attributes', function() { + it('allows assignment of custom attributes with string values', function() { + var el = ReactTestUtils.renderIntoDocument(
); - afterEach(function() { - ReactDOMFeatureFlags.allowCustomAttributes = originalValue; + expect(el.hasAttribute('whatever')).toBe(true); }); - describe('when set to false', function() { - beforeEach(function() { - ReactDOMFeatureFlags.allowCustomAttributes = false; - }); + it('removes custom attributes', function() { + const container = document.createElement('div'); - it('does not allow assignment of custom attributes to elements', function() { - spyOn(console, 'error'); + ReactDOM.render(
, container); + + expect(container.firstChild.getAttribute('whatever')).toBe('30'); + + ReactDOM.render(
, container); + + expect(container.firstChild.hasAttribute('whatever')).toBe(false); + }); + + it('will not assign a boolean custom attributes', function() { + spyOn(console, 'error'); - var el = ReactTestUtils.renderIntoDocument(
); + var el = ReactTestUtils.renderIntoDocument(
); - expect(el.hasAttribute('whatever')).toBe(false); + expect(el.hasAttribute('whatever')).toBe(false); - expect(console.error).toHaveBeenCalledTimes(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Unknown prop `whatever` on
tag', + ); + }); + + it('will not assign a numeric custom attributes', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('whatever')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Unknown prop `whatever` on
tag', + ); + }); + + it('will not assign an implicit boolean custom attributes', function() { + spyOn(console, 'error'); + + // eslint-disable-next-line react/jsx-boolean-value + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('whatever')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Unknown prop `whatever` on
tag', + ); + }); + + describe('data attributes', function() { + it('allows boolean data-attributes', function() { + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('data-test')).toBe('true'); }); - it('allows data attributes', function() { - var el = ReactTestUtils.renderIntoDocument(
); + it('allows numeric data-attributes', function() { + var el = ReactTestUtils.renderIntoDocument(
); - expect(el.hasAttribute('data-whatever')).toBe(true); + expect(el.getAttribute('data-test')).toBe('1'); }); - }); - describe('when set to true', function() { - beforeEach(function() { - ReactDOMFeatureFlags.allowCustomAttributes = true; + it('allows implicit boolean data-attributes', function() { + // eslint-disable-next-line react/jsx-boolean-value + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('data-test')).toBe('true'); }); + }); - it('allows assignment of custom attributes to elements', function() { - var el = ReactTestUtils.renderIntoDocument(
); + describe('aria attributes', function() { + it('allows boolean aria-attributes', function() { + var el = ReactTestUtils.renderIntoDocument(
); - expect(el.hasAttribute('whatever')).toBe(true); + expect(el.getAttribute('aria-hidden')).toBe('true'); }); - it('warns on bad casing', function() { - spyOn(console, 'error'); + it('allows numeric aria-attributes', function() { + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('aria-hidden')).toBe('1'); + }); - ReactTestUtils.renderIntoDocument(
); + it('allows implicit boolean aria-attributes', function() { + // eslint-disable-next-line react/jsx-boolean-value + var el = ReactTestUtils.renderIntoDocument(
); - expect(console.error).toHaveBeenCalledTimes(1); + expect(el.getAttribute('aria-hidden')).toBe('true'); }); }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 85ddfefdfdb5f..8b50986a328b8 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -780,13 +780,8 @@ describe('ReactDOMServerIntegration', () => { describe('unknown attributes', function() { itRenders('unknown attributes', async render => { - const e = await render( -
, - ReactDOMFeatureFlags.allowCustomAttributes ? 0 : 1, - ); - expect(e.getAttribute('foo')).toBe( - ReactDOMFeatureFlags.allowCustomAttributes ? 'bar' : null, - ); + const e = await render(
, 0); + expect(e.getAttribute('foo')).toBe('bar'); }); itRenders('unknown data- attributes', async render => { @@ -802,13 +797,8 @@ describe('ReactDOMServerIntegration', () => { itRenders( 'no unknown attributes for non-standard elements', async render => { - const e = await render( - , - ReactDOMFeatureFlags.allowCustomAttributes ? 0 : 1, - ); - expect(e.getAttribute('foo')).toBe( - ReactDOMFeatureFlags.allowCustomAttributes ? 'bar' : null, - ); + const e = await render(, 0); + expect(e.getAttribute('foo')).toBe('bar'); }, ); @@ -2583,34 +2573,4 @@ describe('ReactDOMServerIntegration', () => {
"}} />, )); }); - - describe('dynamic injection', () => { - beforeEach(() => { - // HACK: we reset modules several times during the test which breaks - // dynamic injection. So we resort to telling resetModules() to run - // our custom init code every time after resetting. We could have a nicer - // way to do this, but this is the only test that needs it, and it will - // be removed anyway when we switch to static injection. - onAfterResetModules = () => { - const DOMProperty = require('DOMProperty'); - DOMProperty.injection.injectDOMPropertyConfig({ - isCustomAttribute: function(name) { - return name.indexOf('foo-') === 0; - }, - Properties: {foobar: null}, - }); - }; - resetModules(); - }); - - afterEach(() => { - onAfterResetModules = null; - }); - - itRenders('injected attributes', async render => { - const e = await render(
, 0); - expect(e.getAttribute('foobar')).toBe('simple'); - expect(e.getAttribute('foo-xyz')).toBe('simple'); - }); - }); }); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index 720870843862b..02e599f4435eb 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -38,7 +38,7 @@ if (__DEV__) { var warnedProperties = {}; var EVENT_NAME_REGEX = /^on[A-Z]/; - var validateProperty = function(tagName, name, debugID) { + var validateProperty = function(tagName, name, value, debugID) { var lowerCasedName = name.toLowerCase(); if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { @@ -84,7 +84,7 @@ if (__DEV__) { var hasBadCasing = standardName != null && standardName !== name; - if (DOMProperty.isWriteableAttribute(name) && !hasBadCasing) { + if (DOMProperty.isWriteable(name, value) && !hasBadCasing) { return true; } @@ -106,7 +106,7 @@ if (__DEV__) { var warnUnknownProperties = function(type, props, debugID) { var unknownProps = []; for (var key in props) { - var isValid = validateProperty(type, key, debugID); + var isValid = validateProperty(type, key, props[key], debugID); if (!isValid) { unknownProps.push(key); } diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 93fce96de1e4a..5561fe7b1856c 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -960,12 +960,12 @@ ReactDOMComponent.Mixin = { } } else if (registrationNameModules.hasOwnProperty(propKey)) { // Do nothing for event names. - } else if (isCustomComponent(this._tag, lastProps)) { - if (!DOMProperty.isReservedProp(propKey)) { + } else if (!DOMProperty.isReservedProp(propKey)) { + if (isCustomComponent(this._tag, lastProps)) { DOMPropertyOperations.deleteValueForAttribute(getNode(this), propKey); + } else { + DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey); } - } else if (DOMProperty.isWriteableAttribute(propKey)) { - DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey); } } for (propKey in nextProps) { @@ -1021,7 +1021,7 @@ ReactDOMComponent.Mixin = { nextProp, ); } - } else if (DOMProperty.isWriteableAttribute(propKey)) { + } else if (DOMProperty.isWriteable(propKey, nextProp)) { var node = getNode(this); // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This From eab17b5b314dff27e7fa964f3d9febb46e3b4cda Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:23:48 -0400 Subject: [PATCH 03/49] Allow numbers and booleans custom attributes. Cut isCustomAttribute --- src/renderers/dom/shared/DOMProperty.js | 31 +------- .../dom/shared/HTMLDOMPropertyConfig.js | 3 - .../__tests__/ReactDOMComponent-test.js | 72 +++++-------------- 3 files changed, 20 insertions(+), 86 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 37078b5f81516..4eb9d921e81a1 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -74,12 +74,6 @@ var DOMPropertyInjection = { var DOMPropertyNames = domPropertyConfig.DOMPropertyNames || {}; var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {}; - if (domPropertyConfig.isCustomAttribute) { - DOMProperty._isCustomAttributeFunctions.push( - domPropertyConfig.isCustomAttribute, - ); - } - for (var propName in Properties) { invariant( !DOMProperty.properties.hasOwnProperty(propName), @@ -217,25 +211,6 @@ var DOMProperty = { */ getPossibleStandardName: __DEV__ ? {autofocus: 'autoFocus'} : null, - /** - * All of the isCustomAttribute() functions that have been injected. - */ - _isCustomAttributeFunctions: [], - - /** - * Checks whether a property name is a custom attribute. - * @method - */ - isCustomAttribute: function(attributeName) { - for (var i = 0; i < DOMProperty._isCustomAttributeFunctions.length; i++) { - var isCustomAttributeFn = DOMProperty._isCustomAttributeFunctions[i]; - if (isCustomAttributeFn(attributeName)) { - return true; - } - } - return false; - }, - /** * Checks whether a property name is a writeable attribute. * @method @@ -249,11 +224,9 @@ var DOMProperty = { return true; } - if (DOMProperty.isCustomAttribute(name)) { - return true; - } + let type = typeof value; - return typeof value === 'string'; + return type !== 'function' && type !== 'object'; }, /** diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 6c0f387873d1e..5d58b43325517 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -22,9 +22,6 @@ var HAS_OVERLOADED_BOOLEAN_VALUE = DOMProperty.injection.HAS_OVERLOADED_BOOLEAN_VALUE; var HTMLDOMPropertyConfig = { - isCustomAttribute: RegExp.prototype.test.bind( - new RegExp('^(data|aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$'), - ), Properties: { /** * Standard Properties diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 94d550c1e2379..ce9c7a4373c93 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1894,22 +1894,29 @@ describe('ReactDOMComponent', () => { expect(container.firstChild.hasAttribute('whatever')).toBe(false); }); - it('will not assign a boolean custom attributes', function() { - spyOn(console, 'error'); - + it('assigns a boolean custom attributes as a string', function() { var el = ReactTestUtils.renderIntoDocument(
); - expect(el.hasAttribute('whatever')).toBe(false); + expect(el.getAttribute('whatever')).toBe('true'); + }); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Unknown prop `whatever` on
tag', - ); + it('assigns an implicit boolean custom attributes as a string', function() { + // eslint-disable-next-line react/jsx-boolean-value + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('whatever')).toBe('true'); }); - it('will not assign a numeric custom attributes', function() { + it('assigns a numeric custom attributes as a string', function() { + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('whatever')).toBe('3'); + }); + + it('will not assign a function custom attributes', function() { spyOn(console, 'error'); - var el = ReactTestUtils.renderIntoDocument(
); + var el = ReactTestUtils.renderIntoDocument(
{}} />); expect(el.hasAttribute('whatever')).toBe(false); @@ -1918,11 +1925,10 @@ describe('ReactDOMComponent', () => { ); }); - it('will not assign an implicit boolean custom attributes', function() { + it('will not assign an object custom attributes', function() { spyOn(console, 'error'); - // eslint-disable-next-line react/jsx-boolean-value - var el = ReactTestUtils.renderIntoDocument(
); + var el = ReactTestUtils.renderIntoDocument(
); expect(el.hasAttribute('whatever')).toBe(false); @@ -1930,47 +1936,5 @@ describe('ReactDOMComponent', () => { 'Warning: Unknown prop `whatever` on
tag', ); }); - - describe('data attributes', function() { - it('allows boolean data-attributes', function() { - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('data-test')).toBe('true'); - }); - - it('allows numeric data-attributes', function() { - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('data-test')).toBe('1'); - }); - - it('allows implicit boolean data-attributes', function() { - // eslint-disable-next-line react/jsx-boolean-value - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('data-test')).toBe('true'); - }); - }); - - describe('aria attributes', function() { - it('allows boolean aria-attributes', function() { - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('aria-hidden')).toBe('true'); - }); - - it('allows numeric aria-attributes', function() { - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('aria-hidden')).toBe('1'); - }); - - it('allows implicit boolean aria-attributes', function() { - // eslint-disable-next-line react/jsx-boolean-value - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('aria-hidden')).toBe('true'); - }); - }); }); }); From 137af3b223d513d06fab4f287fec6be872965ab7 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:52:00 -0400 Subject: [PATCH 04/49] Cover objects with custom attributes in warning test --- src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index ce9c7a4373c93..688934980e51d 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -156,7 +156,7 @@ describe('ReactDOMComponent', () => { it('should group multiple unknown prop warnings together', () => { spyOn(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(
{}} baz={() => {}} />, container); + ReactDOM.render(
{}} baz={{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + From 55e55ba14619ceb9d4fb8c1afb5d6001da1ffc33 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:16:45 -0400 Subject: [PATCH 05/49] Rename DOMProperty.isWriteable to shouldSetAttribute --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 6 +++--- src/renderers/dom/shared/DOMMarkupOperations.js | 2 +- src/renderers/dom/shared/DOMProperty.js | 2 +- src/renderers/dom/shared/DOMPropertyOperations.js | 2 +- .../dom/shared/hooks/ReactDOMUnknownPropertyHook.js | 2 +- src/renderers/dom/stack/client/ReactDOMComponent.js | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 6e34b41af2d70..09ea515de1096 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -231,7 +231,7 @@ function setInitialDOMProperties( } } else if (isCustomComponentTag) { DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp); - } else if (DOMProperty.isWriteable(propKey, nextProp)) { + } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -272,7 +272,7 @@ function updateDOMProperties( } else { DOMPropertyOperations.deleteValueForAttribute(domElement, propKey); } - } else if (DOMProperty.isWriteable(propKey, propValue)) { + } else if (DOMProperty.shouldSetAttribute(propKey, propValue)) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. @@ -1007,7 +1007,7 @@ var ReactDOMFiberComponent = { if (expectedStyle !== serverValue) { warnForPropDifference(propKey, serverValue, expectedStyle); } - } else if (DOMProperty.isWriteable(propKey, nextProp)) { + } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { propertyInfo = DOMProperty.properties[propKey]; if (!isCustomComponentTag && propertyInfo) { diff --git a/src/renderers/dom/shared/DOMMarkupOperations.js b/src/renderers/dom/shared/DOMMarkupOperations.js index 280093d20001f..2e0a8ca762571 100644 --- a/src/renderers/dom/shared/DOMMarkupOperations.js +++ b/src/renderers/dom/shared/DOMMarkupOperations.js @@ -103,7 +103,7 @@ var DOMMarkupOperations = { return attributeName + '=""'; } return attributeName + '=' + quoteAttributeValueForBrowser(value); - } else if (DOMProperty.isWriteable(name, value)) { + } else if (DOMProperty.shouldSetAttribute(name, value)) { if (value == null) { return ''; } diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 4eb9d921e81a1..f0bcd4ec20b8f 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -215,7 +215,7 @@ var DOMProperty = { * Checks whether a property name is a writeable attribute. * @method */ - isWriteable: function(name, value) { + shouldSetAttribute: function(name, value) { if (DOMProperty.isReservedProp(name)) { return false; } diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 4e1fb7132281e..f09255c871bf2 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -194,7 +194,7 @@ var DOMPropertyOperations = { node.setAttribute(attributeName, '' + value); } } - } else if (DOMProperty.isWriteable(name, value)) { + } else if (DOMProperty.shouldSetAttribute(name, value)) { DOMPropertyOperations.setValueForAttribute(node, name, value); return; } diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index 02e599f4435eb..aa579f7338c5a 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -84,7 +84,7 @@ if (__DEV__) { var hasBadCasing = standardName != null && standardName !== name; - if (DOMProperty.isWriteable(name, value) && !hasBadCasing) { + if (DOMProperty.shouldSetAttribute(name, value) && !hasBadCasing) { return true; } diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 5561fe7b1856c..2fd7d8fe922c0 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -1021,7 +1021,7 @@ ReactDOMComponent.Mixin = { nextProp, ); } - } else if (DOMProperty.isWriteable(propKey, nextProp)) { + } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { var node = getNode(this); // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This From 21068448b3616d3edcc8194a7c78c2f9843b0a1f Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:27:28 -0400 Subject: [PATCH 06/49] Rework conditions in shouldSetProperty to avoid edge cases --- src/renderers/dom/shared/DOMProperty.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index f0bcd4ec20b8f..8638f1d652cfa 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -220,13 +220,24 @@ var DOMProperty = { return false; } - if (value == null || DOMProperty.properties.hasOwnProperty(name)) { + let type = typeof value; + + if ( + value === null || + type === 'undefined' || + DOMProperty.properties.hasOwnProperty(name) + ) { return true; } - let type = typeof value; - - return type !== 'function' && type !== 'object'; + switch (type) { + case 'boolean': + case 'number': + case 'string': + return true; + default: + return false; + } }, /** From 9caa863f57ef18b72863bf53ed441eb14a41b319 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:37:06 -0400 Subject: [PATCH 07/49] Update unknown property warning to include custom attribute information --- docs/warnings/unknown-prop.md | 2 +- .../dom/shared/__tests__/ReactDOMComponent-test.js | 12 ++++++++---- .../dom/shared/hooks/ReactDOMUnknownPropertyHook.js | 6 ++++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/docs/warnings/unknown-prop.md b/docs/warnings/unknown-prop.md index eb7585f650d14..86883cafcdca8 100644 --- a/docs/warnings/unknown-prop.md +++ b/docs/warnings/unknown-prop.md @@ -3,7 +3,7 @@ title: Unknown Prop Warning layout: single permalink: warnings/unknown-prop.html --- -The unknown-prop warning will fire if you attempt to render a DOM element with a prop that is not recognized by React as a legal DOM attribute/property. You should ensure that your DOM elements do not have spurious props floating around. +The unknown-prop warning will fire if you attempt to render a DOM element with a prop that is either unrecognized by React as a legal DOM attribute/property, or is not a string, number, or boolean value. You should ensure that your DOM elements do not have spurious props floating around. There are a couple of likely reasons this warning could be appearing: diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 688934980e51d..806b4f084d772 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -148,8 +148,10 @@ describe('ReactDOMComponent', () => { ReactDOM.render(
{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown prop `foo` on
tag. Remove this prop from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + 'Warning: Unknown prop `foo` on
tag. Either remove this prop ' + + 'from the element, or pass a string, number, or boolean value to keep ' + + 'it in the DOM. For details, see https://fb.me/react-unknown-prop' + + '\n in div (at **)', ); }); @@ -159,8 +161,10 @@ describe('ReactDOMComponent', () => { ReactDOM.render(
{}} baz={{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown props `foo`, `baz` on
tag. Remove these props from the element. ' + - 'For details, see https://fb.me/react-unknown-prop\n in div (at **)', + 'Warning: Unknown props `foo`, `baz` on
tag. Either remove these ' + + 'props from the element, or pass a string, number, or boolean value to keep ' + + 'them in the DOM. For details, see https://fb.me/react-unknown-prop' + + '\n in div (at **)', ); }); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index aa579f7338c5a..c2030ec73ed28 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -117,7 +117,8 @@ var warnUnknownProperties = function(type, props, debugID) { if (unknownProps.length === 1) { warning( false, - 'Unknown prop %s on <%s> tag. Remove this prop from the element. ' + + 'Unknown prop %s on <%s> tag. Either remove this prop from the element, ' + + 'or pass a string, number, or boolean value to keep it in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, type, @@ -126,7 +127,8 @@ var warnUnknownProperties = function(type, props, debugID) { } else if (unknownProps.length > 1) { warning( false, - 'Unknown props %s on <%s> tag. Remove these props from the element. ' + + 'Unknown props %s on <%s> tag. Either remove these props from the element, ' + + 'or pass a string, number, or boolean value to keep them in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, type, From 84beb33d7f58e33860567b4de262e524acc32979 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:57:22 -0400 Subject: [PATCH 08/49] Remove ref and key from reserved props --- src/renderers/dom/shared/DOMProperty.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 8638f1d652cfa..947f2ca26a9a8 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -16,9 +16,6 @@ var invariant = require('fbjs/lib/invariant'); var RESERVED_PROPS = { children: true, dangerouslySetInnerHTML: true, - key: true, - ref: true, - autoFocus: true, defaultValue: true, defaultChecked: true, From f406aac5264f68f2e01a530543d51992cc44bf53 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 06:40:10 -0400 Subject: [PATCH 09/49] Ensure SSR test coverage for DOMProperty injections --- .../ReactDOMServerIntegration-test.js | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 8b50986a328b8..6aab717a4a385 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -2573,4 +2573,34 @@ describe('ReactDOMServerIntegration', () => {
"}} />, )); }); + + describe('dynamic injection', () => { + beforeEach(() => { + // HACK: we reset modules several times during the test which breaks + // dynamic injection. So we resort to telling resetModules() to run + // our custom init code every time after resetting. We could have a nicer + // way to do this, but this is the only test that needs it, and it will + // be removed anyway when we switch to static injection. + onAfterResetModules = () => { + const DOMProperty = require('DOMProperty'); + DOMProperty.injection.injectDOMPropertyConfig({ + Properties: {foobar: 0}, + DOMAttributeNames: { + foobar: 'foo-bar', + }, + }); + }; + resetModules(); + }); + + afterEach(() => { + onAfterResetModules = null; + }); + + itRenders('injected attributes', async render => { + const e = await render(
); + + expect(e.getAttribute('foo-bar')).toEqual('test'); + }); + }); }); From da19306fb9ce06def2d6594b20479db5b5d354a8 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 07:04:11 -0400 Subject: [PATCH 10/49] Add ajaxify attribute for internal FB support --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 3 +++ .../dom/shared/__tests__/ReactDOMComponent-test.js | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 5d58b43325517..883c07540a845 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -202,6 +202,9 @@ var HTMLDOMPropertyConfig = { security: 0, // IE-only attribute that controls focus behavior unselectable: 0, + // Facebook internal attribute. Listed to avoid dependency on custom + // property injections + ajaxify: MUST_USE_PROPERTY, }, DOMAttributeNames: { acceptCharset: 'accept-charset', diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 806b4f084d772..7d5971524581a 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1940,5 +1940,12 @@ describe('ReactDOMComponent', () => { 'Warning: Unknown prop `whatever` on
tag', ); }); + + it('assigns ajaxify (an important internal FB attribute)', function() { + var options = {}; + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.ajaxify).toBe(options); + }); }); }); From f09d3f312c668e951dc18004dc5f44ec13cfcab7 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 10:03:46 -0400 Subject: [PATCH 11/49] Ajaxify is a stringifiable object attribute --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 6 +++--- .../dom/shared/__tests__/ReactDOMComponent-test.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 883c07540a845..e8525884e7d77 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -202,9 +202,9 @@ var HTMLDOMPropertyConfig = { security: 0, // IE-only attribute that controls focus behavior unselectable: 0, - // Facebook internal attribute. Listed to avoid dependency on custom - // property injections - ajaxify: MUST_USE_PROPERTY, + // Facebook internal attribute. This is an object attribute that + // impliments a custom toString() method + ajaxify: 0, }, DOMAttributeNames: { acceptCharset: 'accept-charset', diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 7d5971524581a..7d2ac8dcd4e3a 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1942,10 +1942,10 @@ describe('ReactDOMComponent', () => { }); it('assigns ajaxify (an important internal FB attribute)', function() { - var options = {}; + var options = {toString: () => 'ajaxy'}; var el = ReactTestUtils.renderIntoDocument(
); - expect(el.ajaxify).toBe(options); + expect(el.getAttribute('ajaxify')).toBe('ajaxy'); }); }); }); From aeb2db379afc51294096bd9bfec8a78d046f73e0 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 10:56:24 -0400 Subject: [PATCH 12/49] Update test name for custom attributes on custom elements --- .../dom/shared/__tests__/ReactDOMServerIntegration-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 6aab717a4a385..1df61bbbb7da2 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -795,7 +795,7 @@ describe('ReactDOMServerIntegration', () => { }); itRenders( - 'no unknown attributes for non-standard elements', + 'custom attributes for non-standard elements', async render => { const e = await render(, 0); expect(e.getAttribute('foo')).toBe('bar'); From 7cbf2f37d27c305fd511ce679609b9aa92e9654b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 11:43:56 -0400 Subject: [PATCH 13/49] Remove SSR custom injection test --- .../ReactDOMServerIntegration-test.js | 41 ++----------------- 1 file changed, 4 insertions(+), 37 deletions(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 1df61bbbb7da2..9bab6b179dfab 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -794,13 +794,10 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('data-foo')).toBe(false); }); - itRenders( - 'custom attributes for non-standard elements', - async render => { - const e = await render(, 0); - expect(e.getAttribute('foo')).toBe('bar'); - }, - ); + itRenders('custom attributes for non-standard elements', async render => { + const e = await render(, 0); + expect(e.getAttribute('foo')).toBe('bar'); + }); itRenders('unknown attributes for custom elements', async render => { const e = await render(); @@ -2573,34 +2570,4 @@ describe('ReactDOMServerIntegration', () => {
"}} />, )); }); - - describe('dynamic injection', () => { - beforeEach(() => { - // HACK: we reset modules several times during the test which breaks - // dynamic injection. So we resort to telling resetModules() to run - // our custom init code every time after resetting. We could have a nicer - // way to do this, but this is the only test that needs it, and it will - // be removed anyway when we switch to static injection. - onAfterResetModules = () => { - const DOMProperty = require('DOMProperty'); - DOMProperty.injection.injectDOMPropertyConfig({ - Properties: {foobar: 0}, - DOMAttributeNames: { - foobar: 'foo-bar', - }, - }); - }; - resetModules(); - }); - - afterEach(() => { - onAfterResetModules = null; - }); - - itRenders('injected attributes', async render => { - const e = await render(
); - - expect(e.getAttribute('foo-bar')).toEqual('test'); - }); - }); }); From b67dd1335cd95d8d52e39fdf99cfe37dbdb366f0 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:04:57 -0400 Subject: [PATCH 14/49] Remove onAfterResetModules hooks in SSR render tests --- .../shared/__tests__/ReactDOMServerIntegration-test.js | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 9bab6b179dfab..10c9db313b93f 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -312,10 +312,6 @@ function expectMarkupMismatch(serverElement, clientElement) { return testMarkupMatch(serverElement, clientElement, false); } -// TODO: this is a hack for testing dynamic injection. Remove this when we decide -// how to do static injection instead. -let onAfterResetModules = null; - // When there is a test that renders on server and then on client and expects a logged // error, you want to see the error show up both on server and client. Unfortunately, // React refuses to issue the same error twice to avoid clogging up the console. @@ -323,9 +319,6 @@ let onAfterResetModules = null; function resetModules() { // First, reset the modules to load the client renderer. jest.resetModuleRegistry(); - if (typeof onAfterResetModules === 'function') { - onAfterResetModules(); - } // TODO: can we express this test with only public API? ExecutionEnvironment = require('ExecutionEnvironment'); @@ -341,9 +334,6 @@ function resetModules() { // Resetting is important because we want to avoid any shared state // influencing the tests. jest.resetModuleRegistry(); - if (typeof onAfterResetModules === 'function') { - onAfterResetModules(); - } require('ReactFeatureFlags').disableNewFiberFeatures = false; ReactDOMServer = require('react-dom/server'); } From dab72d29d049ef143b64755145a73f721481ff3b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 16:51:41 -0400 Subject: [PATCH 15/49] Do not allow assignment of attributes that are aliased --- src/renderers/dom/shared/DOMProperty.js | 15 +++++++++- .../__tests__/ReactDOMComponent-test.js | 29 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 947f2ca26a9a8..3f268800572cc 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -118,6 +118,9 @@ var DOMPropertyInjection = { if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; + + DOMProperty.aliases[attributeName] = true; + propertyInfo.attributeName = attributeName; if (__DEV__) { DOMProperty.getPossibleStandardName[attributeName] = propName; @@ -197,6 +200,13 @@ var DOMProperty = { */ properties: {}, + /** + * Some attributes are aliased for easier use within React. We don't + * allow direct use of these attributes. See DOMAttributeNames in + * HTMLPropertyConfig and SVGPropertyConfig. + */ + aliases: {}, + /** * Mapping from lowercase property names to the properly cased version, used * to warn in the case of missing properties. Available only in __DEV__. @@ -213,7 +223,10 @@ var DOMProperty = { * @method */ shouldSetAttribute: function(name, value) { - if (DOMProperty.isReservedProp(name)) { + if ( + DOMProperty.isReservedProp(name) || + DOMProperty.aliases.hasOwnProperty(name) + ) { return false; } diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 7d2ac8dcd4e3a..af2cacc47ad2d 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1879,6 +1879,35 @@ describe('ReactDOMComponent', () => { }); }); + describe('Attributes with aliases', function() { + it('does not set aliased attributes on DOM elements', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.className).toBe(''); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Unknown DOM property class. Did you mean className?', + ); + }); + + it('does not set aliased attributes on SVG elements', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument( + , + ); + var text = el.querySelector('text'); + + expect(text.getAttribute('arabic-form')).toBe(null); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Unknown DOM property arabic-form. Did you mean arabicForm?', + ); + }); + }); + describe('Custom attributes', function() { it('allows assignment of custom attributes with string values', function() { var el = ReactTestUtils.renderIntoDocument(
); From 000c0df822147e2f908b0cf35e69314181fa060a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 16:56:06 -0400 Subject: [PATCH 16/49] Update custom attribute test to check value, not just presence --- src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index af2cacc47ad2d..e8211a8b43a20 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1912,7 +1912,7 @@ describe('ReactDOMComponent', () => { it('allows assignment of custom attributes with string values', function() { var el = ReactTestUtils.renderIntoDocument(
); - expect(el.hasAttribute('whatever')).toBe(true); + expect(el.getAttribute('whatever')).toBe("30"); }); it('removes custom attributes', function() { From a77d47bbf218c3153379a3d7a832ea4ae605cc60 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 18:01:22 -0400 Subject: [PATCH 17/49] Address case where class is assigned as an attribute on custom elements. Improve SSR tests --- .../dom/fiber/ReactDOMFiberComponent.js | 16 +++++++++-- .../__tests__/ReactDOMComponent-test.js | 28 ++++++++++++++++++- .../ReactDOMServerIntegration-test.js | 10 +++++++ 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 09ea515de1096..e421362413903 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1007,10 +1007,20 @@ var ReactDOMFiberComponent = { if (expectedStyle !== serverValue) { warnForPropDifference(propKey, serverValue, expectedStyle); } - } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { - propertyInfo = DOMProperty.properties[propKey]; + } else if (isCustomComponentTag) { + // $FlowFixMe - Should be inferred as not undefined. + extraAttributeNames.delete(propKey); + serverValue = DOMPropertyOperations.getValueForAttribute( + domElement, + propKey, + nextProp, + ); - if (!isCustomComponentTag && propertyInfo) { + if (nextProp !== serverValue) { + warnForPropDifference(propKey, serverValue, nextProp); + } + } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { + if ((propertyInfo = DOMProperty.properties[propKey])) { // $FlowFixMe - Should be inferred as not undefined. extraAttributeNames.delete(propertyInfo.attributeName); serverValue = DOMPropertyOperations.getValueForProperty( diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index e8211a8b43a20..574c4a0b576c1 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1906,13 +1906,39 @@ describe('ReactDOMComponent', () => { 'Warning: Unknown DOM property arabic-form. Did you mean arabicForm?', ); }); + + it('sets aliased attributes on custom elements', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument( +
, + ); + + expect(el.getAttribute('class')).toBe('test'); + + expectDev(console.error).not.toHaveBeenCalled(); + }); + + it('updates aliased attributes on custom elements', function() { + var container = document.createElement('div'); + + spyOn(console, 'error'); + + ReactDOM.render(
, container); + + ReactDOM.render(
, container); + + expect(container.firstChild.getAttribute('class')).toBe('bar'); + + expectDev(console.error).not.toHaveBeenCalled(); + }); }); describe('Custom attributes', function() { it('allows assignment of custom attributes with string values', function() { var el = ReactTestUtils.renderIntoDocument(
); - expect(el.getAttribute('whatever')).toBe("30"); + expect(el.getAttribute('whatever')).toBe('30'); }); it('removes custom attributes', function() { diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 10c9db313b93f..dcf416c6c3820 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -614,6 +614,16 @@ describe('ReactDOMServerIntegration', () => { const e = await render(
); expect(e.hasAttribute('className')).toBe(false); }); + + itRenders('no className prop when given the alias', async render => { + const e = await render(
, 1); + expect(e.className).toBe(''); + }); + + itRenders('class for custom elements', async render => { + const e = await render(, 0); + expect(e.getAttribute('class')).toBe('test'); + }); }); describe('htmlFor property', function() { From f661d227302dbb6a28b05b0592bb76006be3b688 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 18:36:53 -0400 Subject: [PATCH 18/49] Cover cases where className and for are given to custom elements --- .../dom/fiber/ReactDOMFiberComponent.js | 2 +- .../__tests__/ReactDOMServerIntegration-test.js | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index e421362413903..b0598829be26d 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1009,7 +1009,7 @@ var ReactDOMFiberComponent = { } } else if (isCustomComponentTag) { // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey); + extraAttributeNames.delete(propKey.toLowerCase()); serverValue = DOMPropertyOperations.getValueForAttribute( domElement, propKey, diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index dcf416c6c3820..b4d2bad6a12bd 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -621,9 +621,14 @@ describe('ReactDOMServerIntegration', () => { }); itRenders('class for custom elements', async render => { - const e = await render(, 0); + const e = await render(
, 0); expect(e.getAttribute('class')).toBe('test'); }); + + itRenders('className for custom elements', async render => { + const e = await render(
, 0); + expect(e.getAttribute('className')).toBe('test'); + }); }); describe('htmlFor property', function() { @@ -653,6 +658,16 @@ describe('ReactDOMServerIntegration', () => { const e = await render(
); expect(e.hasAttribute('htmlFor')).toBe(false); }); + + itRenders('htmlFor attribute on custom elements', async render => { + const e = await render(
); + expect(e.getAttribute('htmlFor')).toBe('test'); + }); + + itRenders('for attribute on custom elements', async render => { + const e = await render(
); + expect(e.getAttribute('for')).toBe('test'); + }); }); describe('numeric properties', function() { From aa3916bcba10f4810d8eee9a7d7ca9521c66ec95 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 09:29:41 -0400 Subject: [PATCH 19/49] Remove unnecessary spys on console.error. Reduce extra space in tests --- .../dom/shared/__tests__/ReactDOMComponent-test.js | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 574c4a0b576c1..fdf408341b0f5 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1908,29 +1908,19 @@ describe('ReactDOMComponent', () => { }); it('sets aliased attributes on custom elements', function() { - spyOn(console, 'error'); - var el = ReactTestUtils.renderIntoDocument(
, ); expect(el.getAttribute('class')).toBe('test'); - - expectDev(console.error).not.toHaveBeenCalled(); }); it('updates aliased attributes on custom elements', function() { var container = document.createElement('div'); - - spyOn(console, 'error'); - ReactDOM.render(
, container); - ReactDOM.render(
, container); expect(container.firstChild.getAttribute('class')).toBe('bar'); - - expectDev(console.error).not.toHaveBeenCalled(); }); }); @@ -1943,7 +1933,6 @@ describe('ReactDOMComponent', () => { it('removes custom attributes', function() { const container = document.createElement('div'); - ReactDOM.render(
, container); expect(container.firstChild.getAttribute('whatever')).toBe('30'); From 6ce33350d2508ba9a114aacf8ff70fdafafd9065 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 10:20:49 -0400 Subject: [PATCH 20/49] Cover cased custom attributes in SSR tests --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 2 +- .../dom/shared/__tests__/ReactDOMServerIntegration-test.js | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index b0598829be26d..11721488bc796 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1030,7 +1030,7 @@ var ReactDOMFiberComponent = { ); } else { // $FlowFixMe - Should be inferred as not undefined. - extraAttributeNames.delete(propKey); + extraAttributeNames.delete(propKey.toLowerCase()); serverValue = DOMPropertyOperations.getValueForAttribute( domElement, propKey, diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index b4d2bad6a12bd..4a26bfc84c4b9 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -842,6 +842,11 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('foo')).toBe(false); }, ); + + itRenders('custom attributes with special casing', async render => { + const e = await render(
); + expect(e.getAttribute('fooBar')).toBe('test'); + }); }); itRenders('no HTML events', async render => { From a11b0bd44bed8e214879429144b689fb9e4d46c1 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 14:31:20 -0400 Subject: [PATCH 21/49] Custom attributes are case sensitive --- src/renderers/dom/shared/DOMProperty.js | 17 +++--- .../__tests__/ReactDOMComponent-test.js | 58 ++++++++++++++----- .../ReactDOMServerIntegration-test.js | 11 +++- .../hooks/ReactDOMUnknownPropertyHook.js | 46 ++++++++++----- 4 files changed, 91 insertions(+), 41 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 3f268800572cc..a3641c932dfdd 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -230,17 +230,20 @@ var DOMProperty = { return false; } - let type = typeof value; + if (DOMProperty.properties.hasOwnProperty(name)) { + return true; + } - if ( - value === null || - type === 'undefined' || - DOMProperty.properties.hasOwnProperty(name) - ) { + if (value === null) { return true; } - switch (type) { + if (name.toLowerCase() !== name) { + return false; + } + + switch (typeof value) { + case 'undefined': case 'boolean': case 'number': case 'string': diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index fdf408341b0f5..74a9f1eef50bc 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -148,7 +148,7 @@ describe('ReactDOMComponent', () => { ReactDOM.render(
{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown prop `foo` on
tag. Either remove this prop ' + + 'Warning: Invalid prop `foo` on
tag. Either remove this prop ' + 'from the element, or pass a string, number, or boolean value to keep ' + 'it in the DOM. For details, see https://fb.me/react-unknown-prop' + '\n in div (at **)', @@ -161,7 +161,7 @@ describe('ReactDOMComponent', () => { ReactDOM.render(
{}} baz={{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown props `foo`, `baz` on
tag. Either remove these ' + + 'Warning: Invalid props `foo`, `baz` on
tag. Either remove these ' + 'props from the element, or pass a string, number, or boolean value to keep ' + 'them in the DOM. For details, see https://fb.me/react-unknown-prop' + '\n in div (at **)', @@ -174,7 +174,7 @@ describe('ReactDOMComponent', () => { ReactDOM.render(
{}} />, container); expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown event handler property onDblClick. Did you mean `onDoubleClick`?\n in div (at **)', + 'Warning: Unknown event handler property `onDblClick`. Did you mean `onDoubleClick`?\n in div (at **)', ); }); @@ -1615,10 +1615,10 @@ describe('ReactDOMComponent', () => { ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(2); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)', + 'Warning: Invalid DOM property `class`. Did you mean `className`?\n in div (at **)', ); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: Unknown event handler property onclick. Did you mean ' + + 'Warning: Unknown event handler property `onclick`. Did you mean ' + '`onClick`?\n in input (at **)', ); }); @@ -1629,10 +1629,10 @@ describe('ReactDOMComponent', () => { ReactDOMServer.renderToString(); expectDev(console.error.calls.count()).toBe(2); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)', + 'Warning: Invalid DOM property `class`. Did you mean `className`?\n in div (at **)', ); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe( - 'Warning: Unknown event handler property onclick. Did you mean ' + + 'Warning: Unknown event handler property `onclick`. Did you mean ' + '`onClick`?\n in input (at **)', ); }); @@ -1647,7 +1647,7 @@ describe('ReactDOMComponent', () => { ReactTestUtils.renderIntoDocument(
, container); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( - 'Warning: Unknown DOM property class. Did you mean className?\n in div (at **)', + 'Warning: Invalid DOM property `class`. Did you mean `className`?\n in div (at **)', ); }); @@ -1825,11 +1825,11 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count()).toBe(2); expectDev(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: Unknown DOM property for. Did you mean htmlFor?\n in label', + 'Warning: Invalid DOM property `for`. Did you mean `htmlFor`?\n in label', ); expectDev(console.error.calls.argsFor(1)[0]).toBe( - 'Warning: Unknown DOM property autofocus. Did you mean autoFocus?\n in input', + 'Warning: Invalid DOM property `autofocus`. Did you mean `autoFocus`?\n in input', ); }); @@ -1846,11 +1846,11 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count()).toBe(2); expectDev(console.error.calls.argsFor(0)[0]).toBe( - 'Warning: Unknown DOM property for. Did you mean htmlFor?\n in label', + 'Warning: Invalid DOM property `for`. Did you mean `htmlFor`?\n in label', ); expectDev(console.error.calls.argsFor(1)[0]).toBe( - 'Warning: Unknown DOM property autofocus. Did you mean autoFocus?\n in input', + 'Warning: Invalid DOM property `autofocus`. Did you mean `autoFocus`?\n in input', ); }); }); @@ -1888,7 +1888,7 @@ describe('ReactDOMComponent', () => { expect(el.className).toBe(''); expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Unknown DOM property class. Did you mean className?', + 'Warning: Invalid DOM property `class`. Did you mean `className`?', ); }); @@ -1903,7 +1903,7 @@ describe('ReactDOMComponent', () => { expect(text.getAttribute('arabic-form')).toBe(null); expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Unknown DOM property arabic-form. Did you mean arabicForm?', + 'Warning: Invalid DOM property `arabic-form`. Did you mean `arabicForm`?', ); }); @@ -1969,7 +1969,7 @@ describe('ReactDOMComponent', () => { expect(el.hasAttribute('whatever')).toBe(false); expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Unknown prop `whatever` on
tag', + 'Warning: Invalid prop `whatever` on
tag', ); }); @@ -1981,7 +1981,7 @@ describe('ReactDOMComponent', () => { expect(el.hasAttribute('whatever')).toBe(false); expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Unknown prop `whatever` on
tag', + 'Warning: Invalid prop `whatever` on
tag.', ); }); @@ -1991,5 +1991,31 @@ describe('ReactDOMComponent', () => { expect(el.getAttribute('ajaxify')).toBe('ajaxy'); }); + + it('only allows lower-case data attributes', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('data-foobar')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid DOM property `data-fooBar`. Custom attributes ' + + 'and data attributes must be lower case. Instead use `data-foobar`', + ); + }); + + it('only allows lower-case custom attributes', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.hasAttribute('foobar')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid DOM property `fooBar`. Custom attributes ' + + 'and data attributes must be lower case. Instead use `foobar`', + ); + }); }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 4a26bfc84c4b9..8080371bb3605 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -809,6 +809,11 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('data-foo')).toBe(false); }); + itRenders('no unknown data- attributes with casing', async render => { + const e = await render(
, 1); + expect(e.hasAttribute('data-foobar')).toBe(false); + }); + itRenders('custom attributes for non-standard elements', async render => { const e = await render(, 0); expect(e.getAttribute('foo')).toBe('bar'); @@ -843,9 +848,9 @@ describe('ReactDOMServerIntegration', () => { }, ); - itRenders('custom attributes with special casing', async render => { - const e = await render(
); - expect(e.getAttribute('fooBar')).toBe('test'); + itRenders('no cased custom attributes', async render => { + const e = await render(
, 1); + expect(e.hasAttribute('fooBar')).toBe(false); }); }); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index c2030ec73ed28..f71af11a2e80c 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -37,10 +37,9 @@ function getStackAddendum(debugID) { if (__DEV__) { var warnedProperties = {}; var EVENT_NAME_REGEX = /^on[A-Z]/; + var ARIA_NAME_REGEX = /^aria-/i; var validateProperty = function(tagName, name, value, debugID) { - var lowerCasedName = name.toLowerCase(); - if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { return true; } @@ -60,6 +59,7 @@ if (__DEV__) { return true; } + var lowerCasedName = name.toLowerCase(); var registrationName = EventPluginRegistry.possibleRegistrationNames.hasOwnProperty( lowerCasedName, ) @@ -69,7 +69,7 @@ if (__DEV__) { if (registrationName != null) { warning( false, - 'Unknown event handler property %s. Did you mean `%s`?%s', + 'Unknown event handler property `%s`. Did you mean `%s`?%s', name, registrationName, getStackAddendum(debugID), @@ -77,29 +77,45 @@ if (__DEV__) { return true; } - // data-* attributes should be lowercase; suggest the lowercase version - var standardName = DOMProperty.getPossibleStandardName.hasOwnProperty(name) - ? DOMProperty.getPossibleStandardName[name] - : null; + if (DOMProperty.isReservedProp(name)) { + return true; + } - var hasBadCasing = standardName != null && standardName !== name; + // Let the ARIA attribute hook validate ARIA attributes + if (ARIA_NAME_REGEX.test(name)) { + return true; + } - if (DOMProperty.shouldSetAttribute(name, value) && !hasBadCasing) { + // Known attributes should match the casing specified in the property config. + if (DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName)) { + var standardName = DOMProperty.getPossibleStandardName[lowerCasedName]; + if (standardName !== name) { + warning( + false, + 'Invalid DOM property `%s`. Did you mean `%s`?%s', + name, + standardName, + getStackAddendum(debugID), + ); + } return true; } - if (standardName != null) { + // Otherwise, we have a custom attribute. Custom attributes should always + // be lowercase. + if (lowerCasedName !== name) { warning( false, - 'Unknown DOM property %s. Did you mean %s?%s', + 'Invalid DOM property `%s`. Custom attributes and data attributes ' + + 'must be lower case. Instead use `%s`.%s', name, - standardName, + lowerCasedName, getStackAddendum(debugID), ); return true; } - return DOMProperty.isReservedProp(name); + return DOMProperty.shouldSetAttribute(name, value); }; } @@ -117,7 +133,7 @@ var warnUnknownProperties = function(type, props, debugID) { if (unknownProps.length === 1) { warning( false, - 'Unknown prop %s on <%s> tag. Either remove this prop from the element, ' + + 'Invalid prop %s on <%s> tag. Either remove this prop from the element, ' + 'or pass a string, number, or boolean value to keep it in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, @@ -127,7 +143,7 @@ var warnUnknownProperties = function(type, props, debugID) { } else if (unknownProps.length > 1) { warning( false, - 'Unknown props %s on <%s> tag. Either remove these props from the element, ' + + 'Invalid props %s on <%s> tag. Either remove these props from the element, ' + 'or pass a string, number, or boolean value to keep them in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, From 07c686587f4615ed553c7bc582bc7a28b6e882ee Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sun, 6 Aug 2017 21:20:44 -0400 Subject: [PATCH 22/49] Allow uppercase letters in custom attributes. Address associated edge cases --- .../dom/fiber/ReactDOMFiberComponent.js | 3 +- src/renderers/dom/shared/DOMProperty.js | 10 ++-- .../__tests__/ReactDOMComponent-test.js | 24 ++------ .../ReactDOMServerIntegration-test.js | 56 ++++++++++++++++--- .../hooks/ReactDOMUnknownPropertyHook.js | 14 ----- 5 files changed, 58 insertions(+), 49 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 11721488bc796..8606b5448550b 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -922,8 +922,7 @@ var ReactDOMFiberComponent = { var extraAttributeNames: Set = new Set(); var attributes = domElement.attributes; for (var i = 0; i < attributes.length; i++) { - // TODO: Do we need to lower case this to get case insensitive matches? - var name = attributes[i].name; + var name = attributes[i].name.toLowerCase(); switch (name) { // Built-in SSR attribute is whitelisted case 'data-reactroot': diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index a3641c932dfdd..1edc7261d1fd0 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -119,7 +119,11 @@ var DOMPropertyInjection = { if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; - DOMProperty.aliases[attributeName] = true; + DOMProperty.aliases[attributeName.toLowerCase()] = true; + + if (lowerCased !== attributeName) { + DOMProperty.aliases[lowerCased] = true; + } propertyInfo.attributeName = attributeName; if (__DEV__) { @@ -238,10 +242,6 @@ var DOMProperty = { return true; } - if (name.toLowerCase() !== name) { - return false; - } - switch (typeof value) { case 'undefined': case 'boolean': diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 74a9f1eef50bc..9cdb405109944 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1992,30 +1992,14 @@ describe('ReactDOMComponent', () => { expect(el.getAttribute('ajaxify')).toBe('ajaxy'); }); - it('only allows lower-case data attributes', function() { - spyOn(console, 'error'); - + it('allows cased data attributes', function() { var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.hasAttribute('data-foobar')).toBe(false); - - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Invalid DOM property `data-fooBar`. Custom attributes ' + - 'and data attributes must be lower case. Instead use `data-foobar`', - ); + expect(el.getAttribute('data-foobar')).toBe('true'); }); - it('only allows lower-case custom attributes', function() { - spyOn(console, 'error'); - + it('allows cased custom attributes', function() { var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.hasAttribute('foobar')).toBe(false); - - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'Warning: Invalid DOM property `fooBar`. Custom attributes ' + - 'and data attributes must be lower case. Instead use `foobar`', - ); + expect(el.getAttribute('foobar')).toBe('true'); }); }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 8080371bb3605..be43ddaf30a10 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -615,6 +615,12 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('className')).toBe(false); }); + itRenders('no classname prop', async render => { + const e = await render(
, 1); + expect(e.hasAttribute('class')).toBe(false); + expect(e.hasAttribute('classname')).toBe(false); + }); + itRenders('no className prop when given the alias', async render => { const e = await render(
, 1); expect(e.className).toBe(''); @@ -793,9 +799,35 @@ describe('ReactDOMServerIntegration', () => { }); }); + describe('cased attributes', function() { + itRenders('no badly cased aliased HTML attribute', async render => { + const e = await render(, 1); + expect(e.hasAttribute('http-equiv')).toBe(false); + expect(e.hasAttribute('httpequiv')).toBe(false); + }); + + itRenders('no badly cased SVG attribute', async render => { + const e = await render(, 1); + expect(e.hasAttribute('textLength')).toBe(false); + }); + + itRenders('no badly cased aliased SVG attribute', async render => { + const e = await render(, 1); + expect(e.hasAttribute('strokedasharray')).toBe(false); + }); + + itRenders( + 'no badly cased original SVG attribute that is aliased', + async render => { + const e = await render(, 1); + expect(e.hasAttribute('stroke-dasharray')).toBe(false); + }, + ); + }); + describe('unknown attributes', function() { itRenders('unknown attributes', async render => { - const e = await render(
, 0); + const e = await render(
); expect(e.getAttribute('foo')).toBe('bar'); }); @@ -809,13 +841,21 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('data-foo')).toBe(false); }); - itRenders('no unknown data- attributes with casing', async render => { - const e = await render(
, 1); - expect(e.hasAttribute('data-foobar')).toBe(false); + itRenders('unknown data- attributes with casing', async render => { + const e = await render(
); + expect(e.getAttribute('data-fooBar')).toBe('true'); }); + itRenders( + 'no unknown data- attributes with casing and null value', + async render => { + const e = await render(
); + expect(e.hasAttribute('data-fooBar')).toBe(false); + }, + ); + itRenders('custom attributes for non-standard elements', async render => { - const e = await render(, 0); + const e = await render(); expect(e.getAttribute('foo')).toBe('bar'); }); @@ -848,9 +888,9 @@ describe('ReactDOMServerIntegration', () => { }, ); - itRenders('no cased custom attributes', async render => { - const e = await render(
, 1); - expect(e.hasAttribute('fooBar')).toBe(false); + itRenders('cased custom attributes', async render => { + const e = await render(
); + expect(e.getAttribute('fooBar')).toBe('test'); }); }); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index f71af11a2e80c..11da5af3c315d 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -101,20 +101,6 @@ if (__DEV__) { return true; } - // Otherwise, we have a custom attribute. Custom attributes should always - // be lowercase. - if (lowerCasedName !== name) { - warning( - false, - 'Invalid DOM property `%s`. Custom attributes and data attributes ' + - 'must be lower case. Instead use `%s`.%s', - name, - lowerCasedName, - getStackAddendum(debugID), - ); - return true; - } - return DOMProperty.shouldSetAttribute(name, value); }; } From 599844e090f18902057b5b4c4a4005d53c929b66 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:28:19 -0400 Subject: [PATCH 23/49] Make ARIA enforcement dev-only --- .../dom/shared/ARIADOMPropertyConfig.js | 74 ------------------- src/renderers/dom/shared/ReactDOMInjection.js | 2 - .../shared/hooks/ReactDOMInvalidARIAHook.js | 8 +- .../dom/shared/hooks/validAriaProperties.js | 69 +++++++++++++++++ 4 files changed, 73 insertions(+), 80 deletions(-) delete mode 100644 src/renderers/dom/shared/ARIADOMPropertyConfig.js create mode 100644 src/renderers/dom/shared/hooks/validAriaProperties.js diff --git a/src/renderers/dom/shared/ARIADOMPropertyConfig.js b/src/renderers/dom/shared/ARIADOMPropertyConfig.js deleted file mode 100644 index 108278940cd1c..0000000000000 --- a/src/renderers/dom/shared/ARIADOMPropertyConfig.js +++ /dev/null @@ -1,74 +0,0 @@ -/** - * Copyright 2013-present, Facebook, Inc. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. An additional grant - * of patent rights can be found in the PATENTS file in the same directory. - * - * @providesModule ARIADOMPropertyConfig - */ - -'use strict'; - -var ARIADOMPropertyConfig = { - Properties: { - // Global States and Properties - 'aria-current': 0, // state - 'aria-details': 0, - 'aria-disabled': 0, // state - 'aria-hidden': 0, // state - 'aria-invalid': 0, // state - 'aria-keyshortcuts': 0, - 'aria-label': 0, - 'aria-roledescription': 0, - // Widget Attributes - 'aria-autocomplete': 0, - 'aria-checked': 0, - 'aria-expanded': 0, - 'aria-haspopup': 0, - 'aria-level': 0, - 'aria-modal': 0, - 'aria-multiline': 0, - 'aria-multiselectable': 0, - 'aria-orientation': 0, - 'aria-placeholder': 0, - 'aria-pressed': 0, - 'aria-readonly': 0, - 'aria-required': 0, - 'aria-selected': 0, - 'aria-sort': 0, - 'aria-valuemax': 0, - 'aria-valuemin': 0, - 'aria-valuenow': 0, - 'aria-valuetext': 0, - // Live Region Attributes - 'aria-atomic': 0, - 'aria-busy': 0, - 'aria-live': 0, - 'aria-relevant': 0, - // Drag-and-Drop Attributes - 'aria-dropeffect': 0, - 'aria-grabbed': 0, - // Relationship Attributes - 'aria-activedescendant': 0, - 'aria-colcount': 0, - 'aria-colindex': 0, - 'aria-colspan': 0, - 'aria-controls': 0, - 'aria-describedby': 0, - 'aria-errormessage': 0, - 'aria-flowto': 0, - 'aria-labelledby': 0, - 'aria-owns': 0, - 'aria-posinset': 0, - 'aria-rowcount': 0, - 'aria-rowindex': 0, - 'aria-rowspan': 0, - 'aria-setsize': 0, - }, - DOMAttributeNames: {}, - DOMPropertyNames: {}, -}; - -module.exports = ARIADOMPropertyConfig; diff --git a/src/renderers/dom/shared/ReactDOMInjection.js b/src/renderers/dom/shared/ReactDOMInjection.js index e507423f6ab76..a14c7cc2d6239 100644 --- a/src/renderers/dom/shared/ReactDOMInjection.js +++ b/src/renderers/dom/shared/ReactDOMInjection.js @@ -11,11 +11,9 @@ 'use strict'; -var ARIADOMPropertyConfig = require('ARIADOMPropertyConfig'); var DOMProperty = require('DOMProperty'); var HTMLDOMPropertyConfig = require('HTMLDOMPropertyConfig'); var SVGDOMPropertyConfig = require('SVGDOMPropertyConfig'); -DOMProperty.injection.injectDOMPropertyConfig(ARIADOMPropertyConfig); DOMProperty.injection.injectDOMPropertyConfig(HTMLDOMPropertyConfig); DOMProperty.injection.injectDOMPropertyConfig(SVGDOMPropertyConfig); diff --git a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js index 7385c7a17639b..1a65728f97d17 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js @@ -23,6 +23,8 @@ if (__DEV__) { ReactDebugCurrentFrame, } = require('ReactGlobalSharedState'); var {getStackAddendumByID} = ReactComponentTreeHook; + + var validAriaProperties = require('./validAriaProperties'); } function getStackAddendum(debugID) { @@ -43,10 +45,8 @@ function validateProperty(tagName, name, debugID) { if (rARIA.test(name)) { var lowerCasedName = name.toLowerCase(); - var standardName = DOMProperty.getPossibleStandardName.hasOwnProperty( - lowerCasedName, - ) - ? DOMProperty.getPossibleStandardName[lowerCasedName] + var standardName = validAriaProperties.hasOwnProperty(lowerCasedName) + ? lowerCasedName : null; // If this is an aria-* attribute, but is not listed in the known DOM diff --git a/src/renderers/dom/shared/hooks/validAriaProperties.js b/src/renderers/dom/shared/hooks/validAriaProperties.js new file mode 100644 index 0000000000000..9a75a6201b8a4 --- /dev/null +++ b/src/renderers/dom/shared/hooks/validAriaProperties.js @@ -0,0 +1,69 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule validAriaProperties + */ + +'use strict'; + +var ariaProperties = { + 'aria-current': 0, // state + 'aria-details': 0, + 'aria-disabled': 0, // state + 'aria-hidden': 0, // state + 'aria-invalid': 0, // state + 'aria-keyshortcuts': 0, + 'aria-label': 0, + 'aria-roledescription': 0, + // Widget Attributes + 'aria-autocomplete': 0, + 'aria-checked': 0, + 'aria-expanded': 0, + 'aria-haspopup': 0, + 'aria-level': 0, + 'aria-modal': 0, + 'aria-multiline': 0, + 'aria-multiselectable': 0, + 'aria-orientation': 0, + 'aria-placeholder': 0, + 'aria-pressed': 0, + 'aria-readonly': 0, + 'aria-required': 0, + 'aria-selected': 0, + 'aria-sort': 0, + 'aria-valuemax': 0, + 'aria-valuemin': 0, + 'aria-valuenow': 0, + 'aria-valuetext': 0, + // Live Region Attributes + 'aria-atomic': 0, + 'aria-busy': 0, + 'aria-live': 0, + 'aria-relevant': 0, + // Drag-and-Drop Attributes + 'aria-dropeffect': 0, + 'aria-grabbed': 0, + // Relationship Attributes + 'aria-activedescendant': 0, + 'aria-colcount': 0, + 'aria-colindex': 0, + 'aria-colspan': 0, + 'aria-controls': 0, + 'aria-describedby': 0, + 'aria-errormessage': 0, + 'aria-flowto': 0, + 'aria-labelledby': 0, + 'aria-owns': 0, + 'aria-posinset': 0, + 'aria-rowcount': 0, + 'aria-rowindex': 0, + 'aria-rowspan': 0, + 'aria-setsize': 0, +}; + +module.exports = ariaProperties; From 14d5ea042f6a9727edfbe1a975ebea7ed8b15d80 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 10:27:33 -0400 Subject: [PATCH 24/49] Remove non-case sensitive standard attributes. Make ARIA hook dev only. --- .../dom/shared/HTMLDOMPropertyConfig.js | 83 +---------------- .../dom/shared/SVGDOMPropertyConfig.js | 92 ------------------- .../__tests__/DOMPropertyOperations-test.js | 1 + 3 files changed, 4 insertions(+), 172 deletions(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index e8525884e7d77..37ae6d0cfe126 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -26,15 +26,11 @@ var HTMLDOMPropertyConfig = { /** * Standard Properties */ - accept: 0, acceptCharset: 0, accessKey: 0, - action: 0, 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 @@ -44,134 +40,71 @@ var HTMLDOMPropertyConfig = { cellPadding: 0, cellSpacing: 0, charSet: 0, - challenge: 0, checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, - cite: 0, classID: 0, className: 0, cols: HAS_POSITIVE_NUMERIC_VALUE, colSpan: 0, - content: 0, contentEditable: 0, contextMenu: 0, controls: HAS_BOOLEAN_VALUE, controlsList: 0, - coords: 0, crossOrigin: 0, - data: 0, // For `` acts as `src`. dateTime: 0, default: HAS_BOOLEAN_VALUE, defer: HAS_BOOLEAN_VALUE, - dir: 0, disabled: HAS_BOOLEAN_VALUE, download: HAS_OVERLOADED_BOOLEAN_VALUE, - draggable: 0, encType: 0, - form: 0, formAction: 0, formEncType: 0, formMethod: 0, formNoValidate: HAS_BOOLEAN_VALUE, formTarget: 0, frameBorder: 0, - headers: 0, - height: 0, hidden: HAS_BOOLEAN_VALUE, - high: 0, - href: 0, hrefLang: 0, htmlFor: 0, httpEquiv: 0, - id: 0, inputMode: 0, - integrity: 0, - is: 0, keyParams: 0, keyType: 0, - kind: 0, - label: 0, - lang: 0, - list: 0, - loop: HAS_BOOLEAN_VALUE, - low: 0, - manifest: 0, marginHeight: 0, marginWidth: 0, - max: 0, maxLength: 0, - media: 0, mediaGroup: 0, - method: 0, - min: 0, minLength: 0, // Caution; `option.selected` is not updated if `select.multiple` is // disabled with `removeAttribute`. multiple: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, muted: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, - name: 0, - nonce: 0, noValidate: HAS_BOOLEAN_VALUE, open: HAS_BOOLEAN_VALUE, - optimum: 0, - pattern: 0, - placeholder: 0, playsInline: HAS_BOOLEAN_VALUE, - poster: 0, - preload: 0, - profile: 0, radioGroup: 0, readOnly: HAS_BOOLEAN_VALUE, referrerPolicy: 0, - rel: 0, required: HAS_BOOLEAN_VALUE, reversed: HAS_BOOLEAN_VALUE, - role: 0, rows: HAS_POSITIVE_NUMERIC_VALUE, rowSpan: HAS_NUMERIC_VALUE, - sandbox: 0, - scope: 0, scoped: HAS_BOOLEAN_VALUE, - scrolling: 0, seamless: HAS_BOOLEAN_VALUE, selected: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, - shape: 0, size: HAS_POSITIVE_NUMERIC_VALUE, - sizes: 0, // support for projecting regular DOM Elements via V1 named slots ( shadow dom ) - slot: 0, span: HAS_POSITIVE_NUMERIC_VALUE, spellCheck: 0, - src: 0, srcDoc: 0, srcLang: 0, srcSet: 0, - start: HAS_NUMERIC_VALUE, - step: 0, + // Style must be explicitely set in the attribute list. React components + // expect a style object style: 0, - summary: 0, + start: HAS_NUMERIC_VALUE, tabIndex: 0, - target: 0, - title: 0, - // Setting .type throws on non- tags - type: 0, useMap: 0, value: 0, - width: 0, - wmode: 0, - wrap: 0, - - /** - * RDFa Properties - */ - about: 0, - datatype: 0, - inlist: 0, - prefix: 0, - // property is also supported for OpenGraph in meta tags. - property: 0, - resource: 0, - typeof: 0, - vocab: 0, /** * Non-standard Properties @@ -182,8 +115,6 @@ var HTMLDOMPropertyConfig = { autoCorrect: 0, // autoSave allows WebKit/Blink to persist values of input fields on page reloads autoSave: 0, - // color is for Safari mask-icon link - color: 0, // itemProp, itemScope, itemType are for // Microdata support. See http://schema.org/docs/gs.html itemProp: 0, @@ -194,14 +125,6 @@ var HTMLDOMPropertyConfig = { // https://html.spec.whatwg.org/multipage/microdata.html#microdata-dom-api itemID: 0, itemRef: 0, - // results show looking glass icon and recent searches on input - // search fields in WebKit/Blink - results: 0, - // IE-only attribute that specifies security restrictions on an iframe - // as an alternative to the sandbox attribute on IE<10 - security: 0, - // IE-only attribute that controls focus behavior - unselectable: 0, // Facebook internal attribute. This is an object attribute that // impliments a custom toString() method ajaxify: 0, diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index dbb9da3be1f90..7965311d18dc1 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -35,28 +35,17 @@ var NS = { // - width var ATTRS = { accentHeight: 'accent-height', - accumulate: 0, - additive: 0, alignmentBaseline: 'alignment-baseline', allowReorder: 'allowReorder', - alphabetic: 0, - amplitude: 0, arabicForm: 'arabic-form', - ascent: 0, attributeName: 'attributeName', attributeType: 'attributeType', autoReverse: 'autoReverse', - azimuth: 0, baseFrequency: 'baseFrequency', baseProfile: 'baseProfile', baselineShift: 'baseline-shift', - bbox: 0, - begin: 0, - bias: 0, - by: 0, calcMode: 'calcMode', capHeight: 'cap-height', - clip: 0, clipPath: 'clip-path', clipRule: 'clip-rule', clipPathUnits: 'clipPathUnits', @@ -66,35 +55,17 @@ var ATTRS = { colorRendering: 'color-rendering', contentScriptType: 'contentScriptType', contentStyleType: 'contentStyleType', - cursor: 0, - cx: 0, - cy: 0, - d: 0, - decelerate: 0, - descent: 0, diffuseConstant: 'diffuseConstant', - direction: 0, - display: 0, - divisor: 0, dominantBaseline: 'dominant-baseline', - dur: 0, - dx: 0, - dy: 0, edgeMode: 'edgeMode', - elevation: 0, enableBackground: 'enable-background', - end: 0, - exponent: 0, externalResourcesRequired: 'externalResourcesRequired', - fill: 0, fillOpacity: 'fill-opacity', fillRule: 'fill-rule', - filter: 0, filterRes: 'filterRes', filterUnits: 'filterUnits', floodColor: 'flood-color', floodOpacity: 'flood-opacity', - focusable: 0, fontFamily: 'font-family', fontSize: 'font-size', fontSizeAdjust: 'font-size-adjust', @@ -102,34 +73,17 @@ var ATTRS = { fontStyle: 'font-style', fontVariant: 'font-variant', fontWeight: 'font-weight', - format: 0, - from: 0, - fx: 0, - fy: 0, - g1: 0, - g2: 0, glyphName: 'glyph-name', glyphOrientationHorizontal: 'glyph-orientation-horizontal', glyphOrientationVertical: 'glyph-orientation-vertical', glyphRef: 'glyphRef', gradientTransform: 'gradientTransform', gradientUnits: 'gradientUnits', - hanging: 0, horizAdvX: 'horiz-adv-x', horizOriginX: 'horiz-origin-x', - ideographic: 0, imageRendering: 'image-rendering', - in: 0, - in2: 0, - intercept: 0, - k: 0, - k1: 0, - k2: 0, - k3: 0, - k4: 0, kernelMatrix: 'kernelMatrix', kernelUnitLength: 'kernelUnitLength', - kerning: 0, keyPoints: 'keyPoints', keySplines: 'keySplines', keyTimes: 'keyTimes', @@ -137,27 +91,15 @@ var ATTRS = { letterSpacing: 'letter-spacing', lightingColor: 'lighting-color', limitingConeAngle: 'limitingConeAngle', - local: 0, markerEnd: 'marker-end', markerMid: 'marker-mid', markerStart: 'marker-start', markerHeight: 'markerHeight', markerUnits: 'markerUnits', markerWidth: 'markerWidth', - mask: 0, maskContentUnits: 'maskContentUnits', maskUnits: 'maskUnits', - mathematical: 0, - mode: 0, numOctaves: 'numOctaves', - offset: 0, - opacity: 0, - operator: 0, - order: 0, - orient: 0, - orientation: 0, - origin: 0, - overflow: 0, overlinePosition: 'overline-position', overlineThickness: 'overline-thickness', paintOrder: 'paint-order', @@ -167,15 +109,12 @@ var ATTRS = { patternTransform: 'patternTransform', patternUnits: 'patternUnits', pointerEvents: 'pointer-events', - points: 0, pointsAtX: 'pointsAtX', pointsAtY: 'pointsAtY', pointsAtZ: 'pointsAtZ', preserveAlpha: 'preserveAlpha', preserveAspectRatio: 'preserveAspectRatio', primitiveUnits: 'primitiveUnits', - r: 0, - radius: 0, refX: 'refX', refY: 'refY', renderingIntent: 'rendering-intent', @@ -183,31 +122,17 @@ var ATTRS = { repeatDur: 'repeatDur', requiredExtensions: 'requiredExtensions', requiredFeatures: 'requiredFeatures', - restart: 0, - result: 0, - rotate: 0, - rx: 0, - ry: 0, - scale: 0, - seed: 0, shapeRendering: 'shape-rendering', - slope: 0, - spacing: 0, specularConstant: 'specularConstant', specularExponent: 'specularExponent', - speed: 0, spreadMethod: 'spreadMethod', startOffset: 'startOffset', stdDeviation: 'stdDeviation', - stemh: 0, - stemv: 0, stitchTiles: 'stitchTiles', stopColor: 'stop-color', stopOpacity: 'stop-opacity', strikethroughPosition: 'strikethrough-position', strikethroughThickness: 'strikethrough-thickness', - string: 0, - stroke: 0, strokeDasharray: 'stroke-dasharray', strokeDashoffset: 'stroke-dashoffset', strokeLinecap: 'stroke-linecap', @@ -224,13 +149,8 @@ var ATTRS = { textDecoration: 'text-decoration', textRendering: 'text-rendering', textLength: 'textLength', - to: 0, - transform: 0, - u1: 0, - u2: 0, underlinePosition: 'underline-position', underlineThickness: 'underline-thickness', - unicode: 0, unicodeBidi: 'unicode-bidi', unicodeRange: 'unicode-range', unitsPerEm: 'units-per-em', @@ -238,22 +158,15 @@ var ATTRS = { vHanging: 'v-hanging', vIdeographic: 'v-ideographic', vMathematical: 'v-mathematical', - values: 0, vectorEffect: 'vector-effect', - version: 0, vertAdvY: 'vert-adv-y', vertOriginX: 'vert-origin-x', vertOriginY: 'vert-origin-y', viewBox: 'viewBox', viewTarget: 'viewTarget', - visibility: 0, - widths: 0, wordSpacing: 'word-spacing', writingMode: 'writing-mode', - x: 0, xHeight: 'x-height', - x1: 0, - x2: 0, xChannelSelector: 'xChannelSelector', xlinkActuate: 'xlink:actuate', xlinkArcrole: 'xlink:arcrole', @@ -263,15 +176,10 @@ var ATTRS = { xlinkTitle: 'xlink:title', xlinkType: 'xlink:type', xmlBase: 'xml:base', - xmlns: 0, xmlnsXlink: 'xmlns:xlink', xmlLang: 'xml:lang', xmlSpace: 'xml:space', - y: 0, - y1: 0, - y2: 0, yChannelSelector: 'yChannelSelector', - z: 0, zoomAndPan: 'zoomAndPan', }; diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index c0033f1ba2dba..359e1c98e3b18 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -70,6 +70,7 @@ describe('DOMPropertyOperations', () => { return ''; }, }; + var container = document.createElement('div'); ReactDOM.render(
, container); expect(container.firstChild.getAttribute('role')).toBe(''); From 99206dbbd98dc8de51df57905c175a0279efd21c Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:01:59 -0400 Subject: [PATCH 25/49] Remove case sensitive props --- .../dom/shared/HTMLDOMPropertyConfig.js | 61 +--- .../dom/shared/SVGDOMPropertyConfig.js | 60 ---- .../hooks/ReactDOMUnknownPropertyHook.js | 5 +- .../dom/shared/hooks/possibleStandardNames.js | 309 ++++++++++++++++++ 4 files changed, 315 insertions(+), 120 deletions(-) create mode 100644 src/renderers/dom/shared/hooks/possibleStandardNames.js diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 37ae6d0cfe126..fb01ea43a89a5 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -27,53 +27,25 @@ var HTMLDOMPropertyConfig = { * Standard Properties */ acceptCharset: 0, - accessKey: 0, allowFullScreen: HAS_BOOLEAN_VALUE, - allowTransparency: 0, // specifies target context for links with `preload` type async: HAS_BOOLEAN_VALUE, - autoComplete: 0, // autoFocus is polyfilled/normalized by AutoFocusUtils // autoFocus: HAS_BOOLEAN_VALUE, autoPlay: HAS_BOOLEAN_VALUE, capture: HAS_BOOLEAN_VALUE, - cellPadding: 0, - cellSpacing: 0, - charSet: 0, checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, - classID: 0, - className: 0, cols: HAS_POSITIVE_NUMERIC_VALUE, - colSpan: 0, - contentEditable: 0, - contextMenu: 0, controls: HAS_BOOLEAN_VALUE, - controlsList: 0, - crossOrigin: 0, - dateTime: 0, + className: 0, default: HAS_BOOLEAN_VALUE, defer: HAS_BOOLEAN_VALUE, disabled: HAS_BOOLEAN_VALUE, download: HAS_OVERLOADED_BOOLEAN_VALUE, - encType: 0, - formAction: 0, - formEncType: 0, - formMethod: 0, formNoValidate: HAS_BOOLEAN_VALUE, - formTarget: 0, - frameBorder: 0, hidden: HAS_BOOLEAN_VALUE, - hrefLang: 0, htmlFor: 0, httpEquiv: 0, - inputMode: 0, - keyParams: 0, - keyType: 0, - marginHeight: 0, - marginWidth: 0, - maxLength: 0, - mediaGroup: 0, - minLength: 0, // Caution; `option.selected` is not updated if `select.multiple` is // disabled with `removeAttribute`. multiple: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, @@ -81,9 +53,7 @@ var HTMLDOMPropertyConfig = { noValidate: HAS_BOOLEAN_VALUE, open: HAS_BOOLEAN_VALUE, playsInline: HAS_BOOLEAN_VALUE, - radioGroup: 0, readOnly: HAS_BOOLEAN_VALUE, - referrerPolicy: 0, required: HAS_BOOLEAN_VALUE, reversed: HAS_BOOLEAN_VALUE, rows: HAS_POSITIVE_NUMERIC_VALUE, @@ -94,37 +64,12 @@ var HTMLDOMPropertyConfig = { size: HAS_POSITIVE_NUMERIC_VALUE, // support for projecting regular DOM Elements via V1 named slots ( shadow dom ) span: HAS_POSITIVE_NUMERIC_VALUE, - spellCheck: 0, - srcDoc: 0, - srcLang: 0, - srcSet: 0, // Style must be explicitely set in the attribute list. React components // expect a style object - style: 0, start: HAS_NUMERIC_VALUE, - tabIndex: 0, - useMap: 0, - value: 0, - - /** - * Non-standard Properties - */ - // autoCapitalize and autoCorrect are supported in Mobile Safari for - // keyboard hints. - autoCapitalize: 0, - autoCorrect: 0, - // autoSave allows WebKit/Blink to persist values of input fields on page reloads - autoSave: 0, - // itemProp, itemScope, itemType are for - // Microdata support. See http://schema.org/docs/gs.html - itemProp: 0, + // itemScope is for for Microdata support. + // See http://schema.org/docs/gs.html itemScope: HAS_BOOLEAN_VALUE, - itemType: 0, - // itemID and itemRef are for Microdata support as well but - // only specified in the WHATWG spec document. See - // https://html.spec.whatwg.org/multipage/microdata.html#microdata-dom-api - itemID: 0, - itemRef: 0, // Facebook internal attribute. This is an object attribute that // impliments a custom toString() method ajaxify: 0, diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index 7965311d18dc1..d11b344215e01 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -36,34 +36,19 @@ var NS = { var ATTRS = { accentHeight: 'accent-height', alignmentBaseline: 'alignment-baseline', - allowReorder: 'allowReorder', arabicForm: 'arabic-form', - attributeName: 'attributeName', - attributeType: 'attributeType', - autoReverse: 'autoReverse', - baseFrequency: 'baseFrequency', - baseProfile: 'baseProfile', baselineShift: 'baseline-shift', - calcMode: 'calcMode', capHeight: 'cap-height', clipPath: 'clip-path', clipRule: 'clip-rule', - clipPathUnits: 'clipPathUnits', colorInterpolation: 'color-interpolation', colorInterpolationFilters: 'color-interpolation-filters', colorProfile: 'color-profile', colorRendering: 'color-rendering', - contentScriptType: 'contentScriptType', - contentStyleType: 'contentStyleType', - diffuseConstant: 'diffuseConstant', dominantBaseline: 'dominant-baseline', - edgeMode: 'edgeMode', enableBackground: 'enable-background', - externalResourcesRequired: 'externalResourcesRequired', fillOpacity: 'fill-opacity', fillRule: 'fill-rule', - filterRes: 'filterRes', - filterUnits: 'filterUnits', floodColor: 'flood-color', floodOpacity: 'flood-opacity', fontFamily: 'font-family', @@ -76,59 +61,22 @@ var ATTRS = { glyphName: 'glyph-name', glyphOrientationHorizontal: 'glyph-orientation-horizontal', glyphOrientationVertical: 'glyph-orientation-vertical', - glyphRef: 'glyphRef', - gradientTransform: 'gradientTransform', - gradientUnits: 'gradientUnits', horizAdvX: 'horiz-adv-x', horizOriginX: 'horiz-origin-x', imageRendering: 'image-rendering', - kernelMatrix: 'kernelMatrix', - kernelUnitLength: 'kernelUnitLength', - keyPoints: 'keyPoints', - keySplines: 'keySplines', - keyTimes: 'keyTimes', - lengthAdjust: 'lengthAdjust', letterSpacing: 'letter-spacing', lightingColor: 'lighting-color', - limitingConeAngle: 'limitingConeAngle', markerEnd: 'marker-end', markerMid: 'marker-mid', markerStart: 'marker-start', - markerHeight: 'markerHeight', - markerUnits: 'markerUnits', - markerWidth: 'markerWidth', - maskContentUnits: 'maskContentUnits', - maskUnits: 'maskUnits', - numOctaves: 'numOctaves', overlinePosition: 'overline-position', overlineThickness: 'overline-thickness', paintOrder: 'paint-order', panose1: 'panose-1', pathLength: 'pathLength', - patternContentUnits: 'patternContentUnits', - patternTransform: 'patternTransform', - patternUnits: 'patternUnits', pointerEvents: 'pointer-events', - pointsAtX: 'pointsAtX', - pointsAtY: 'pointsAtY', - pointsAtZ: 'pointsAtZ', - preserveAlpha: 'preserveAlpha', - preserveAspectRatio: 'preserveAspectRatio', - primitiveUnits: 'primitiveUnits', - refX: 'refX', - refY: 'refY', renderingIntent: 'rendering-intent', - repeatCount: 'repeatCount', - repeatDur: 'repeatDur', - requiredExtensions: 'requiredExtensions', - requiredFeatures: 'requiredFeatures', shapeRendering: 'shape-rendering', - specularConstant: 'specularConstant', - specularExponent: 'specularExponent', - spreadMethod: 'spreadMethod', - startOffset: 'startOffset', - stdDeviation: 'stdDeviation', - stitchTiles: 'stitchTiles', stopColor: 'stop-color', stopOpacity: 'stop-opacity', strikethroughPosition: 'strikethrough-position', @@ -140,11 +88,6 @@ var ATTRS = { strokeMiterlimit: 'stroke-miterlimit', strokeOpacity: 'stroke-opacity', strokeWidth: 'stroke-width', - surfaceScale: 'surfaceScale', - systemLanguage: 'systemLanguage', - tableValues: 'tableValues', - targetX: 'targetX', - targetY: 'targetY', textAnchor: 'text-anchor', textDecoration: 'text-decoration', textRendering: 'text-rendering', @@ -167,7 +110,6 @@ var ATTRS = { wordSpacing: 'word-spacing', writingMode: 'writing-mode', xHeight: 'x-height', - xChannelSelector: 'xChannelSelector', xlinkActuate: 'xlink:actuate', xlinkArcrole: 'xlink:arcrole', xlinkHref: 'xlink:href', @@ -179,8 +121,6 @@ var ATTRS = { xmlnsXlink: 'xmlns:xlink', xmlLang: 'xml:lang', xmlSpace: 'xml:space', - yChannelSelector: 'yChannelSelector', - zoomAndPan: 'zoomAndPan', }; var SVGDOMPropertyConfig = { diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index 11da5af3c315d..f32128785c8bc 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -38,6 +38,7 @@ if (__DEV__) { var warnedProperties = {}; var EVENT_NAME_REGEX = /^on[A-Z]/; var ARIA_NAME_REGEX = /^aria-/i; + var possibleStandardNames = require('possibleStandardNames'); var validateProperty = function(tagName, name, value, debugID) { if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { @@ -87,8 +88,8 @@ if (__DEV__) { } // Known attributes should match the casing specified in the property config. - if (DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName)) { - var standardName = DOMProperty.getPossibleStandardName[lowerCasedName]; + if (possibleStandardNamesName.hasOwnProperty(lowerCasedName)) { + var standardName = getpossibleStandardNames[lowerCasedName]; if (standardName !== name) { warning( false, diff --git a/src/renderers/dom/shared/hooks/possibleStandardNames.js b/src/renderers/dom/shared/hooks/possibleStandardNames.js new file mode 100644 index 0000000000000..3f83cee2adcd0 --- /dev/null +++ b/src/renderers/dom/shared/hooks/possibleStandardNames.js @@ -0,0 +1,309 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule possibleStandardNames + */ + +var possibleStandardNames = { + // HTML + acceptcharset: 'acceptCharset', + 'accept-charset': 'acceptCharset', + accesskey: 'accessKey', + allowfullscreen: 'allowFullScreen', + allowtransparency: 'allowTransparency', + autocapitalize: 'autoCapitalize', + autocomplete: 'autoComplete', + autocorrect: 'autoCorrect', + autofocus: 'autoFocus', + autoplay: 'autoPlay', + autosave: 'autoSave', + cellpadding: 'cellPadding', + cellspacing: 'cellSpacing', + charset: 'charSet', + class: 'className', + classid: 'classID', + classname: 'className', + colspan: 'colSpan', + contenteditable: 'contentEditable', + contextmenu: 'contextMenu', + controlslist: 'controlsList', + crossorigin: 'crossOrigin', + datetime: 'dateTime', + defaultvalue: 'defaultValue', + defaultchecked: 'defaultChecked', + enctype: 'encType', + for: 'htmlFor', + formMethod: 'formMethod', + formaction: 'formAction', + formenctype: 'formEncType', + formnovalidate: 'formNoValidate', + formtarget: 'formTarget', + frameborder: 'frameBorder', + hreflang: 'hrefLang', + htmlfor: 'htmlFor', + httpequiv: 'httpEquiv', + 'http-equiv': 'httpEquiv', + innerhtml: 'innerHTML', + inputmode: 'inputMode', + itemid: 'itemID', + itemprop: 'itemProp', + itemref: 'itemRef', + itemscope: 'itemScope', + itemtype: 'itemType', + keyparams: 'keyParams', + keytype: 'keyType', + marginWidth: 'marginWidth', + marginheight: 'marginHeight', + maxlength: 'maxLength', + mediagroup: 'mediaGroup', + minLength: 'minlength', + novalidate: 'noValidate', + playsinline: 'playsInline', + radiogroup: 'radioGroup', + readonly: 'readOnly', + referrerpolicy: 'referrerPolicy', + rowspan: 'rowSpan', + spellcheck: 'spellCheck', + srcdoc: 'srcDoc', + srclang: 'srcLang', + srcset: 'srcSet', + tabindex: 'tabIndex', + usemap: 'useMap', + + // SVG + accentheight: 'accentHeight', + 'accent-height': 'accentHeight', + alignmentbaseline: 'alignmentBaseline', + 'alignment-baseline': 'alignmentBaseline', + allowreorder: 'allowReorder', + 'aribic-form': 'arabicForm', + arabicform: 'arabicForm', + attributename: 'attributeName', + attributetype: 'attributeType', + autoreverse: 'autoReverse', + basefrequency: 'baseFrequency', + baseprofile: 'baseProfile', + 'baseline-shift': 'baselineShift', + baselineshift: 'baselineShift', + calcmode: 'calcMode', + 'cap-height': 'capHeight', + capheight: 'capHeight', + 'clip-path': 'clipPath', + clippath: 'clipPath', + 'clip-rule': 'clipRule', + cliprule: 'clipRule', + clippathunits: 'clipPathUnits', + 'color-interpolation': 'colorInterpolation', + colorinterpolation: 'colorInterpolation', + 'color-interpolation-filters': 'colorInterpolationFilters', + colorinterpolationfilters: 'colorInterpolationFilters', + 'color-profile': 'colorProfile', + colorprofile: 'colorProfile', + 'color-rendering': 'colorRendering', + colorrendering: 'colorRendering', + contentscripttype: 'contentScriptType', + contentstyletype: 'contentStyleType', + diffuseconstant: 'diffuseConstant', + 'dominant-baseline': 'dominantBaseline', + dominantbaseline: 'dominantBaseline', + edgemode: 'edgeMode', + 'enable-background': 'enableBackground', + enablebackground: 'enableBackground', + externalresourcesrequired: 'externalResourcesRequired', + 'fill-opacity': 'fillOpacity', + fillopacity: 'fillOpacity', + 'fill-rule': 'fillRule', + fillrule: 'fillRule', + filterres: 'filterRes', + filterunits: 'filterUnits', + floodcolor: 'floodColor', + 'flood-color': 'floodColor', + 'flood-opacity': 'floodOpacity', + floodOpacity: 'floodOpacity', + 'font-family': 'fontFamily', + fontfamily: 'fontFamily', + 'font-size': 'fontSize', + fontsize: 'fontSize', + 'font-size-adjust': 'fontSizeAdjust', + fontsizeadjust: 'fontSizeAdjust', + 'font-stretch': 'fontStretch', + fontstretch: 'fontStretch', + 'font-style': 'fontStyle', + fontstyle: 'fontStyle', + 'font-variant': 'fontVariant', + fontvariant: 'fontVariant', + 'font-weight': 'fontWeight', + fontweight: 'fontWeight', + 'glyph-name': 'glyphName', + glyphname: 'glyphName', + 'glyph-orientation-horizontal': 'glyphOrientationHorizontal', + glyphorientationhorizontal: 'glyphOrientationHorizontal', + 'glyph-orientation-vertical': 'glyphOrientationVertical', + glyphorientationvertical: 'glyphOrientationVertical', + glyphref: 'glyphRef', + gradienttransform: 'gradientTransform', + gradientunits: 'gradientUnits', + 'horiz-adv-x': 'horizAdvX', + horizadvx: 'horizAdvX', + 'horiz-origin-x': 'horizOriginX', + horizoriginx: 'horizOriginX', + 'image-rendering': 'imageRendering', + imagerendering: 'imageRendering', + kernelmatrix: 'kernelMatrix', + kernelunitlength: 'kernelUnitLength', + keypoints: 'keyPoints', + keysplines: 'keySplines', + keytimes: 'keyTimes', + lengthadjust: 'lengthAdjust', + 'letter-spacing': 'letterSpacing', + letterspacing: 'letterSpacing', + 'lighting-color': 'lightingColor', + lightingcolor: 'lightingColor', + limitingconeangle: 'limitingConeAngle', + 'marker-end': 'markerEnd', + markerend: 'markerEnd', + 'marker-mid': 'markerMid', + markermid: 'markerMid', + 'marker-start': 'markerStart', + markerstart: 'markerStart', + markerheight: 'markerHeight', + markerunits: 'markerUnits', + markerwidth: 'markerWidth', + maskcontentunits: 'maskContentUnits', + maskunits: 'maskUnits', + numoctaves: 'numOctaves', + 'overline-position': 'overlinePosition', + overlineposition: 'overlinePosition', + 'overline-thickness': 'overlineThickness', + overlinethickness: 'overlineThickness', + 'paint-order': 'paintOrder', + paintorder: 'paintOrder', + 'panose-1': 'panose1', + pathlength: 'pathLength', + patterncontentunits: 'patternContentUnits', + patterntransform: 'patternTransform', + patternunits: 'patternUnits', + 'pointer-events': 'pointerEvents', + pointerevents: 'pointerEvents', + pointsatx: 'pointsAtX', + pointsaty: 'pointsAtY', + pointsatz: 'pointsAtZ', + preservealpha: 'preserveAlpha', + preserveaspectratio: 'preserveAspectRatio', + primitiveunits: 'primitiveUnits', + refx: 'refX', + refy: 'refY', + 'rendering-intent': 'renderingIntent', + renderingintent: 'renderingIntent', + repeatcount: 'repeatCount', + repeatdur: 'repeatDur', + requiredextensions: 'requiredExtensions', + requiredfeatures: 'requiredFeatures', + 'shape-rendering': 'shapeRendering', + shaperendering: 'shapeRendering', + specularconstant: 'specularConstant', + specularexponent: 'specularExponent', + spreadmethod: 'spreadMethod', + startoffset: 'startOffset', + stddeviation: 'stdDeviation', + stitchtiles: 'stitchTiles', + 'stop-color': 'stopColor', + stopcolor: 'stopColor', + stopopacity: 'stopOpacity', + 'stop-opacity': 'stopOpacity', + 'strikethrough-position': 'strikethroughPosition', + strikethroughposition: 'strikethroughPosition', + strikethroughthickness: 'strikethroughThickness', + 'strikethrough-thickness': 'strikethroughThickness', + 'stroke-dasharray': 'strokeDasharray', + strokedasharray: 'strokeDasharray', + 'stroke-dashoffset': 'strokeDashoffset', + strokedashoffset: 'strokeDashoffset', + 'stroke-linecap': 'strokeLinecap', + strokelinecap: 'strokeLinecap', + 'stroke-linejoin': 'strokeLinejoin', + strokelinejoin: 'strokeLinejoin', + 'stroke-miterlimit': 'strokeMiterlimit', + strokemiterlimit: 'strokeMiterlimit', + 'stroke-opacity': 'strokeOpacity', + strokeopacity: 'strokeOpacity', + 'stroke-width': 'strokeWidth', + surfacescale: 'surfaceScale', + systemlanguage: 'systemLanguage', + tablevalues: 'tableValues', + targetx: 'targetX', + targety: 'targetY', + 'text-anchor': 'textAnchor', + textanchor: 'textAnchor', + 'text-decoration': 'textDecoration', + textdecoration: 'textDecoration', + 'text-rendering': 'textRendering', + textrendering: 'textRendering', + textlength: 'textLength', + 'underline-position': 'underlinePosition', + underlineposition: 'underlinePosition', + 'underline-thickness': 'underlineThickness', + underlinethickness: 'underlineThickness', + 'unicode-bidi': 'unicodeBidi', + unicodebidi: 'unicodeBidi', + 'unicode-range': 'unicodeRange', + unicoderange: 'unicodeRange', + 'units-per-em': 'unitsPerEm', + unitsperem: 'unitsPerEm', + 'v-alphabetic': 'vAlphabetic', + valphabetic: 'vAlphabetic', + 'v-hanging': 'vHanging', + vhanging: 'vHanging', + 'v-ideographic': 'vIdeographic', + videographic: 'vIdeographic', + 'v-mathematical': 'vMathematical', + vmathematical: 'vMathematical', + 'vector-effect': 'vectorEffect', + vectoreffect: 'vectorEffect', + 'vert-adv-y': 'vertAdvY', + vertadvy: 'vertAdvY', + 'vert-origin-x': 'vertOriginX', + vertoriginx: 'vertOriginX', + 'vert-origin-y': 'vertOriginY', + vertoriginy: 'vertOriginY', + viewbox: 'viewBox', + viewtarget: 'viewTarget', + 'word-spacing': 'wordSpacing', + wordspacing: 'wordSpacing', + 'writing-mode': 'writingMode', + writingmode: 'writingMode', + 'x-height': 'xHeight', + xheight: 'xHeight', + xchannelselector: 'xChannelSelector', + 'xlink:actuate': 'xlinkActuate', + xlinkactuate: 'xlinkActuate', + 'xlink:arcrole': 'xlinkArcrole', + xlinkarcrole: 'xlinkArcrole', + 'xlink:href': 'xlinkHref', + xlinkhref: 'xlinkHref', + 'xlink:role': 'xlinkRole', + xlinkrole: 'xlinkRole', + 'xlink:show': 'xlinkShow', + xlinkshow: 'xlinkShow', + 'xlink:title': 'xlinkTitle', + xlinktitle: 'xlinkTitle', + 'xlink:type': 'xlinkType', + xlinktype: 'xlinkType', + 'xml:base': 'xmlBase', + xmlbase: 'xmlBase', + 'xmlns:xlink': 'xmlnsXlink', + xmlnsxlink: 'xmlnsXlink', + 'xml:lang': 'xmlLang', + xmllang: 'xmlLang', + 'xml:space': 'xmlSpace', + xmlspace: 'xmlSpace', + ychannelselector: 'yChannelSelector', + zoomandpan: 'zoomAndPan', +}; + +module.exports = possibleStandardNames; From a3c2aef83f10c072be0d1aeb4b3749df7ef5769e Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:17:26 -0400 Subject: [PATCH 26/49] Add back a few attributes and explain why they are needed --- .../dom/shared/HTMLDOMPropertyConfig.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index fb01ea43a89a5..6caed3e37d0cc 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -23,10 +23,6 @@ var HAS_OVERLOADED_BOOLEAN_VALUE = var HTMLDOMPropertyConfig = { Properties: { - /** - * Standard Properties - */ - acceptCharset: 0, allowFullScreen: HAS_BOOLEAN_VALUE, // specifies target context for links with `preload` type async: HAS_BOOLEAN_VALUE, @@ -37,15 +33,12 @@ var HTMLDOMPropertyConfig = { checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, cols: HAS_POSITIVE_NUMERIC_VALUE, controls: HAS_BOOLEAN_VALUE, - className: 0, default: HAS_BOOLEAN_VALUE, defer: HAS_BOOLEAN_VALUE, disabled: HAS_BOOLEAN_VALUE, download: HAS_OVERLOADED_BOOLEAN_VALUE, formNoValidate: HAS_BOOLEAN_VALUE, hidden: HAS_BOOLEAN_VALUE, - htmlFor: 0, - httpEquiv: 0, // Caution; `option.selected` is not updated if `select.multiple` is // disabled with `removeAttribute`. multiple: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, @@ -67,12 +60,23 @@ var HTMLDOMPropertyConfig = { // Style must be explicitely set in the attribute list. React components // expect a style object start: HAS_NUMERIC_VALUE, + // Style must be explicitely set in the attribute list. React components + // expect a style object + style: 0, // itemScope is for for Microdata support. // See http://schema.org/docs/gs.html itemScope: HAS_BOOLEAN_VALUE, // Facebook internal attribute. This is an object attribute that // impliments a custom toString() method ajaxify: 0, + // These attributes must stay in the white-list because they have + // different attribute names (see DOMAttributeNames below) + acceptCharset: 0, + className: 0, + htmlFor: 0, + httpEquiv: 0, + // Attributes with mutation methods must be specified in the whitelist + value: 0, }, DOMAttributeNames: { acceptCharset: 'accept-charset', From ca601c6f3564ee943cbe85c684f0a0f35b3c1041 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:28:47 -0400 Subject: [PATCH 27/49] Remove possibleStandardNames from DOMProperty.js --- src/renderers/dom/shared/DOMProperty.js | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 1edc7261d1fd0..1ba92f62c0441 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -112,10 +112,6 @@ var DOMPropertyInjection = { propName, ); - if (__DEV__) { - DOMProperty.getPossibleStandardName[lowerCased] = propName; - } - if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; @@ -126,9 +122,6 @@ var DOMPropertyInjection = { } propertyInfo.attributeName = attributeName; - if (__DEV__) { - DOMProperty.getPossibleStandardName[attributeName] = propName; - } } if (DOMAttributeNamespaces.hasOwnProperty(propName)) { @@ -211,17 +204,6 @@ var DOMProperty = { */ aliases: {}, - /** - * Mapping from lowercase property names to the properly cased version, used - * to warn in the case of missing properties. Available only in __DEV__. - * - * autofocus is predefined, because adding it to the property whitelist - * causes unintended side effects. - * - * @type {Object} - */ - getPossibleStandardName: __DEV__ ? {autofocus: 'autoFocus'} : null, - /** * Checks whether a property name is a writeable attribute. * @method From 3e866cfcd75a28dc00a9f17647fd8f2fd4f5375a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:54:27 -0400 Subject: [PATCH 28/49] Fix typo in HTMLPropertyConfig comment --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 6caed3e37d0cc..add967c6379f8 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -60,7 +60,7 @@ var HTMLDOMPropertyConfig = { // Style must be explicitely set in the attribute list. React components // expect a style object start: HAS_NUMERIC_VALUE, - // Style must be explicitely set in the attribute list. React components + // Style must be explicitly set in the attribute list. React components // expect a style object style: 0, // itemScope is for for Microdata support. From b7d6996393b2acf9af8bd607ca389aa64e58532a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 14:08:11 -0400 Subject: [PATCH 29/49] Remove duplicative comment --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index add967c6379f8..c8680247cb737 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -55,11 +55,9 @@ var HTMLDOMPropertyConfig = { seamless: HAS_BOOLEAN_VALUE, selected: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, size: HAS_POSITIVE_NUMERIC_VALUE, + start: HAS_NUMERIC_VALUE, // support for projecting regular DOM Elements via V1 named slots ( shadow dom ) span: HAS_POSITIVE_NUMERIC_VALUE, - // Style must be explicitely set in the attribute list. React components - // expect a style object - start: HAS_NUMERIC_VALUE, // Style must be explicitly set in the attribute list. React components // expect a style object style: 0, From 545bcab51b10a0b0c3e74612a6d96e64ce566f4d Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 14:52:05 -0400 Subject: [PATCH 30/49] Add back loop boolean property --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index c8680247cb737..6ba987fd9ea9c 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -56,6 +56,7 @@ var HTMLDOMPropertyConfig = { selected: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, size: HAS_POSITIVE_NUMERIC_VALUE, start: HAS_NUMERIC_VALUE, + loop: HAS_BOOLEAN_VALUE, // support for projecting regular DOM Elements via V1 named slots ( shadow dom ) span: HAS_POSITIVE_NUMERIC_VALUE, // Style must be explicitly set in the attribute list. React components From afb609e09335c3225c3f842c07d2e4513e98a26b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sun, 6 Aug 2017 22:15:27 -0400 Subject: [PATCH 31/49] Allow improperly cased aliased attributes. Add additional tests --- .../dom/fiber/ReactDOMFiberComponent.js | 2 +- .../dom/shared/DOMMarkupOperations.js | 4 +- src/renderers/dom/shared/DOMProperty.js | 40 +++++++----- .../dom/shared/DOMPropertyOperations.js | 12 +--- .../dom/shared/SVGDOMPropertyConfig.js | 4 -- .../ReactDOMServerIntegration-test.js | 62 +++++++++++++++---- .../hooks/ReactDOMUnknownPropertyHook.js | 4 +- 7 files changed, 80 insertions(+), 48 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 8606b5448550b..7cffee7bf4ef7 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1019,7 +1019,7 @@ var ReactDOMFiberComponent = { warnForPropDifference(propKey, serverValue, nextProp); } } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { - if ((propertyInfo = DOMProperty.properties[propKey])) { + if ((propertyInfo = DOMProperty.getPropertyInfo(propKey))) { // $FlowFixMe - Should be inferred as not undefined. extraAttributeNames.delete(propertyInfo.attributeName); serverValue = DOMPropertyOperations.getValueForProperty( diff --git a/src/renderers/dom/shared/DOMMarkupOperations.js b/src/renderers/dom/shared/DOMMarkupOperations.js index 2e0a8ca762571..9323f7182776c 100644 --- a/src/renderers/dom/shared/DOMMarkupOperations.js +++ b/src/renderers/dom/shared/DOMMarkupOperations.js @@ -88,9 +88,7 @@ var DOMMarkupOperations = { * @return {?string} Markup string, or null if the property was invalid. */ createMarkupForProperty: function(name, value) { - var propertyInfo = DOMProperty.properties.hasOwnProperty(name) - ? DOMProperty.properties[name] - : null; + var propertyInfo = DOMProperty.getPropertyInfo(name); if (propertyInfo) { if (shouldIgnoreValue(propertyInfo, value)) { return ''; diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 1ba92f62c0441..84e34a72599f6 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -13,16 +13,18 @@ var invariant = require('fbjs/lib/invariant'); +// These attributes should be all lowercase to allow for +// case insensitive checks var RESERVED_PROPS = { children: true, - dangerouslySetInnerHTML: true, - autoFocus: true, - defaultValue: true, - defaultChecked: true, - innerHTML: true, - suppressContentEditableWarning: true, - onFocusIn: true, - onFocusOut: true, + dangerouslysetinnerhtml: true, + autofocus: true, + defaultvalue: true, + defaultchecked: true, + innerhtml: true, + suppresscontenteditablewarning: true, + onfocusin: true, + onfocusout: true, }; function checkMask(value, bitmask) { @@ -117,10 +119,6 @@ var DOMPropertyInjection = { DOMProperty.aliases[attributeName.toLowerCase()] = true; - if (lowerCased !== attributeName) { - DOMProperty.aliases[lowerCased] = true; - } - propertyInfo.attributeName = attributeName; } @@ -136,7 +134,7 @@ var DOMPropertyInjection = { propertyInfo.mutationMethod = DOMMutationMethods[propName]; } - DOMProperty.properties[propName] = propertyInfo; + DOMProperty.properties[lowerCased] = propertyInfo; } }, }; @@ -209,14 +207,16 @@ var DOMProperty = { * @method */ shouldSetAttribute: function(name, value) { + let lowerCased = name.toLowerCase(); + if ( DOMProperty.isReservedProp(name) || - DOMProperty.aliases.hasOwnProperty(name) + DOMProperty.aliases.hasOwnProperty(lowerCased) ) { return false; } - if (DOMProperty.properties.hasOwnProperty(name)) { + if (DOMProperty.properties.hasOwnProperty(lowerCased)) { return true; } @@ -235,6 +235,14 @@ var DOMProperty = { } }, + getPropertyInfo(name) { + var lowerCased = name.toLowerCase(); + + return DOMProperty.properties.hasOwnProperty(lowerCased) + ? DOMProperty.properties[lowerCased] + : null; + }, + /** * Checks to see if a property name is within the list of properties * reserved for internal React operations. These properties should @@ -245,7 +253,7 @@ var DOMProperty = { * @return {boolean} If the name is within reserved props */ isReservedProp(name) { - return RESERVED_PROPS.hasOwnProperty(name); + return RESERVED_PROPS.hasOwnProperty(name.toLowerCase()); }, injection: DOMPropertyInjection, diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index f09255c871bf2..5f93ab1f468fc 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -79,9 +79,7 @@ var DOMPropertyOperations = { */ getValueForProperty: function(node, name, expected) { if (__DEV__) { - var propertyInfo = DOMProperty.properties.hasOwnProperty(name) - ? DOMProperty.properties[name] - : null; + var propertyInfo = DOMProperty.getPropertyInfo(name); if (propertyInfo) { var mutationMethod = propertyInfo.mutationMethod; if (mutationMethod || propertyInfo.mustUseProperty) { @@ -164,9 +162,7 @@ var DOMPropertyOperations = { * @param {*} value */ setValueForProperty: function(node, name, value) { - var propertyInfo = DOMProperty.properties.hasOwnProperty(name) - ? DOMProperty.properties[name] - : null; + var propertyInfo = DOMProperty.getPropertyInfo(name); if (propertyInfo) { var mutationMethod = propertyInfo.mutationMethod; if (mutationMethod) { @@ -255,9 +251,7 @@ var DOMPropertyOperations = { * @param {string} name */ deleteValueForProperty: function(node, name) { - var propertyInfo = DOMProperty.properties.hasOwnProperty(name) - ? DOMProperty.properties[name] - : null; + var propertyInfo = DOMProperty.getPropertyInfo(name); if (propertyInfo) { var mutationMethod = propertyInfo.mutationMethod; if (mutationMethod) { diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index d11b344215e01..7fb37de7b6bac 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -73,7 +73,6 @@ var ATTRS = { overlineThickness: 'overline-thickness', paintOrder: 'paint-order', panose1: 'panose-1', - pathLength: 'pathLength', pointerEvents: 'pointer-events', renderingIntent: 'rendering-intent', shapeRendering: 'shape-rendering', @@ -91,7 +90,6 @@ var ATTRS = { textAnchor: 'text-anchor', textDecoration: 'text-decoration', textRendering: 'text-rendering', - textLength: 'textLength', underlinePosition: 'underline-position', underlineThickness: 'underline-thickness', unicodeBidi: 'unicode-bidi', @@ -105,8 +103,6 @@ var ATTRS = { vertAdvY: 'vert-adv-y', vertOriginX: 'vert-origin-x', vertOriginY: 'vert-origin-y', - viewBox: 'viewBox', - viewTarget: 'viewTarget', wordSpacing: 'word-spacing', writingMode: 'writing-mode', xHeight: 'x-height', diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index be43ddaf30a10..506dff65a5205 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -615,9 +615,9 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('className')).toBe(false); }); - itRenders('no classname prop', async render => { + itRenders('badly cased className with a warning', async render => { const e = await render(
, 1); - expect(e.hasAttribute('class')).toBe(false); + expect(e.getAttribute('class')).toBe('test'); expect(e.hasAttribute('classname')).toBe(false); }); @@ -626,6 +626,14 @@ describe('ReactDOMServerIntegration', () => { expect(e.className).toBe(''); }); + itRenders( + 'no className prop when given a badly cased alias', + async render => { + const e = await render(
, 1); + expect(e.className).toBe(''); + }, + ); + itRenders('class for custom elements', async render => { const e = await render(
, 0); expect(e.getAttribute('class')).toBe('test'); @@ -643,6 +651,11 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('for')).toBe('myFor'); }); + itRenders('htmlfor with warning', async render => { + const e = await render(
, 1); + expect(e.getAttribute('for')).toBe('myFor'); + }); + itRenders('htmlFor with an empty string', async render => { const e = await render(
); expect(e.getAttribute('for')).toBe(''); @@ -800,21 +813,27 @@ describe('ReactDOMServerIntegration', () => { }); describe('cased attributes', function() { - itRenders('no badly cased aliased HTML attribute', async render => { - const e = await render(, 1); - expect(e.hasAttribute('http-equiv')).toBe(false); - expect(e.hasAttribute('httpequiv')).toBe(false); - }); + itRenders( + 'badly cased aliased HTML attribute with a warning', + async render => { + const e = await render(, 1); + expect(e.getAttribute('http-equiv')).toBe('refresh'); + expect(e.hasAttribute('httpequiv')).toBe(false); + }, + ); - itRenders('no badly cased SVG attribute', async render => { + itRenders('badly cased SVG attribute with a warning', async render => { const e = await render(, 1); - expect(e.hasAttribute('textLength')).toBe(false); + expect(e.getAttribute('textLength')).toBe('10'); }); - itRenders('no badly cased aliased SVG attribute', async render => { - const e = await render(, 1); - expect(e.hasAttribute('strokedasharray')).toBe(false); - }); + itRenders( + 'badly cased aliased SVG attribute with a warning', + async render => { + const e = await render(, 1); + expect(e.getAttribute('stroke-dasharray')).toBe('10 10'); + }, + ); itRenders( 'no badly cased original SVG attribute that is aliased', @@ -1247,6 +1266,23 @@ describe('ReactDOMServerIntegration', () => { ); }); + itRenders( + 'svg element with a badly cased xlink with a warning', + async render => { + let e = await render( + , + 1, + ); + e = e.firstChild; + expect(e.childNodes.length).toBe(0); + expect(e.tagName).toBe('image'); + expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg'); + expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe( + 'http://i.imgur.com/w7GCRPb.png', + ); + }, + ); + itRenders('a math element', async render => { const e = await render(); expect(e.childNodes.length).toBe(0); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index f32128785c8bc..febe782e60c6d 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -88,8 +88,8 @@ if (__DEV__) { } // Known attributes should match the casing specified in the property config. - if (possibleStandardNamesName.hasOwnProperty(lowerCasedName)) { - var standardName = getpossibleStandardNames[lowerCasedName]; + if (possibleStandardNames.hasOwnProperty(lowerCasedName)) { + var standardName = possibleStandardNames[lowerCasedName]; if (standardName !== name) { warning( false, From 71871c4998df8bccfca3899e87ba9e54d64309f0 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sun, 6 Aug 2017 22:34:03 -0400 Subject: [PATCH 32/49] Handle special properties like onFocusOut --- .../dom/shared/hooks/ReactDOMUnknownPropertyHook.js | 7 ++++++- src/renderers/dom/shared/hooks/possibleStandardNames.js | 5 ++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index febe782e60c6d..fe843b458b11d 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -39,6 +39,10 @@ if (__DEV__) { var EVENT_NAME_REGEX = /^on[A-Z]/; var ARIA_NAME_REGEX = /^aria-/i; var possibleStandardNames = require('possibleStandardNames'); + var unsupportedProps = { + onfocusin: true, + onfocusout: true, + }; var validateProperty = function(tagName, name, value, debugID) { if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { @@ -78,7 +82,8 @@ if (__DEV__) { return true; } - if (DOMProperty.isReservedProp(name)) { + // Unsupported props are handled by another validation + if (unsupportedProps.hasOwnProperty(lowerCasedName)) { return true; } diff --git a/src/renderers/dom/shared/hooks/possibleStandardNames.js b/src/renderers/dom/shared/hooks/possibleStandardNames.js index 3f83cee2adcd0..1e7ef05597ad5 100644 --- a/src/renderers/dom/shared/hooks/possibleStandardNames.js +++ b/src/renderers/dom/shared/hooks/possibleStandardNames.js @@ -28,11 +28,13 @@ var possibleStandardNames = { class: 'className', classid: 'classID', classname: 'className', + children: 'children', colspan: 'colSpan', contenteditable: 'contentEditable', contextmenu: 'contextMenu', controlslist: 'controlsList', crossorigin: 'crossOrigin', + dangerouslysetinnerhtml: 'dangerouslySetInnerHTML', datetime: 'dateTime', defaultvalue: 'defaultValue', defaultchecked: 'defaultChecked', @@ -81,7 +83,7 @@ var possibleStandardNames = { alignmentbaseline: 'alignmentBaseline', 'alignment-baseline': 'alignmentBaseline', allowreorder: 'allowReorder', - 'aribic-form': 'arabicForm', + 'arabic-form': 'arabicForm', arabicform: 'arabicForm', attributename: 'attributeName', attributetype: 'attributeType', @@ -233,6 +235,7 @@ var possibleStandardNames = { 'stroke-opacity': 'strokeOpacity', strokeopacity: 'strokeOpacity', 'stroke-width': 'strokeWidth', + suppresscontenteditablewarning: 'suppressContentEditableWarning', surfacescale: 'surfaceScale', systemlanguage: 'systemLanguage', tablevalues: 'tableValues', From 32813208a74073b80f7664ecc1a90bedfb29df00 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:10:17 -0400 Subject: [PATCH 33/49] Add some comments to document where casing matters. Remove DOMPropertyNames --- src/renderers/dom/shared/DOMProperty.js | 11 +++++++---- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 84e34a72599f6..2e3059e8b2f21 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -117,6 +117,9 @@ var DOMPropertyInjection = { if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; + // Track as lowercase to prevent assignment for aliasesd attitibutes + // like `http-equiv`. This covers cases like `hTTp-equiv`, which + // should not write `http-equiv` to the DOM. DOMProperty.aliases[attributeName.toLowerCase()] = true; propertyInfo.attributeName = attributeName; @@ -126,14 +129,14 @@ var DOMPropertyInjection = { propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName]; } - if (DOMPropertyNames.hasOwnProperty(propName)) { - propertyInfo.propertyName = DOMPropertyNames[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 + // for `allowFullscreen` DOMProperty.properties[lowerCased] = propertyInfo; } }, diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 6ba987fd9ea9c..931390bcc5741 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -66,7 +66,8 @@ var HTMLDOMPropertyConfig = { // See http://schema.org/docs/gs.html itemScope: HAS_BOOLEAN_VALUE, // Facebook internal attribute. This is an object attribute that - // impliments a custom toString() method + // impliments a custom toString() method. Objects are not allowed as + // custom attributes unless they are specified by an attribute config ajaxify: 0, // These attributes must stay in the white-list because they have // different attribute names (see DOMAttributeNames below) @@ -83,7 +84,6 @@ var HTMLDOMPropertyConfig = { htmlFor: 'for', httpEquiv: 'http-equiv', }, - DOMPropertyNames: {}, DOMMutationMethods: { value: function(node, value) { if (value == null) { From 19bfbafb0fc613ce5b33e999bd18fd83266812fd Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:18:23 -0400 Subject: [PATCH 34/49] Fix spelling mistake in ajaxify HTML property comment --- src/renderers/dom/shared/HTMLDOMPropertyConfig.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 931390bcc5741..1fe69bedb1a20 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -66,7 +66,7 @@ var HTMLDOMPropertyConfig = { // See http://schema.org/docs/gs.html itemScope: HAS_BOOLEAN_VALUE, // Facebook internal attribute. This is an object attribute that - // impliments a custom toString() method. Objects are not allowed as + // implements a custom toString() method. Objects are not allowed as // custom attributes unless they are specified by an attribute config ajaxify: 0, // These attributes must stay in the white-list because they have From 6df8b4fc126058313341159a69d3030dbd30681a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:52:06 -0400 Subject: [PATCH 35/49] Remove alias test that covers multiple aliases for one property --- .../dom/shared/__tests__/DOMPropertyOperations-test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 359e1c98e3b18..5b2c96a262a87 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -67,13 +67,13 @@ describe('DOMPropertyOperations', () => { // This ensures that we have consistent behavior. var obj = { toString: function() { - return ''; + return 'css-class'; }, }; var container = document.createElement('div'); - ReactDOM.render(
, container); - expect(container.firstChild.getAttribute('role')).toBe(''); + ReactDOM.render(
, container); + expect(container.firstChild.getAttribute('class')).toBe('css-class'); }); it('should not remove empty attributes for special properties', () => { From cb570759125a926be7e97d77e50056a966f564ea Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:52:31 -0400 Subject: [PATCH 36/49] Fix typo in comment --- src/renderers/dom/shared/DOMProperty.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 2e3059e8b2f21..89c14937226b1 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -117,7 +117,7 @@ var DOMPropertyInjection = { if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; - // Track as lowercase to prevent assignment for aliasesd attitibutes + // Track as lowercase to prevent assignment for aliased attributes // like `http-equiv`. This covers cases like `hTTp-equiv`, which // should not write `http-equiv` to the DOM. DOMProperty.aliases[attributeName.toLowerCase()] = true; From 21678fc4c04645054cd812288a681e1c4b413f11 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:53:32 -0400 Subject: [PATCH 37/49] Build SVG aliases dynamically --- .../dom/shared/SVGDOMPropertyConfig.js | 200 ++++++++---------- 1 file changed, 93 insertions(+), 107 deletions(-) diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index 7fb37de7b6bac..73b97e7d21885 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -16,108 +16,91 @@ var NS = { xml: 'http://www.w3.org/XML/1998/namespace', }; -// We use attributes for everything SVG so let's avoid some duplication and run -// code instead. -// The following are all specified in the HTML config already so we exclude here. -// - class (as className) -// - color -// - height -// - id -// - lang -// - max -// - media -// - method -// - min -// - name -// - style -// - target -// - type -// - width -var ATTRS = { - accentHeight: 'accent-height', - alignmentBaseline: 'alignment-baseline', - arabicForm: 'arabic-form', - baselineShift: 'baseline-shift', - capHeight: 'cap-height', - clipPath: 'clip-path', - clipRule: 'clip-rule', - colorInterpolation: 'color-interpolation', - colorInterpolationFilters: 'color-interpolation-filters', - colorProfile: 'color-profile', - colorRendering: 'color-rendering', - dominantBaseline: 'dominant-baseline', - enableBackground: 'enable-background', - fillOpacity: 'fill-opacity', - fillRule: 'fill-rule', - floodColor: 'flood-color', - floodOpacity: 'flood-opacity', - fontFamily: 'font-family', - fontSize: 'font-size', - fontSizeAdjust: 'font-size-adjust', - fontStretch: 'font-stretch', - fontStyle: 'font-style', - fontVariant: 'font-variant', - fontWeight: 'font-weight', - glyphName: 'glyph-name', - glyphOrientationHorizontal: 'glyph-orientation-horizontal', - glyphOrientationVertical: 'glyph-orientation-vertical', - horizAdvX: 'horiz-adv-x', - horizOriginX: 'horiz-origin-x', - imageRendering: 'image-rendering', - letterSpacing: 'letter-spacing', - lightingColor: 'lighting-color', - markerEnd: 'marker-end', - markerMid: 'marker-mid', - markerStart: 'marker-start', - overlinePosition: 'overline-position', - overlineThickness: 'overline-thickness', - paintOrder: 'paint-order', - panose1: 'panose-1', - pointerEvents: 'pointer-events', - renderingIntent: 'rendering-intent', - shapeRendering: 'shape-rendering', - stopColor: 'stop-color', - stopOpacity: 'stop-opacity', - strikethroughPosition: 'strikethrough-position', - strikethroughThickness: 'strikethrough-thickness', - strokeDasharray: 'stroke-dasharray', - strokeDashoffset: 'stroke-dashoffset', - strokeLinecap: 'stroke-linecap', - strokeLinejoin: 'stroke-linejoin', - strokeMiterlimit: 'stroke-miterlimit', - strokeOpacity: 'stroke-opacity', - strokeWidth: 'stroke-width', - textAnchor: 'text-anchor', - textDecoration: 'text-decoration', - textRendering: 'text-rendering', - underlinePosition: 'underline-position', - underlineThickness: 'underline-thickness', - unicodeBidi: 'unicode-bidi', - unicodeRange: 'unicode-range', - unitsPerEm: 'units-per-em', - vAlphabetic: 'v-alphabetic', - vHanging: 'v-hanging', - vIdeographic: 'v-ideographic', - vMathematical: 'v-mathematical', - vectorEffect: 'vector-effect', - vertAdvY: 'vert-adv-y', - vertOriginX: 'vert-origin-x', - vertOriginY: 'vert-origin-y', - wordSpacing: 'word-spacing', - writingMode: 'writing-mode', - xHeight: 'x-height', - xlinkActuate: 'xlink:actuate', - xlinkArcrole: 'xlink:arcrole', - xlinkHref: 'xlink:href', - xlinkRole: 'xlink:role', - xlinkShow: 'xlink:show', - xlinkTitle: 'xlink:title', - xlinkType: 'xlink:type', - xmlBase: 'xml:base', - xmlnsXlink: 'xmlns:xlink', - xmlLang: 'xml:lang', - xmlSpace: 'xml:space', -}; +var ATTRS = [ + 'accent-height', + 'alignment-baseline', + 'arabic-form', + 'baseline-shift', + 'cap-height', + 'clip-path', + 'clip-rule', + 'color-interpolation', + 'color-interpolation-filters', + 'color-profile', + 'color-rendering', + 'dominant-baseline', + 'enable-background', + 'fill-opacity', + 'fill-rule', + 'flood-color', + 'flood-opacity', + 'font-family', + 'font-size', + 'font-size-adjust', + 'font-stretch', + 'font-style', + 'font-variant', + 'font-weight', + 'glyph-name', + 'glyph-orientation-horizontal', + 'glyph-orientation-vertical', + 'horiz-adv-x', + 'horiz-origin-x', + 'image-rendering', + 'letter-spacing', + 'lighting-color', + 'marker-end', + 'marker-mid', + 'marker-start', + 'overline-position', + 'overline-thickness', + 'paint-order', + 'panose-1', + 'pointer-events', + 'rendering-intent', + 'shape-rendering', + 'stop-color', + 'stop-opacity', + 'strikethrough-position', + 'strikethrough-thickness', + 'stroke-dasharray', + 'stroke-dashoffset', + 'stroke-linecap', + 'stroke-linejoin', + 'stroke-miterlimit', + 'stroke-opacity', + 'stroke-width', + 'text-anchor', + 'text-decoration', + 'text-rendering', + 'underline-position', + 'underline-thickness', + 'unicode-bidi', + 'unicode-range', + 'units-per-em', + 'v-alphabetic', + 'v-hanging', + 'v-ideographic', + 'v-mathematical', + 'vector-effect', + 'vert-adv-y', + 'vert-origin-x', + 'vert-origin-y', + 'word-spacing', + 'writing-mode', + 'x-height', + 'xlink:actuate', + 'xlink:arcrole', + 'xlink:href', + 'xlink:role', + 'xlink:show', + 'xlink:title', + 'xlink:type', + 'xml:base', + 'xmlns:xlink', + 'xml:lang', + 'xml:space', +]; var SVGDOMPropertyConfig = { Properties: {}, @@ -136,11 +119,14 @@ var SVGDOMPropertyConfig = { DOMAttributeNames: {}, }; -Object.keys(ATTRS).forEach(key => { - SVGDOMPropertyConfig.Properties[key] = 0; - if (ATTRS[key]) { - SVGDOMPropertyConfig.DOMAttributeNames[key] = ATTRS[key]; - } +var CAMELIZE = /[\-\:]([a-z])/g; +var capitalize = token => token[1].toUpperCase(); + +ATTRS.forEach(original => { + var reactName = original.replace(CAMELIZE, capitalize); + + SVGDOMPropertyConfig.Properties[reactName] = 0; + SVGDOMPropertyConfig.DOMAttributeNames[reactName] = original; }); module.exports = SVGDOMPropertyConfig; From 90b4cce736588d838d565936d82e9e81bd23b572 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 08:14:28 -0400 Subject: [PATCH 38/49] Remove unused DOMPropertyNames reference --- src/renderers/dom/shared/DOMProperty.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 89c14937226b1..e017f0829ea66 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -70,7 +70,6 @@ var DOMPropertyInjection = { var Properties = domPropertyConfig.Properties || {}; var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {}; var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {}; - var DOMPropertyNames = domPropertyConfig.DOMPropertyNames || {}; var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {}; for (var propName in Properties) { From fa3db62f187d9fc1af9950b2cb18374443ab16a2 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 10:27:35 -0400 Subject: [PATCH 39/49] Do not translate bad casings of aliased attributes - classname writes to the DOM as classname - class does not write to the DOM - cLASS does not write to the DOM - arabic-form does not write to the DOM --- src/renderers/dom/shared/DOMProperty.js | 32 +++++----- .../__tests__/ReactDOMComponent-test.js | 27 ++++++++- .../ReactDOMServerIntegration-test.js | 58 +++++++++---------- 3 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index e017f0829ea66..a1b825c9668df 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -116,12 +116,11 @@ var DOMPropertyInjection = { if (DOMAttributeNames.hasOwnProperty(propName)) { var attributeName = DOMAttributeNames[propName]; - // Track as lowercase to prevent assignment for aliased attributes - // like `http-equiv`. This covers cases like `hTTp-equiv`, which - // should not write `http-equiv` to the DOM. - DOMProperty.aliases[attributeName.toLowerCase()] = true; - propertyInfo.attributeName = attributeName; + + // Use the lowercase form of the attribute name to prevent + // badly cased React attribute alises from writing to the DOM. + DOMProperty.aliases[attributeName.toLowerCase()] = true; } if (DOMAttributeNamespaces.hasOwnProperty(propName)) { @@ -136,7 +135,7 @@ var DOMPropertyInjection = { // without case-sensitivity. This allows the whitelist to pick up // `allowfullscreen`, which should be written using the property configuration // for `allowFullscreen` - DOMProperty.properties[lowerCased] = propertyInfo; + DOMProperty.properties[propName] = propertyInfo; } }, }; @@ -209,21 +208,18 @@ var DOMProperty = { * @method */ shouldSetAttribute: function(name, value) { - let lowerCased = name.toLowerCase(); - - if ( - DOMProperty.isReservedProp(name) || - DOMProperty.aliases.hasOwnProperty(lowerCased) - ) { + if (DOMProperty.isReservedProp(name)) { return false; } - if (DOMProperty.properties.hasOwnProperty(lowerCased)) { + if (value === null || DOMProperty.properties.hasOwnProperty(name)) { return true; } - if (value === null) { - return true; + // Prevent aliases, and badly cased aliases like `class` or `cLASS` + // from showing up in the DOM + if (DOMProperty.aliases.hasOwnProperty(name.toLowerCase())) { + return false; } switch (typeof value) { @@ -238,10 +234,8 @@ var DOMProperty = { }, getPropertyInfo(name) { - var lowerCased = name.toLowerCase(); - - return DOMProperty.properties.hasOwnProperty(lowerCased) - ? DOMProperty.properties[lowerCased] + return DOMProperty.properties.hasOwnProperty(name) + ? DOMProperty.properties[name] : null; }, diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 9cdb405109944..ea4cad48cd9d8 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1880,7 +1880,7 @@ describe('ReactDOMComponent', () => { }); describe('Attributes with aliases', function() { - it('does not set aliased attributes on DOM elements', function() { + it('does not set aliased attributes on HTML attributes', function() { spyOn(console, 'error'); var el = ReactTestUtils.renderIntoDocument(
); @@ -1892,7 +1892,20 @@ describe('ReactDOMComponent', () => { ); }); - it('does not set aliased attributes on SVG elements', function() { + it('does not set incorrectly cased aliased attributes on HTML attributes with a warning', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.className).toBe(''); + expect(el.hasAttribute('class')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid DOM property `cLASS`. Did you mean `className`?', + ); + }); + + it('does not set aliased attributes on SVG elements with a warning', function() { spyOn(console, 'error'); var el = ReactTestUtils.renderIntoDocument( @@ -1900,7 +1913,7 @@ describe('ReactDOMComponent', () => { ); var text = el.querySelector('text'); - expect(text.getAttribute('arabic-form')).toBe(null); + expect(text.hasAttribute('arabic-form')).toBe(false); expectDev(console.error.calls.argsFor(0)[0]).toContain( 'Warning: Invalid DOM property `arabic-form`. Did you mean `arabicForm`?', @@ -1915,6 +1928,14 @@ describe('ReactDOMComponent', () => { expect(el.getAttribute('class')).toBe('test'); }); + it('aliased attributes on custom elements with bad casing', function() { + var el = ReactTestUtils.renderIntoDocument( +
, + ); + + expect(el.getAttribute('class')).toBe('test'); + }); + it('updates aliased attributes on custom elements', function() { var container = document.createElement('div'); ReactDOM.render(
, container); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 506dff65a5205..9c1fd05acac68 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -615,10 +615,10 @@ describe('ReactDOMServerIntegration', () => { expect(e.hasAttribute('className')).toBe(false); }); - itRenders('badly cased className with a warning', async render => { + itRenders('no badly cased className with a warning', async render => { const e = await render(
, 1); - expect(e.getAttribute('class')).toBe('test'); - expect(e.hasAttribute('classname')).toBe(false); + expect(e.hasAttribute('class')).toBe(false); + expect(e.hasAttribute('classname')).toBe(true); }); itRenders('no className prop when given the alias', async render => { @@ -651,9 +651,10 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('for')).toBe('myFor'); }); - itRenders('htmlfor with warning', async render => { + itRenders('no badly cased htmlfor', async render => { const e = await render(
, 1); - expect(e.getAttribute('for')).toBe('myFor'); + expect(e.hasAttribute('for')).toBe(false); + expect(e.getAttribute('htmlfor')).toBe('myFor'); }); itRenders('htmlFor with an empty string', async render => { @@ -817,8 +818,8 @@ describe('ReactDOMServerIntegration', () => { 'badly cased aliased HTML attribute with a warning', async render => { const e = await render(, 1); - expect(e.getAttribute('http-equiv')).toBe('refresh'); - expect(e.hasAttribute('httpequiv')).toBe(false); + expect(e.hasAttribute('http-equiv')).toBe(false); + expect(e.getAttribute('httpequiv')).toBe('refresh'); }, ); @@ -827,13 +828,11 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('textLength')).toBe('10'); }); - itRenders( - 'badly cased aliased SVG attribute with a warning', - async render => { - const e = await render(, 1); - expect(e.getAttribute('stroke-dasharray')).toBe('10 10'); - }, - ); + itRenders('no badly cased aliased SVG attribute alias', async render => { + const e = await render(, 1); + expect(e.hasAttribute('stroke-dasharray')).toBe(false); + expect(e.getAttribute('strokedasharray')).toBe('10 10'); + }); itRenders( 'no badly cased original SVG attribute that is aliased', @@ -1253,7 +1252,7 @@ describe('ReactDOMServerIntegration', () => { expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg'); }); - itRenders('svg element with an xlink', async render => { + itRenders('svg child element', async render => { let e = await render( , ); @@ -1266,22 +1265,19 @@ describe('ReactDOMServerIntegration', () => { ); }); - itRenders( - 'svg element with a badly cased xlink with a warning', - async render => { - let e = await render( - , - 1, - ); - e = e.firstChild; - expect(e.childNodes.length).toBe(0); - expect(e.tagName).toBe('image'); - expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg'); - expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe( - 'http://i.imgur.com/w7GCRPb.png', - ); - }, - ); + itRenders('no svg child element with a badly cased', async render => { + let e = await render( + , + 1, + ); + e = e.firstChild; + expect(e.hasAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe( + false, + ); + expect(e.getAttribute('xlinkhref')).toBe( + 'http://i.imgur.com/w7GCRPb.png', + ); + }); itRenders('a math element', async render => { const e = await render(); From 21db7191812d48da930c57ce1130fefc2d26d6e3 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 20:37:06 -0400 Subject: [PATCH 40/49] Revise the way custom booleans are treated - Custom attributes can not have boolean values unless they are aria or data attributes - Attributes with boolean values have been added back to the whitelist - Warnings now exclude booleans from supported types - Associated test coverage --- src/renderers/dom/shared/DOMProperty.js | 5 +++- .../dom/shared/HTMLDOMPropertyConfig.js | 15 ++++++++++++ .../dom/shared/SVGDOMPropertyConfig.js | 14 +++++++++++ .../__tests__/ReactDOMComponent-test.js | 24 ++++++++++++++----- .../ReactDOMServerIntegration-test.js | 24 ++++++++++++------- .../hooks/ReactDOMUnknownPropertyHook.js | 4 ++-- 6 files changed, 69 insertions(+), 17 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index a1b825c9668df..8afd7b1ffb280 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -31,6 +31,8 @@ function checkMask(value, bitmask) { return (value & bitmask) === bitmask; } +var isDataOrAriaAttribute = /^(data|aria)/i + var DOMPropertyInjection = { /** * Mapping from normalized, camelcased property names to a configuration that @@ -223,8 +225,9 @@ var DOMProperty = { } switch (typeof value) { - case 'undefined': case 'boolean': + return isDataOrAriaAttribute.test(name) + case 'undefined': case 'number': case 'string': return true; diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 1fe69bedb1a20..56696e2907802 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -77,6 +77,21 @@ var HTMLDOMPropertyConfig = { httpEquiv: 0, // Attributes with mutation methods must be specified in the whitelist value: 0, + // The following attributes expect boolean values. They must be in + // the whitelist to allow boolean attribute assignment: + autoComplete: 0, + // IE only true/false iFrame attribute + // https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx + allowTransparency: 0, + contentEditable: 0, + draggable: 0, + spellCheck: 0, + // autoCapitalize and autoCorrect are supported in Mobile Safari for + // keyboard hints. + autoCapitalize: 0, + autoCorrect: 0, + // autoSave allows WebKit/Blink to persist values of input fields on page reloads + autoSave: 0, }, DOMAttributeNames: { acceptCharset: 'accept-charset', diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index 73b97e7d21885..3478d14df3d7f 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -16,6 +16,15 @@ var NS = { xml: 'http://www.w3.org/XML/1998/namespace', }; +/** + * This is a list of all SVG attributes that need special + * casing, namespacing, or boolean value assignment. + * + * SVG Attributes List: + * https://www.w3.org/TR/SVG/attindex.html + * SMIL Spec: + * https://www.w3.org/TR/smil + */ var ATTRS = [ 'accent-height', 'alignment-baseline', @@ -100,6 +109,11 @@ var ATTRS = [ 'xmlns:xlink', 'xml:lang', 'xml:space', + // The following attributes expect boolean values. They must be in + // the whitelist to allow boolean attribute assignment: + 'autoReverse', + 'externalResourcesRequired', + 'preserveAlpha' ]; var SVGDOMPropertyConfig = { diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index ea4cad48cd9d8..43e600105ff60 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -149,7 +149,7 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Invalid prop `foo` on
tag. Either remove this prop ' + - 'from the element, or pass a string, number, or boolean value to keep ' + + 'from the element, or pass a string or number value to keep ' + 'it in the DOM. For details, see https://fb.me/react-unknown-prop' + '\n in div (at **)', ); @@ -162,7 +162,7 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count(0)).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Invalid props `foo`, `baz` on
tag. Either remove these ' + - 'props from the element, or pass a string, number, or boolean value to keep ' + + 'props from the element, or pass a string or number value to keep ' + 'them in the DOM. For details, see https://fb.me/react-unknown-prop' + '\n in div (at **)', ); @@ -1963,17 +1963,29 @@ describe('ReactDOMComponent', () => { expect(container.firstChild.hasAttribute('whatever')).toBe(false); }); - it('assigns a boolean custom attributes as a string', function() { + it('does not assign a boolean custom attributes as a string', function() { + spyOn(console, 'error'); + var el = ReactTestUtils.renderIntoDocument(
); - expect(el.getAttribute('whatever')).toBe('true'); + expect(el.hasAttribute('whatever')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `whatever` on
tag', + ); }); - it('assigns an implicit boolean custom attributes as a string', function() { + it('does not assign an implicit boolean custom attributes', function() { + spyOn(console, 'error'); + // eslint-disable-next-line react/jsx-boolean-value var el = ReactTestUtils.renderIntoDocument(
); - expect(el.getAttribute('whatever')).toBe('true'); + expect(el.hasAttribute('whatever')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `whatever` on
tag', + ); }); it('assigns a numeric custom attributes as a string', function() { diff --git a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js index 9c1fd05acac68..fb985538f08e5 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js @@ -467,16 +467,14 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('width')).toBe('30'); }); - // this seems like it might mask programmer error, but it's existing behavior. - itRenders('string prop with true value', async render => { - const e = await render(); - expect(e.getAttribute('href')).toBe('true'); + itRenders('no string prop with true value', async render => { + const e = await render(, 1); + expect(e.hasAttribute('href')).toBe(false); }); - // this seems like it might mask programmer error, but it's existing behavior. - itRenders('string prop with false value', async render => { - const e = await render(); - expect(e.getAttribute('href')).toBe('false'); + itRenders('no string prop with false value', async render => { + const e = await render(, 1); + expect(e.hasAttribute('href')).toBe(false); }); itRenders('no string prop with null value', async render => { @@ -864,6 +862,16 @@ describe('ReactDOMServerIntegration', () => { expect(e.getAttribute('data-fooBar')).toBe('true'); }); + itRenders('unknown data- attributes with boolean true', async render => { + const e = await render(
); + expect(e.getAttribute('data-fooBar')).toBe('true'); + }); + + itRenders('unknown data- attributes with boolean false', async render => { + const e = await render(
); + expect(e.getAttribute('data-fooBar')).toBe('false'); + }); + itRenders( 'no unknown data- attributes with casing and null value', async render => { diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index fe843b458b11d..a258a3e6ef8fc 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -126,7 +126,7 @@ var warnUnknownProperties = function(type, props, debugID) { warning( false, 'Invalid prop %s on <%s> tag. Either remove this prop from the element, ' + - 'or pass a string, number, or boolean value to keep it in the DOM. ' + + 'or pass a string or number value to keep it in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, type, @@ -136,7 +136,7 @@ var warnUnknownProperties = function(type, props, debugID) { warning( false, 'Invalid props %s on <%s> tag. Either remove these props from the element, ' + - 'or pass a string, number, or boolean value to keep them in the DOM. ' + + 'or pass a string or number value to keep them in the DOM. ' + 'For details, see https://fb.me/react-unknown-prop%s', unknownPropString, type, From c2c3d63bc2bf5b392d9677e2ae76f1139e3ecf2c Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 21:10:59 -0400 Subject: [PATCH 41/49] Add developer warnings for NaN and ARIA hooks --- src/renderers/dom/shared/DOMProperty.js | 4 +-- .../dom/shared/SVGDOMPropertyConfig.js | 2 +- .../__tests__/ReactDOMComponent-test.js | 13 +++++++ .../__tests__/ReactDOMInvalidARIAHook-test.js | 22 ++++++++++++ .../shared/hooks/ReactDOMInvalidARIAHook.js | 35 +++++++++++++++++++ .../hooks/ReactDOMUnknownPropertyHook.js | 11 ++++++ 6 files changed, 84 insertions(+), 3 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index 8afd7b1ffb280..af0f041a1fb81 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -31,7 +31,7 @@ function checkMask(value, bitmask) { return (value & bitmask) === bitmask; } -var isDataOrAriaAttribute = /^(data|aria)/i +var isDataOrAriaAttribute = /^(data|aria)/i; var DOMPropertyInjection = { /** @@ -226,7 +226,7 @@ var DOMProperty = { switch (typeof value) { case 'boolean': - return isDataOrAriaAttribute.test(name) + return isDataOrAriaAttribute.test(name); case 'undefined': case 'number': case 'string': diff --git a/src/renderers/dom/shared/SVGDOMPropertyConfig.js b/src/renderers/dom/shared/SVGDOMPropertyConfig.js index 3478d14df3d7f..657e2f3a5ce18 100644 --- a/src/renderers/dom/shared/SVGDOMPropertyConfig.js +++ b/src/renderers/dom/shared/SVGDOMPropertyConfig.js @@ -113,7 +113,7 @@ var ATTRS = [ // the whitelist to allow boolean attribute assignment: 'autoReverse', 'externalResourcesRequired', - 'preserveAlpha' + 'preserveAlpha', ]; var SVGDOMPropertyConfig = { diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 43e600105ff60..28918cafb03a7 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -2034,5 +2034,18 @@ describe('ReactDOMComponent', () => { var el = ReactTestUtils.renderIntoDocument(
); expect(el.getAttribute('foobar')).toBe('true'); }); + + it('warns on NaN attributes', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument(
); + + expect(el.getAttribute('whatever')).toBe('NaN'); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Received NaN for numeric attribute `whatever`. If this is ' + + 'expected, cast the value to a string.\n in div', + ); + }); }); }); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js b/src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js index 60303ec95e8da..e8c5cb479c3ad 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js @@ -63,5 +63,27 @@ describe('ReactDOMInvalidARIAHook', () => { 'Did you mean aria-haspopup?', ); }); + + it('should warn for use of recognized camel case aria attributes', () => { + spyOn(console, 'error'); + // The valid attribute name is aria-haspopup. + mountComponent({ariaHasPopup: 'true'}); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid ARIA attribute ariaHasPopup. ' + + 'Did you mean aria-haspopup?', + ); + }); + + it('should warn for use of unrecognized camel case aria attributes', () => { + spyOn(console, 'error'); + // The valid attribute name is aria-haspopup. + mountComponent({ariaSomethingInvalid: 'true'}); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid ARIA attribute ariaSomethingInvalid. ARIA ' + + 'attributes follow the pattern aria-* and must be lowercase.', + ); + }); }); }); diff --git a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js index 1a65728f97d17..545d469f95fd2 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMInvalidARIAHook.js @@ -15,6 +15,9 @@ var DOMProperty = require('DOMProperty'); var warnedProperties = {}; var rARIA = new RegExp('^(aria)-[' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$'); +var rARIACamel = new RegExp( + '^(aria)[A-Z][' + DOMProperty.ATTRIBUTE_NAME_CHAR + ']*$', +); if (__DEV__) { var warning = require('fbjs/lib/warning'); @@ -43,6 +46,38 @@ function validateProperty(tagName, name, debugID) { return true; } + if (rARIACamel.test(name)) { + var ariaName = 'aria-' + name.slice(4).toLowerCase(); + var correctName = validAriaProperties.hasOwnProperty(ariaName) + ? ariaName + : null; + + // If this is an aria-* attribute, but is not listed in the known DOM + // DOM properties, then it is an invalid aria-* attribute. + if (correctName == null) { + warning( + false, + 'Invalid ARIA attribute %s. ARIA attributes follow the pattern aria-* and must be lowercase.%s', + name, + getStackAddendum(debugID), + ); + warnedProperties[name] = true; + return true; + } + // aria-* attributes should be lowercase; suggest the lowercase version. + if (name !== correctName) { + warning( + false, + 'Invalid ARIA attribute %s. Did you mean %s?%s', + name, + correctName, + getStackAddendum(debugID), + ); + warnedProperties[name] = true; + return true; + } + } + if (rARIA.test(name)) { var lowerCasedName = name.toLowerCase(); var standardName = validAriaProperties.hasOwnProperty(lowerCasedName) diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index a258a3e6ef8fc..b5a916af28ce9 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -107,6 +107,17 @@ if (__DEV__) { return true; } + if (typeof value === 'number' && isNaN(value)) { + warning( + false, + 'Received NaN for numeric attribute `%s`. If this is expected, cast ' + + 'the value to a string.%s', + name, + getStackAddendum(debugID), + ); + return true; + } + return DOMProperty.shouldSetAttribute(name, value); }; } From 7f16b4de7c9403e1a06e93fbde67ab4edf5269dc Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 8 Aug 2017 11:33:33 +0100 Subject: [PATCH 42/49] Revert changes to the docs We'll update them separately --- docs/warnings/unknown-prop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/warnings/unknown-prop.md b/docs/warnings/unknown-prop.md index 86883cafcdca8..eb7585f650d14 100644 --- a/docs/warnings/unknown-prop.md +++ b/docs/warnings/unknown-prop.md @@ -3,7 +3,7 @@ title: Unknown Prop Warning layout: single permalink: warnings/unknown-prop.html --- -The unknown-prop warning will fire if you attempt to render a DOM element with a prop that is either unrecognized by React as a legal DOM attribute/property, or is not a string, number, or boolean value. You should ensure that your DOM elements do not have spurious props floating around. +The unknown-prop warning will fire if you attempt to render a DOM element with a prop that is not recognized by React as a legal DOM attribute/property. You should ensure that your DOM elements do not have spurious props floating around. There are a couple of likely reasons this warning could be appearing: From 3908cd3d50de25baae6b9b586187e46ba6634bd5 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 8 Aug 2017 07:15:59 -0400 Subject: [PATCH 43/49] Use string comparison instead of regex to check for data and aria attributes. --- src/renderers/dom/shared/DOMProperty.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index af0f041a1fb81..da944b3b78c3d 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -31,8 +31,6 @@ function checkMask(value, bitmask) { return (value & bitmask) === bitmask; } -var isDataOrAriaAttribute = /^(data|aria)/i; - var DOMPropertyInjection = { /** * Mapping from normalized, camelcased property names to a configuration that @@ -218,15 +216,18 @@ var DOMProperty = { return true; } + var lowerCased = name.toLowerCase(); + // Prevent aliases, and badly cased aliases like `class` or `cLASS` // from showing up in the DOM - if (DOMProperty.aliases.hasOwnProperty(name.toLowerCase())) { + if (DOMProperty.aliases.hasOwnProperty(lowerCased)) { return false; } switch (typeof value) { case 'boolean': - return isDataOrAriaAttribute.test(name); + var prefix = lowerCased.slice(0, 5); + return prefix === 'data-' || prefix === 'aria-'; case 'undefined': case 'number': case 'string': From 2479fcceaa02ca3e80d2f2610e82bad033bf77ed Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 8 Aug 2017 08:35:28 -0400 Subject: [PATCH 44/49] Warn about unsupported properties without case sensitivity --- .../__tests__/ReactDOMComponent-test.js | 33 +++++++++++++++ .../hooks/ReactDOMUnknownPropertyHook.js | 40 ++++++++++++++----- .../dom/shared/utils/assertValidProps.js | 11 ----- .../dom/stack/client/ReactDOMComponent.js | 11 ----- 4 files changed, 62 insertions(+), 33 deletions(-) diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 28918cafb03a7..e47f4efee3f90 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1006,6 +1006,15 @@ describe('ReactDOMComponent', () => { ); }); + it('should validate against use of innerHTML without case sensitivity', () => { + spyOn(console, 'error'); + mountComponent({innerhtml: 'Hi Jim!'}); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Directly setting property `innerHTML` is not permitted. ', + ); + }); + it('should validate use of dangerouslySetInnerHTML', () => { expect(function() { mountComponent({dangerouslySetInnerHTML: 'Hi Jim!'}); @@ -1597,6 +1606,18 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count()).toBe(2); }); + it('should warn about props that are no longer supported without case sensitivity', () => { + spyOn(console, 'error'); + ReactTestUtils.renderIntoDocument(
); + expectDev(console.error.calls.count()).toBe(0); + + ReactTestUtils.renderIntoDocument(
{}} />); + expectDev(console.error.calls.count()).toBe(1); + + ReactTestUtils.renderIntoDocument(
{}} />); + expectDev(console.error.calls.count()).toBe(2); + }); + it('should warn about props that are no longer supported (ssr)', () => { spyOn(console, 'error'); ReactDOMServer.renderToString(
); @@ -1609,6 +1630,18 @@ describe('ReactDOMComponent', () => { expectDev(console.error.calls.count()).toBe(2); }); + it('should warn about props that are no longer supported without case sensitivity (ssr)', () => { + spyOn(console, 'error'); + ReactDOMServer.renderToString(
); + expectDev(console.error.calls.count()).toBe(0); + + ReactDOMServer.renderToString(
{}} />); + expectDev(console.error.calls.count()).toBe(1); + + ReactDOMServer.renderToString(
{}} />); + expectDev(console.error.calls.count()).toBe(2); + }); + it('gives source code refs for unknown prop warning', () => { spyOn(console, 'error'); ReactTestUtils.renderIntoDocument(
); diff --git a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js index b5a916af28ce9..a00ad18a7576c 100644 --- a/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js +++ b/src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js @@ -39,18 +39,12 @@ if (__DEV__) { var EVENT_NAME_REGEX = /^on[A-Z]/; var ARIA_NAME_REGEX = /^aria-/i; var possibleStandardNames = require('possibleStandardNames'); - var unsupportedProps = { - onfocusin: true, - onfocusout: true, - }; var validateProperty = function(tagName, name, value, debugID) { if (warnedProperties.hasOwnProperty(name) && warnedProperties[name]) { return true; } - warnedProperties[name] = true; - if (EventPluginRegistry.registrationNameModules.hasOwnProperty(name)) { return true; } @@ -79,16 +73,33 @@ if (__DEV__) { registrationName, getStackAddendum(debugID), ); + warnedProperties[name] = true; return true; } - // Unsupported props are handled by another validation - if (unsupportedProps.hasOwnProperty(lowerCasedName)) { + // Let the ARIA attribute hook validate ARIA attributes + if (ARIA_NAME_REGEX.test(name)) { return true; } - // Let the ARIA attribute hook validate ARIA attributes - if (ARIA_NAME_REGEX.test(name)) { + if (lowerCasedName === 'onfocusin' || lowerCasedName === 'onfocusout') { + warning( + false, + 'React uses onFocus and onBlur instead of onFocusIn and onFocusOut. ' + + 'All React events are normalized to bubble, so onFocusIn and onFocusOut ' + + 'are not needed/supported by React.', + ); + warnedProperties[name] = true; + return true; + } + + if (lowerCasedName === 'innerhtml') { + warning( + false, + 'Directly setting property `innerHTML` is not permitted. ' + + 'For more information, lookup documentation on `dangerouslySetInnerHTML`.', + ); + warnedProperties[name] = true; return true; } @@ -104,6 +115,7 @@ if (__DEV__) { getStackAddendum(debugID), ); } + warnedProperties[name] = true; return true; } @@ -115,10 +127,16 @@ if (__DEV__) { name, getStackAddendum(debugID), ); + warnedProperties[name] = true; return true; } - return DOMProperty.shouldSetAttribute(name, value); + if (!DOMProperty.shouldSetAttribute(name, value)) { + warnedProperties[name] = true; + return false; + } + + return true; }; } diff --git a/src/renderers/dom/shared/utils/assertValidProps.js b/src/renderers/dom/shared/utils/assertValidProps.js index 27f7b39641f95..5de6bf0032bee 100644 --- a/src/renderers/dom/shared/utils/assertValidProps.js +++ b/src/renderers/dom/shared/utils/assertValidProps.js @@ -63,11 +63,6 @@ function assertValidProps( ); } if (__DEV__) { - warning( - props.innerHTML == null, - 'Directly setting property `innerHTML` is not permitted. ' + - 'For more information, lookup documentation on `dangerouslySetInnerHTML`.', - ); warning( props.suppressContentEditableWarning || !props.contentEditable || @@ -77,12 +72,6 @@ function assertValidProps( 'those nodes are unexpectedly modified or duplicated. This is ' + 'probably not intentional.', ); - warning( - props.onFocusIn == null && props.onFocusOut == null, - 'React uses onFocus and onBlur instead of onFocusIn and onFocusOut. ' + - 'All React events are normalized to bubble, so onFocusIn and onFocusOut ' + - 'are not needed/supported by React.', - ); } invariant( props.style == null || typeof props.style === 'object', diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 2fd7d8fe922c0..bda3a8dab44c9 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -103,11 +103,6 @@ function assertValidProps(component, props) { ); } if (__DEV__) { - warning( - props.innerHTML == null, - 'Directly setting property `innerHTML` is not permitted. ' + - 'For more information, lookup documentation on `dangerouslySetInnerHTML`.', - ); warning( props.suppressContentEditableWarning || !props.contentEditable || @@ -117,12 +112,6 @@ function assertValidProps(component, props) { 'those nodes are unexpectedly modified or duplicated. This is ' + 'probably not intentional.', ); - warning( - props.onFocusIn == null && props.onFocusOut == null, - 'React uses onFocus and onBlur instead of onFocusIn and onFocusOut. ' + - 'All React events are normalized to bubble, so onFocusIn and onFocusOut ' + - 'are not needed/supported by React.', - ); } invariant( props.style == null || typeof props.style === 'object', From 81622229794aa3910f7c614fadace1ccfac0af3a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 8 Aug 2017 09:04:30 -0400 Subject: [PATCH 45/49] Remove attributes that are updated to invalid values --- .../dom/fiber/ReactDOMFiberComponent.js | 24 +++++-------------- .../dom/shared/DOMPropertyOperations.js | 8 +++++-- .../__tests__/ReactDOMComponent-test.js | 15 ++++++++++++ .../dom/stack/client/ReactDOMComponent.js | 2 +- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 7cffee7bf4ef7..f8f6f8f016e30 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -231,17 +231,11 @@ function setInitialDOMProperties( } } else if (isCustomComponentTag) { DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp); - } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { + } else if (nextProp != null) { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. - if (nextProp != null) { - DOMPropertyOperations.setValueForProperty( - domElement, - propKey, - nextProp, - ); - } + DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp); } } } @@ -272,19 +266,13 @@ function updateDOMProperties( } else { DOMPropertyOperations.deleteValueForAttribute(domElement, propKey); } - } else if (DOMProperty.shouldSetAttribute(propKey, propValue)) { + } else if (propValue != null) { + DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue); + } else { // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This // brings us in line with the same behavior we have on initial render. - if (propValue != null) { - DOMPropertyOperations.setValueForProperty( - domElement, - propKey, - propValue, - ); - } else { - DOMPropertyOperations.deleteValueForProperty(domElement, propKey); - } + DOMPropertyOperations.deleteValueForProperty(domElement, propKey); } } } diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 5f93ab1f468fc..a58fe97cbefd0 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -190,8 +190,12 @@ var DOMPropertyOperations = { node.setAttribute(attributeName, '' + value); } } - } else if (DOMProperty.shouldSetAttribute(name, value)) { - DOMPropertyOperations.setValueForAttribute(node, name, value); + } else { + DOMPropertyOperations.setValueForAttribute( + node, + name, + DOMProperty.shouldSetAttribute(name, value) ? value : null, + ); return; } diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index e47f4efee3f90..76b7c589ed447 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -2080,5 +2080,20 @@ describe('ReactDOMComponent', () => { 'expected, cast the value to a string.\n in div', ); }); + + it('removes a property when it becomes invalid', function() { + spyOn(console, 'error'); + + var container = document.createElement('div'); + ReactDOM.render(
, container); + ReactDOM.render(
, container); + var el = container.firstChild; + + expect(el.hasAttribute('whatever')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `whatever` on
tag.', + ); + }); }); }); diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index bda3a8dab44c9..6d1a3f0b274db 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -1010,7 +1010,7 @@ ReactDOMComponent.Mixin = { nextProp, ); } - } else if (DOMProperty.shouldSetAttribute(propKey, nextProp)) { + } else if (!DOMProperty.isReservedProp(propKey)) { var node = getNode(this); // If we're updating to null or undefined, we should remove the property // from the DOM node instead of inadvertently setting to a string. This From e9c8910c04fab7d1347ec8ee01a9a985feeacdae Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 8 Aug 2017 10:22:34 -0400 Subject: [PATCH 46/49] Support object property values with toString methods. Allow boolean props to coerce objects --- src/renderers/dom/shared/DOMProperty.js | 15 +++- .../dom/shared/DOMPropertyOperations.js | 3 +- .../dom/shared/HTMLDOMPropertyConfig.js | 4 - .../__tests__/ReactDOMComponent-test.js | 82 +++++++++++++++++-- .../hooks/ReactDOMUnknownPropertyHook.js | 29 ++++--- 5 files changed, 109 insertions(+), 24 deletions(-) diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index da944b3b78c3d..9c48ed1d64692 100644 --- a/src/renderers/dom/shared/DOMProperty.js +++ b/src/renderers/dom/shared/DOMProperty.js @@ -25,6 +25,7 @@ var RESERVED_PROPS = { suppresscontenteditablewarning: true, onfocusin: true, onfocusout: true, + style: true, }; function checkMask(value, bitmask) { @@ -212,7 +213,7 @@ var DOMProperty = { return false; } - if (value === null || DOMProperty.properties.hasOwnProperty(name)) { + if (value === null) { return true; } @@ -224,14 +225,26 @@ var DOMProperty = { return false; } + var propertyInfo = DOMProperty.properties[name]; + switch (typeof value) { case 'boolean': + if (propertyInfo) { + return true; + } var prefix = lowerCased.slice(0, 5); return prefix === 'data-' || prefix === 'aria-'; case 'undefined': case 'number': case 'string': return true; + case 'object': + // Allow HAS_BOOLEAN_VALUE to coerce to true + if (propertyInfo && propertyInfo.hasBooleanValue) { + return true; + } + + return value.toString !== Object.prototype.toString; default: return false; } diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index a58fe97cbefd0..96dead745ba6d 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -163,7 +163,8 @@ var DOMPropertyOperations = { */ setValueForProperty: function(node, name, value) { var propertyInfo = DOMProperty.getPropertyInfo(name); - if (propertyInfo) { + + if (propertyInfo && DOMProperty.shouldSetAttribute(name, value)) { var mutationMethod = propertyInfo.mutationMethod; if (mutationMethod) { mutationMethod(node, value); diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 56696e2907802..93de4b6d6e7ff 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -65,10 +65,6 @@ var HTMLDOMPropertyConfig = { // itemScope is for for Microdata support. // See http://schema.org/docs/gs.html itemScope: HAS_BOOLEAN_VALUE, - // Facebook internal attribute. This is an object attribute that - // implements a custom toString() method. Objects are not allowed as - // custom attributes unless they are specified by an attribute config - ajaxify: 0, // These attributes must stay in the white-list because they have // different attribute names (see DOMAttributeNames below) acceptCharset: 0, diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 76b7c589ed447..233614db0d672 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -2051,13 +2051,6 @@ describe('ReactDOMComponent', () => { ); }); - it('assigns ajaxify (an important internal FB attribute)', function() { - var options = {toString: () => 'ajaxy'}; - var el = ReactTestUtils.renderIntoDocument(
); - - expect(el.getAttribute('ajaxify')).toBe('ajaxy'); - }); - it('allows cased data attributes', function() { var el = ReactTestUtils.renderIntoDocument(
); expect(el.getAttribute('data-foobar')).toBe('true'); @@ -2096,4 +2089,79 @@ describe('ReactDOMComponent', () => { ); }); }); + + describe('Object stringification', function() { + it('does not allow objects without a toString method for members of the whitelist', function() { + spyOn(console, 'error'); + + var el = ReactTestUtils.renderIntoDocument( +
, + ); + + expect(el.hasAttribute('allowtransparency')).toBe(false); + + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `allowTransparency` on
tag.', + ); + }); + + it('should pass objects as attributes if they define toString', () => { + var obj = { + toString() { + return 'hello'; + }, + }; + var container = document.createElement('div'); + + ReactDOM.render(, container); + expect(container.firstChild.src).toBe('hello'); + + ReactDOM.render(, container); + expect(container.firstChild.getAttribute('arabic-form')).toBe('hello'); + + ReactDOM.render(
, container); + expect(container.firstChild.getAttribute('customAttribute')).toBe( + 'hello', + ); + }); + + it('should not pass objects on known SVG attributes if they do not define toString', () => { + spyOn(console, 'error'); + var obj = {}; + var container = document.createElement('div'); + + ReactDOM.render(, container); + expect(container.firstChild.getAttribute('arabic-form')).toBe(null); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `arabicForm` on tag.', + ); + }); + + it('should not pass objects on custom attributes if they do not define toString', () => { + spyOn(console, 'error'); + var obj = {}; + var container = document.createElement('div'); + + ReactDOM.render(
, container); + expect(container.firstChild.getAttribute('customAttribute')).toBe(null); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Invalid prop `customAttribute` on
tag.', + ); + }); + + it('allows objects that inherit a custom toString method', function() { + var parent = {toString: () => 'hello.jpg'}; + var child = Object.create(parent); + var img = ReactTestUtils.renderIntoDocument(); + + expect(img.src).toBe('hello.jpg'); + }); + + it('allows objects when a property uses the HAS_BOOLEAN_VALUE flag', function() { + var el = ReactTestUtils.renderIntoDocument(