Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom attributes by default #7311

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
afae07c
Allow custom attributes. Add flag to toggle custom attribute behavior
nhunzaker Apr 8, 2016
3388f4f
Update custom attribute logic
nhunzaker Aug 3, 2017
9f7a696
Allow numbers and booleans custom attributes. Cut isCustomAttribute
nhunzaker Aug 3, 2017
fd94756
Cover objects with custom attributes in warning test
nhunzaker Aug 3, 2017
f870980
Rename DOMProperty.isWriteable to shouldSetAttribute
nhunzaker Aug 3, 2017
ab64ef3
Rework conditions in shouldSetProperty to avoid edge cases
nhunzaker Aug 3, 2017
7717ed7
Update unknown property warning to include custom attribute information
nhunzaker Aug 3, 2017
2b402a4
Remove ref and key from reserved props
nhunzaker Aug 3, 2017
1681e7f
Ensure SSR test coverage for DOMProperty injections
nhunzaker Aug 4, 2017
b5736c7
Add ajaxify attribute for internal FB support
nhunzaker Aug 4, 2017
976ca82
Ajaxify is a stringifiable object attribute
nhunzaker Aug 4, 2017
17d3c49
Update test name for custom attributes on custom elements
nhunzaker Aug 4, 2017
40dee5a
Remove SSR custom injection test
nhunzaker Aug 4, 2017
b26384d
Remove onAfterResetModules hooks in SSR render tests
nhunzaker Aug 4, 2017
d672771
Do not allow assignment of attributes that are aliased
nhunzaker Aug 4, 2017
a871d55
Update custom attribute test to check value, not just presence
nhunzaker Aug 4, 2017
c748e84
Address case where class is assigned as an attribute on custom elemen…
nhunzaker Aug 4, 2017
16dd18a
Cover cases where className and for are given to custom elements
nhunzaker Aug 4, 2017
e68bf4d
Remove unnecessary spys on console.error. Reduce extra space in tests
nhunzaker Aug 5, 2017
83e46aa
Cover cased custom attributes in SSR tests
nhunzaker Aug 5, 2017
facfa87
Custom attributes are case sensitive
nhunzaker Aug 5, 2017
82e05e0
Allow uppercase letters in custom attributes. Address associated edge…
nhunzaker Aug 7, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/warnings/unknown-prop.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
48 changes: 25 additions & 23 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.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.
Expand Down Expand Up @@ -275,10 +272,7 @@ function updateDOMProperties(
} else {
DOMPropertyOperations.deleteValueForAttribute(domElement, propKey);
}
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)
) {
} 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.
Expand Down Expand Up @@ -928,8 +922,7 @@ var ReactDOMFiberComponent = {
var extraAttributeNames: Set<string> = 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
Expand Down Expand Up @@ -1019,28 +1012,37 @@ var ReactDOMFiberComponent = {
if (expectedStyle !== serverValue) {
warnForPropDifference(propKey, serverValue, expectedStyle);
}
} else if (
isCustomComponentTag ||
DOMProperty.isCustomAttribute(propKey)
) {
} else if (isCustomComponentTag) {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey);
extraAttributeNames.delete(propKey.toLowerCase());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not obvious to me why this is necessary. Do we have a test verifying this change? Can you add a comment as to why we do this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And is this only necessary for custom tags? What about custom attributes? If I do <div myAttribute="" /> or <div data-MyAttribute="">, does this also mess up server validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, it looks there's a related todo a earlier in the document:
https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L931

Accessing the attribute name from the DOM is always going to return a lower cased name, so we need to do this for regular DOM elements too. For consistency, and until we close the thread on this, I've added an adjustment and test in 83e46aa.

I think we really just need to decide if we care about allowing custom attributes to have custom casing. If not, we should add that logic to shouldSetAttribute and probably make a separate warning along the lines of "Custom attributes must be lowercase".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor if just warning when custom attributes use casing.

serverValue = DOMPropertyOperations.getValueForAttribute(
domElement,
propKey,
nextProp,
);

if (nextProp !== serverValue) {
warnForPropDifference(propKey, serverValue, nextProp);
}
} else if ((propertyInfo = DOMProperty.properties[propKey])) {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propertyInfo.attributeName);
serverValue = DOMPropertyOperations.getValueForProperty(
domElement,
propKey,
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(
domElement,
propKey,
nextProp,
);
} else {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey.toLowerCase());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1031 properly removes attributes from the extraAttributeNames list because React assigns a lower cased version of the property name as attributeName. That doesn't happen for properties outside of this configuration, so we need to downcase them here (just like on custom attributes).

serverValue = DOMPropertyOperations.getValueForAttribute(
domElement,
propKey,
nextProp,
);
}

if (nextProp !== serverValue) {
warnForPropDifference(propKey, serverValue, nextProp);
}
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/dom/shared/DOMMarkupOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ var DOMMarkupOperations = {
return attributeName + '=""';
}
return attributeName + '=' + quoteAttributeValueForBrowser(value);
} else if (DOMProperty.isCustomAttribute(name)) {
} else if (DOMProperty.shouldSetAttribute(name, value)) {
if (value == null) {
return '';
}
Expand Down
86 changes: 63 additions & 23 deletions src/renderers/dom/shared/DOMProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@

var invariant = require('fbjs/lib/invariant');

var RESERVED_PROPS = {
children: true,
dangerouslySetInnerHTML: true,
autoFocus: true,
defaultValue: true,
defaultChecked: true,
innerHTML: true,
suppressContentEditableWarning: true,
onFocusIn: true,
onFocusOut: true,
};

function checkMask(value, bitmask) {
return (value & bitmask) === bitmask;
}
Expand All @@ -32,11 +44,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.
Expand Down Expand Up @@ -64,12 +71,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),
Expand Down Expand Up @@ -117,6 +118,13 @@ var DOMPropertyInjection = {

if (DOMAttributeNames.hasOwnProperty(propName)) {
var attributeName = DOMAttributeNames[propName];

DOMProperty.aliases[attributeName.toLowerCase()] = true;

if (lowerCased !== attributeName) {
DOMProperty.aliases[lowerCased] = true;
}

propertyInfo.attributeName = attributeName;
if (__DEV__) {
DOMProperty.getPossibleStandardName[attributeName] = propName;
Expand Down Expand Up @@ -196,6 +204,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__.
Expand All @@ -208,22 +223,47 @@ 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.
* Checks whether a property name is a writeable attribute.
* @method
*/
isCustomAttribute: function(attributeName) {
for (var i = 0; i < DOMProperty._isCustomAttributeFunctions.length; i++) {
var isCustomAttributeFn = DOMProperty._isCustomAttributeFunctions[i];
if (isCustomAttributeFn(attributeName)) {
shouldSetAttribute: function(name, value) {
if (
DOMProperty.isReservedProp(name) ||
DOMProperty.aliases.hasOwnProperty(name)
) {
return false;
}

if (DOMProperty.properties.hasOwnProperty(name)) {
return true;
}

if (value === null) {
return true;
}

switch (typeof value) {
case 'undefined':
case 'boolean':
case 'number':
case 'string':
return true;
}
default:
return false;
}
return false;
},

/**
* 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,
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ var DOMPropertyOperations = {
node.setAttribute(attributeName, '' + value);
}
}
} else if (DOMProperty.isCustomAttribute(name)) {
} else if (DOMProperty.shouldSetAttribute(name, value)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
return;
}
Expand Down Expand Up @@ -272,7 +272,7 @@ var DOMPropertyOperations = {
} else {
node.removeAttribute(propertyInfo.attributeName);
}
} else if (DOMProperty.isCustomAttribute(name)) {
} else {
node.removeAttribute(name);
}

Expand Down
6 changes: 3 additions & 3 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -205,6 +202,9 @@ var HTMLDOMPropertyConfig = {
security: 0,
// IE-only attribute that controls focus behavior
unselectable: 0,
// Facebook internal attribute. This is an object attribute that
// impliments a custom toString() method
ajaxify: 0,
},
DOMAttributeNames: {
acceptCharset: 'accept-charset',
Expand Down
Loading