Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. #10385
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,21 @@ | |
|
|
||
| var invariant = require('fbjs/lib/invariant'); | ||
|
|
||
| // These attributes should be all lowercase to allow for | ||
| // case insensitive checks | ||
| var RESERVED_PROPS = { | ||
| children: true, | ||
| dangerouslysetinnerhtml: true, | ||
| autofocus: true, | ||
| defaultvalue: true, | ||
| defaultchecked: true, | ||
| innerhtml: true, | ||
| suppresscontenteditablewarning: true, | ||
| onfocusin: true, | ||
| onfocusout: true, | ||
|
There was a problem hiding this comment. These two are currently not validated at all. If I type <div onfocusin={function() {}} />it will silently ignore it. I would expect it to warn. There was a problem hiding this comment. Good catch. I these from I think we just need to make the |
||
| style: true, | ||
| }; | ||
|
|
||
| function checkMask(value, bitmask) { | ||
| return (value & bitmask) === bitmask; | ||
| } | ||
|
|
@@ -32,11 +47,6 @@ var DOMPropertyInjection = { | |
| * Inject some specialized knowledge about the DOM. This takes a config object | ||
| * with the following properties: | ||
| * | ||
| * 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. | ||
|
|
@@ -61,15 +71,8 @@ var DOMPropertyInjection = { | |
| var Properties = domPropertyConfig.Properties || {}; | ||
| var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {}; | ||
| var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {}; | ||
| var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {}; | ||
|
|
||
| for (var propName in Properties) { | ||
| invariant( | ||
| !DOMProperty.properties.hasOwnProperty(propName), | ||
|
|
@@ -111,30 +114,24 @@ var DOMPropertyInjection = { | |
| propName, | ||
| ); | ||
|
|
||
| if (DOMAttributeNames.hasOwnProperty(propName)) { | ||
| var attributeName = DOMAttributeNames[propName]; | ||
|
|
||
| propertyInfo.attributeName = attributeName; | ||
| } | ||
|
|
||
| if (DOMAttributeNamespaces.hasOwnProperty(propName)) { | ||
| propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName]; | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. DOMPropertyNames is not used by any injection. |
||
| if (DOMMutationMethods.hasOwnProperty(propName)) { | ||
| propertyInfo.mutationMethod = DOMMutationMethods[propName]; | ||
| } | ||
|
|
||
| // Downcase references to whitelist properties to check for membership | ||
|
There was a problem hiding this comment. Where do these become lowercase? This comment confused me a bit because I expected to find |
||
| // without case-sensitivity. This allows the whitelist to pick up | ||
| // `allowfullscreen`, which should be written using the property configuration | ||
| // for `allowFullscreen` | ||
| DOMProperty.properties[propName] = propertyInfo; | ||
| } | ||
| }, | ||
|
|
@@ -197,33 +194,58 @@ var DOMProperty = { | |
| properties: {}, | ||
|
|
||
| /** | ||
| * Checks whether a property name is a writeable attribute. | ||
| * @method | ||
| */ | ||
| shouldSetAttribute: function(name, value) { | ||
| if (DOMProperty.isReservedProp(name)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (value === null) { | ||
| return true; | ||
| } | ||
|
|
||
| var lowerCased = name.toLowerCase(); | ||
|
|
||
| var propertyInfo = DOMProperty.properties[name]; | ||
|
|
||
| switch (typeof value) { | ||
| case 'boolean': | ||
| if (propertyInfo) { | ||
| return true; | ||
|
There was a problem hiding this comment. @sebmarkbage I'd like to figure out how much Likewise for numeric fields (which we've dropped from the whitelist, except for things like I think this might have mattered back when they were assigned as properties. I'd have to do some archeology. Dan and I made the call to keep their property info the same as 15.x, but I still can't help but wonder. |
||
| } | ||
| var prefix = lowerCased.slice(0, 5); | ||
| return prefix === 'data-' || prefix === 'aria-'; | ||
| case 'undefined': | ||
| case 'number': | ||
| case 'string': | ||
| return true; | ||
| case 'object': | ||
| return true; | ||
| default: | ||
| // function, symbol | ||
| return false; | ||
| } | ||
| }, | ||
|
|
||
| getPropertyInfo(name) { | ||
| return DOMProperty.properties.hasOwnProperty(name) | ||
| ? DOMProperty.properties[name] | ||
| : null; | ||
| }, | ||
|
|
||
| /** | ||
| * 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.toLowerCase()); | ||
| }, | ||
|
|
||
| injection: DOMPropertyInjection, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly confusing because if I use it, it says:
But then if I try
innerHTML, of course it isn’t supported:It seems better if the same warning was displayed immediately (or if we just got a generic "unknown property" warning for wrong casing like
innerhtml).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on making the warning for
onFocusInandonFocusOutcase insensitive. I can do that for innerHTML too.These show up in
assertValidProps:https://github.com/facebook/react/blob/master/src/renderers/dom/shared/utils/assertValidProps.js
The easiest thing to do is move them to
ReactDOMUnknownPropertyHook.