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

Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. #10385

Merged
merged 49 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
af36bfa
Allow custom attributes. Add flag to toggle custom attribute behavior
nhunzaker Apr 8, 2016
601eabd
Update custom attribute logic
nhunzaker Aug 3, 2017
eab17b5
Allow numbers and booleans custom attributes. Cut isCustomAttribute
nhunzaker Aug 3, 2017
137af3b
Cover objects with custom attributes in warning test
nhunzaker Aug 3, 2017
55e55ba
Rename DOMProperty.isWriteable to shouldSetAttribute
nhunzaker Aug 3, 2017
2106844
Rework conditions in shouldSetProperty to avoid edge cases
nhunzaker Aug 3, 2017
9caa863
Update unknown property warning to include custom attribute information
nhunzaker Aug 3, 2017
84beb33
Remove ref and key from reserved props
nhunzaker Aug 3, 2017
f406aac
Ensure SSR test coverage for DOMProperty injections
nhunzaker Aug 4, 2017
da19306
Add ajaxify attribute for internal FB support
nhunzaker Aug 4, 2017
f09d3f3
Ajaxify is a stringifiable object attribute
nhunzaker Aug 4, 2017
aeb2db3
Update test name for custom attributes on custom elements
nhunzaker Aug 4, 2017
7cbf2f3
Remove SSR custom injection test
nhunzaker Aug 4, 2017
b67dd13
Remove onAfterResetModules hooks in SSR render tests
nhunzaker Aug 4, 2017
dab72d2
Do not allow assignment of attributes that are aliased
nhunzaker Aug 4, 2017
000c0df
Update custom attribute test to check value, not just presence
nhunzaker Aug 4, 2017
a77d47b
Address case where class is assigned as an attribute on custom elemen…
nhunzaker Aug 4, 2017
f661d22
Cover cases where className and for are given to custom elements
nhunzaker Aug 4, 2017
aa3916b
Remove unnecessary spys on console.error. Reduce extra space in tests
nhunzaker Aug 5, 2017
6ce3335
Cover cased custom attributes in SSR tests
nhunzaker Aug 5, 2017
a11b0bd
Custom attributes are case sensitive
nhunzaker Aug 5, 2017
07c6865
Allow uppercase letters in custom attributes. Address associated edge…
nhunzaker Aug 7, 2017
599844e
Make ARIA enforcement dev-only
nhunzaker Aug 7, 2017
14d5ea0
Remove non-case sensitive standard attributes. Make ARIA hook dev only.
nhunzaker Aug 4, 2017
99206db
Remove case sensitive props
nhunzaker Aug 4, 2017
a3c2aef
Add back a few attributes and explain why they are needed
nhunzaker Aug 4, 2017
ca601c6
Remove possibleStandardNames from DOMProperty.js
nhunzaker Aug 4, 2017
3e866cf
Fix typo in HTMLPropertyConfig comment
nhunzaker Aug 4, 2017
b7d6996
Remove duplicative comment
nhunzaker Aug 4, 2017
545bcab
Add back loop boolean property
nhunzaker Aug 4, 2017
afb609e
Allow improperly cased aliased attributes. Add additional tests
nhunzaker Aug 7, 2017
71871c4
Handle special properties like onFocusOut
nhunzaker Aug 7, 2017
3281320
Add some comments to document where casing matters. Remove DOMPropert…
nhunzaker Aug 7, 2017
19bfbaf
Fix spelling mistake in ajaxify HTML property comment
nhunzaker Aug 7, 2017
6df8b4f
Remove alias test that covers multiple aliases for one property
nhunzaker Aug 7, 2017
cb57075
Fix typo in comment
nhunzaker Aug 7, 2017
21678fc
Build SVG aliases dynamically
nhunzaker Aug 7, 2017
90b4cce
Remove unused DOMPropertyNames reference
nhunzaker Aug 7, 2017
fa3db62
Do not translate bad casings of aliased attributes
nhunzaker Aug 7, 2017
21db719
Revise the way custom booleans are treated
nhunzaker Aug 8, 2017
c2c3d63
Add developer warnings for NaN and ARIA hooks
nhunzaker Aug 8, 2017
7f16b4d
Revert changes to the docs
gaearon Aug 8, 2017
3908cd3
Use string comparison instead of regex to check for data and aria att…
nhunzaker Aug 8, 2017
2479fcc
Warn about unsupported properties without case sensitivity
nhunzaker Aug 8, 2017
8162222
Remove attributes that are updated to invalid values
nhunzaker Aug 8, 2017
e9c8910
Support object property values with toString methods. Allow boolean p…
nhunzaker Aug 8, 2017
6b1572a
Add back ajaxify test
nhunzaker Aug 8, 2017
cf1b0d7
Address bad references in ReactDOMComponent-test. Format.
nhunzaker Aug 8, 2017
b923740
Allow all objects and pass incorrect aliases
sophiebits Aug 15, 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
68 changes: 29 additions & 39 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,20 +231,11 @@ function setInitialDOMProperties(
}
} else if (isCustomComponentTag) {
DOMPropertyOperations.setValueForAttribute(domElement, propKey, nextProp);
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)
) {
} else if (nextProp != null) {
// If we're updating to null or undefined, we should remove the property
// from the DOM node instead of inadvertently setting to a string. This
// brings us in line with the same behavior we have on initial render.
if (nextProp != null) {
DOMPropertyOperations.setValueForProperty(
domElement,
propKey,
nextProp,
);
}
DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp);
}
}
}
Expand Down Expand Up @@ -275,22 +266,13 @@ function updateDOMProperties(
} else {
DOMPropertyOperations.deleteValueForAttribute(domElement, propKey);
}
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)
) {
} else if (propValue != null) {
DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue);
} else {
// If we're updating to null or undefined, we should remove the property
// from the DOM node instead of inadvertently setting to a string. This
// brings us in line with the same behavior we have on initial render.
if (propValue != null) {
DOMPropertyOperations.setValueForProperty(
domElement,
propKey,
propValue,
);
} else {
DOMPropertyOperations.deleteValueForProperty(domElement, propKey);
}
DOMPropertyOperations.deleteValueForProperty(domElement, propKey);
}
}
}
Expand Down Expand Up @@ -928,8 +910,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 SSR attribute is whitelisted
case 'data-reactroot':
Expand Down Expand Up @@ -1013,28 +994,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());
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.getPropertyInfo(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());
serverValue = DOMPropertyOperations.getValueForAttribute(
domElement,
propKey,
nextProp,
);
}

if (nextProp !== serverValue) {
warnForPropDifference(propKey, serverValue, nextProp);
}
Expand Down
74 changes: 0 additions & 74 deletions src/renderers/dom/shared/ARIADOMPropertyConfig.js

This file was deleted.

6 changes: 2 additions & 4 deletions src/renderers/dom/shared/DOMMarkupOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,7 @@ var DOMMarkupOperations = {
* @return {?string} Markup string, or null if the property was invalid.
*/
createMarkupForProperty: function(name, value) {
var propertyInfo = DOMProperty.properties.hasOwnProperty(name)
? DOMProperty.properties[name]
: null;
var propertyInfo = DOMProperty.getPropertyInfo(name);
if (propertyInfo) {
if (shouldIgnoreValue(propertyInfo, value)) {
return '';
Expand All @@ -103,7 +101,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
129 changes: 86 additions & 43 deletions src/renderers/dom/shared/DOMProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator

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:

Warning: Invalid DOM property innerhtml. Did you mean innerHTML?

But then if I try innerHTML, of course it isn’t supported:

Warning: Directly setting property innerHTML is not permitted. For more information, lookup documentation on dangerouslySetInnerHTML.

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).

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'm working on making the warning for onFocusIn and onFocusOut case 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.

suppresscontenteditablewarning: true,
onfocusin: true,
onfocusout: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I these from possibleStandardNames to prevent the UnknownDOMPropertyHook from picking it up in addition to the dev warning about onfocusin.

I think we just need to make the onFocusIn warning case insensitive. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works for me.

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'll get this.

style: true,
};

function checkMask(value, bitmask) {
return (value & bitmask) === bitmask;
}
Expand All @@ -32,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.
Expand All @@ -61,15 +71,8 @@ var DOMPropertyInjection = {
var Properties = domPropertyConfig.Properties || {};
var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {};
var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {};
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 @@ -111,30 +114,28 @@ var DOMPropertyInjection = {
propName,
);

if (__DEV__) {
DOMProperty.getPossibleStandardName[lowerCased] = propName;
}

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

propertyInfo.attributeName = attributeName;
if (__DEV__) {
DOMProperty.getPossibleStandardName[attributeName] = propName;
}

// Use the lowercase form of the attribute name to prevent
// badly cased React attribute alises from writing to the DOM.
DOMProperty.aliases[attributeName.toLowerCase()] = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is value always a boolean? In this case shall we name it hasDifferentCanonicalName or something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is always a boolean. That name change is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a good name for the "React way" attribute names.... We call them "special React properties", "javascript form", etc....

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like to say canonical form.

}

if (DOMAttributeNamespaces.hasOwnProperty(propName)) {
propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName];
}

if (DOMPropertyNames.hasOwnProperty(propName)) {
propertyInfo.propertyName = DOMPropertyNames[propName];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DOMPropertyNames is not used by any injection.


if (DOMMutationMethods.hasOwnProperty(propName)) {
propertyInfo.mutationMethod = DOMMutationMethods[propName];
}

// Downcase references to whitelist properties to check for membership
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do these become lowercase? This comment confused me a bit because I expected to find toLowerCase() call somewhere close but it’s not there.

// 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;
}
},
Expand Down Expand Up @@ -197,33 +198,75 @@ var DOMProperty = {
properties: {},

/**
* Mapping from lowercase property names to the properly cased version, used
* to warn in the case of missing properties. Available only in __DEV__.
*
* autofocus is predefined, because adding it to the property whitelist
* causes unintended side effects.
*
* @type {Object}
* Some attributes are aliased for easier use within React. We don't
* allow direct use of these attributes. See DOMAttributeNames in
* HTMLPropertyConfig and SVGPropertyConfig.
*/
getPossibleStandardName: __DEV__ ? {autofocus: 'autoFocus'} : null,
aliases: {},

/**
* 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)) {
return false;
}

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

var lowerCased = name.toLowerCase();

// Prevent aliases, and badly cased aliases like `class` or `cLASS`
// from showing up in the DOM
if (DOMProperty.aliases.hasOwnProperty(lowerCased)) {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we allow these but just warn? What's the rationale for hard-blocking them?

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 think it was to heavily steer users towards the alias. Dan wanted to avoid confusion where you pass class, or stroke-dasharray, it's technical valid and the HTML/SVG writes the way you expect, but it's not "The React Way", so you see a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I think we should allow both. For 16 we would warn, but we could explore lifting the warnings in 17/18, only warning when the alias and the standard form are included in the same prop object.

For that to work, we might want to figure out which key gets priority: the alias or the native attribute name?

}

var propertyInfo = DOMProperty.properties[name];

switch (typeof value) {
case 'boolean':
if (propertyInfo) {
return true;
Copy link
Contributor Author

@nhunzaker nhunzaker Aug 15, 2017

Choose a reason for hiding this comment

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

@sebmarkbage I'd like to figure out how much HAS_BOOLEAN_VALUE, HAS_NUMERIC_VALUE, and HAS_POSITIVE_NUMERIC_VALUE are developer convenience vs ensuring correctness. I still don't understand why all of the attribute names we have to include for booleans aren't flagged with HAS_BOOLEAN_VALUE.

Likewise for numeric fields (which we've dropped from the whitelist, except for things like cols, rows, start, etc.

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':
// Allow HAS_BOOLEAN_VALUE to coerce to true
if (propertyInfo && propertyInfo.hasBooleanValue) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we coerce objects just for boolean values?


return value.toString !== Object.prototype.toString;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we dedupe this equality check with a constant towards the top of the 'object' case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, we should generally do a pass and inline extra the property access in this and similar functions. Let's do it after we decide the fate of this PR.

default:
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We came to the conclusion that the heuristic is worse than always tostringing. So we can remove this whole thing. It'll always return true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking care of this, @spicyj!

}
return false;
},
Copy link
Contributor Author

@nhunzaker nhunzaker Aug 9, 2017

Choose a reason for hiding this comment

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

@sebmarkbage Follow up from #10416 regarding valueOf.

Is it safe to evaluate stringification through valueOf or toString, and fallback to null if we don't like the value?

Like:

var stringified = '' + value
return stringified !== '[object Object]' ? stringified : null

I imagine just checking for [object Object] isn't the way to go, but something similar.

We could change this function to be more of a "getAttributeValue" function, which would return null in the case where an attribute should be assigned. That way if a valueOf or toString, heaven forbid, have side-effects, we only call it once.


getPropertyInfo(name) {
return DOMProperty.properties.hasOwnProperty(name)
? DOMProperty.properties[name]
: null;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a final word here. In this scenario, all attributes are referenced as their lowercase form internally. I've hidden that within getPropertyInfo to avoid needing to know about lowercasing attributes outside of DOMProperty.js.


/**
* 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,
Expand Down
Loading