From afae07ccad7cab265741321667756ceb008996e5 Mon Sep 17 00:00:00 2001 From: Nate Hunzaker Date: Fri, 8 Apr 2016 11:07:50 -0400 Subject: [PATCH 01/23] 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 346e4091d20..fb13b57d2f4 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. @@ -1019,28 +1013,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 11b9d9be11a..ee34594ffec 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 793df14dd77..e831c21de31 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 46846b3d3a8..c49fe716fa1 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 bb24b4a12fd..07f3d71bf28 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 f07cfcc9d73..bd5b5622968 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 2f7c137cbb1..85ddfefdfdb 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 e68bf66d913..72087084386 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 5453df69b53..93fce96de1e 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 3388f4fec2f13d88b5c9c5aa2d53dbea3971393a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:05:26 -0400 Subject: [PATCH 02/23] 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 fb13b57d2f4..e17324c875b 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. @@ -1013,7 +1013,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 ee34594ffec..280093d2000 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 e831c21de31..37078b5f815 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 c49fe716fa1..4e1fb713228 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 07f3d71bf28..bb24b4a12fd 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 bd5b5622968..94d550c1e23 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 85ddfefdfdb..8b50986a328 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 72087084386..02e599f4435 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 93fce96de1e..5561fe7b185 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 9f7a6968d392bea4ffc02f7989ed87b061e99b24 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:23:48 -0400 Subject: [PATCH 03/23] 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 37078b5f815..4eb9d921e81 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 6c0f387873d..5d58b433255 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 94d550c1e23..ce9c7a4373c 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 fd94756ae0633da0313a04d0a8e16fdc8673a8b0 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 08:52:00 -0400 Subject: [PATCH 04/23] 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 ce9c7a4373c..688934980e5 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 f87098005ffcb30f2f0e240a82a7b4717530460b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:16:45 -0400 Subject: [PATCH 05/23] 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 e17324c875b..2d8192b63d6 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. @@ -1013,7 +1013,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 280093d2000..2e0a8ca7625 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 4eb9d921e81..f0bcd4ec20b 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 4e1fb713228..f09255c871b 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 02e599f4435..aa579f7338c 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 5561fe7b185..2fd7d8fe922 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 ab64ef3b63147507806d5b712daf9d474ee33bca Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:27:28 -0400 Subject: [PATCH 06/23] 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 f0bcd4ec20b..8638f1d652c 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 7717ed7c11516df3ac9ed955d344010a35d6cb64 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:37:06 -0400 Subject: [PATCH 07/23] 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 eb7585f650d..86883cafcdc 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 688934980e5..806b4f084d7 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 aa579f7338c..c2030ec73ed 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 2b402a40e612e38f4dfb6ecf554e0cae30c545df Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 3 Aug 2017 16:57:22 -0400 Subject: [PATCH 08/23] 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 8638f1d652c..947f2ca26a9 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 1681e7f467b8dab612cbc7037ff0692f294fe546 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 06:40:10 -0400 Subject: [PATCH 09/23] 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 8b50986a328..6aab717a4a3 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 b5736c7ad649843883c01e4018f3d998ebe26983 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 07:04:11 -0400 Subject: [PATCH 10/23] 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 5d58b433255..883c07540a8 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 806b4f084d7..7d597152458 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 976ca82d7a115633bc00b589cc4948d61983f269 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 10:03:46 -0400 Subject: [PATCH 11/23] 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 883c07540a8..e8525884e7d 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 7d597152458..7d2ac8dcd4e 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 17d3c49b20c991b7d22561b69eaab619cb60c3e9 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 10:56:24 -0400 Subject: [PATCH 12/23] 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 6aab717a4a3..1df61bbbb7d 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 40dee5a588b873f3e1320ecbecd28f9a6df1979e Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 11:43:56 -0400 Subject: [PATCH 13/23] 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 1df61bbbb7d..9bab6b179df 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 b26384dac5dd9d5f203cdacf30e207c09aacd5ab Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 13:04:57 -0400 Subject: [PATCH 14/23] 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 9bab6b179df..10c9db313b9 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 d672771b21651eb880a978b852863e61e0579c9f Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 16:51:41 -0400 Subject: [PATCH 15/23] 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 947f2ca26a9..3f268800572 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 7d2ac8dcd4e..af2cacc47ad 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 a871d55b436f80558597c6ca2c4c689f0e64e036 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 16:56:06 -0400 Subject: [PATCH 16/23] 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 af2cacc47ad..e8211a8b43a 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 c748e84732a832f20dbdd1afaf710f723f5c5b8b Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 18:01:22 -0400 Subject: [PATCH 17/23] 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 2d8192b63d6..7b8361a7838 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1013,10 +1013,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 e8211a8b43a..574c4a0b576 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 10c9db313b9..dcf416c6c38 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 16dd18a9cfaec652e7aa18c5dc6e9d3056e639dd Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 4 Aug 2017 18:36:53 -0400 Subject: [PATCH 18/23] 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 7b8361a7838..73318d6cd7c 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1015,7 +1015,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 dcf416c6c38..b4d2bad6a12 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 e68bf4d4f62ec5b70e63c43d723ed406d033b017 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 09:29:41 -0400 Subject: [PATCH 19/23] 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 574c4a0b576..fdf408341b0 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 83e46aacfa4c675f7b887838725f1c2f88eb647f Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 10:20:49 -0400 Subject: [PATCH 20/23] 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 73318d6cd7c..9b46875c92c 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -1036,7 +1036,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 b4d2bad6a12..4a26bfc84c4 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 facfa871a20e808ca983bcbac744abdcb2c24e74 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 5 Aug 2017 14:31:20 -0400 Subject: [PATCH 21/23] 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 3f268800572..a3641c932df 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 fdf408341b0..74a9f1eef50 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 4a26bfc84c4..8080371bb36 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 c2030ec73ed..f71af11a2e8 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 82e05e03f065cc206c7f1f24ccdc273b3a8d129a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sun, 6 Aug 2017 21:20:44 -0400 Subject: [PATCH 22/23] 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 9b46875c92c..5df978dde49 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 attributes are whitelisted // TODO: Once these are gone from the server renderer, we don't need diff --git a/src/renderers/dom/shared/DOMProperty.js b/src/renderers/dom/shared/DOMProperty.js index a3641c932df..1edc7261d1f 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 74a9f1eef50..9cdb4051099 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 8080371bb36..be43ddaf30a 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 f71af11a2e8..11da5af3c31 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 f355e75a407303392a2af9d89da11a36bbd96b96 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 7 Aug 2017 07:28:19 -0400 Subject: [PATCH 23/23] 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 108278940cd..00000000000 --- 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 e507423f6ab..a14c7cc2d62 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 7385c7a1763..1a65728f97d 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 00000000000..9a75a6201b8 --- /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;