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

Changes to attribute whitelist logic #10564

Merged
merged 36 commits into from
Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e10694a
Remove HTMLPropertyConfig entries for non-boolean values
nhunzaker Aug 24, 2017
711bafe
Only HAS_BOOLEAN_VALUE attribute flag can assign booleans
nhunzaker Aug 29, 2017
4c5dfbb
Use a non-boolean attribute in object assignment tests
nhunzaker Aug 29, 2017
e086ff1
Add HAS_STRING_BOOLEAN_VALUE attribute flag
nhunzaker Aug 29, 2017
0410651
Fix boolean tests, add boolean warning.
nhunzaker Aug 29, 2017
ab75d9a
Reserved props should allow booleans
nhunzaker Aug 29, 2017
6077e0b
Remove outdated comments
gaearon Aug 29, 2017
6f913ed
Style tweaks
gaearon Aug 29, 2017
aca8d9c
Don't treat dashed SVG tags as custom elements
gaearon Aug 29, 2017
af7d035
SVG elements like font-face are not custom attributes
nhunzaker Aug 29, 2017
9c0751f
Move namespace check to isCustomAttribute. Add caveat for stack.
nhunzaker Aug 29, 2017
f8da44e
Remove unused namespace variable assignment
nhunzaker Aug 29, 2017
b93e093
Fix the DEV-only whitelist
gaearon Aug 29, 2017
fbcced1
Don't read property twice
gaearon Aug 29, 2017
a270e03
Ignore and warn about non-string `is` attribute
gaearon Aug 29, 2017
ed92af0
Blacklist "aria" and "data" attributes
gaearon Aug 29, 2017
5a339a1
Don't pass unknown on* attributes through
gaearon Aug 29, 2017
0b2ba65
Remove dead code
gaearon Aug 29, 2017
3f85316
Avoid accessing namespace when possible
nhunzaker Aug 29, 2017
76a6318
Drop .only in ReactDOMComponent-test
nhunzaker Aug 29, 2017
b29bb74
Make isCustomComponent logic more solid
gaearon Aug 29, 2017
0a2aec4
Do attribute name check earlier
gaearon Aug 29, 2017
501e86d
Fix fbjs import
gaearon Aug 29, 2017
8d1f487
Revert unintentional edit
gaearon Aug 29, 2017
72666fa
Re-allow "data" attribute
gaearon Aug 29, 2017
cb687ed
Use stricter check when attaching events
gaearon Aug 29, 2017
1d61379
Pass SVG boolean attributes with correct casing
gaearon Aug 29, 2017
2b0f61a
Fix the test
gaearon Aug 29, 2017
1590c2a
Undo the SVG dashed-name fix
gaearon Aug 30, 2017
cb883a6
Prettier
gaearon Aug 30, 2017
32f6321
Fix lint
gaearon Aug 30, 2017
6d9c0e0
Fix flow
gaearon Aug 30, 2017
10e0c09
Pass "aria" through but still warn
gaearon Aug 30, 2017
ba71ec1
Remove special cases for onfocusin, onfocusout
gaearon Aug 30, 2017
dc760af
Add a more specific warning for unknown events
gaearon Aug 31, 2017
10950c4
Pass badly cased React attributes through with warning
gaearon Aug 31, 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
21 changes: 6 additions & 15 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ var HTMLDOMPropertyConfig = {
// name warnings.
Properties: {
allowFullScreen: HAS_BOOLEAN_VALUE,
// IE only true/false iFrame attribute
// https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx
allowTransparency: HAS_BOOLEAN_VALUE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is adding HAS_BOOLEAN_VALUE always safe? It changes semantics (falsy get removed). Are any of these actually true by default?

Should we keep using this flag or should we introduce a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, darn it. Yes. We need a new flag.

// specifies target context for links with `preload` type
async: HAS_BOOLEAN_VALUE,
// autoFocus is polyfilled/normalized by AutoFocusUtils
Expand All @@ -35,11 +38,13 @@ var HTMLDOMPropertyConfig = {
capture: HAS_BOOLEAN_VALUE,
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
cols: HAS_POSITIVE_NUMERIC_VALUE,
contentEditable: HAS_BOOLEAN_VALUE,
controls: HAS_BOOLEAN_VALUE,
default: HAS_BOOLEAN_VALUE,
defer: HAS_BOOLEAN_VALUE,
disabled: HAS_BOOLEAN_VALUE,
download: HAS_OVERLOADED_BOOLEAN_VALUE,
draggable: HAS_BOOLEAN_VALUE,
formNoValidate: HAS_BOOLEAN_VALUE,
hidden: HAS_BOOLEAN_VALUE,
loop: HAS_BOOLEAN_VALUE,
Expand All @@ -62,6 +67,7 @@ var HTMLDOMPropertyConfig = {
start: HAS_NUMERIC_VALUE,
// support for projecting regular DOM Elements via V1 named slots ( shadow dom )
span: HAS_POSITIVE_NUMERIC_VALUE,
spellCheck: HAS_BOOLEAN_VALUE,
// Style must be explicitly set in the attribute list. React components
// expect a style object
style: 0,
Expand All @@ -76,21 +82,6 @@ var HTMLDOMPropertyConfig = {
httpEquiv: 0,
// Attributes with mutation methods must be specified in the whitelist
value: 0,
// The following attributes expect boolean values. They must be in
// the whitelist to allow boolean attribute assignment:
autoComplete: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed autoComplete. It is actually on/off, not true/false. Thanks @syranide!

// IE only true/false iFrame attribute
// https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx
allowTransparency: 0,
contentEditable: 0,
draggable: 0,
spellCheck: 0,
// autoCapitalize and autoCorrect are supported in Mobile Safari for
// keyboard hints.
autoCapitalize: 0,
autoCorrect: 0,
// autoSave allows WebKit/Blink to persist values of input fields on page reloads
autoSave: 0,
Copy link
Contributor Author

@nhunzaker nhunzaker Aug 29, 2017

Choose a reason for hiding this comment

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

Same deal with autoSave, autoCorrect, and autoCapitalize. I pulled this over from: #10531

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we special case these so that they work? It's odd when the form <x yyy /> doesn't work like the serialized HTML form.

Copy link
Contributor Author

@nhunzaker nhunzaker Aug 31, 2017

Choose a reason for hiding this comment

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

Just for context, "on" and "off" were deprecated for autocapitalize in iOS5. Acceptable values 1 are "none", "sentences", "words", and "characters". When no value is given, it defaults to "sentence" for form tags and "none" for password input elements, but otherwise uses the attribute on the related form.

This default value appears to be an empty string, at least when I log out input.outerHTML in Safari. It also enables capitalization, at least in a very quick check in the ios simulator.

Hmm. It is frustrating that true is the assignment type for implicit attributes (like <input autocapitalize />). Maybe in a breaking release of JSX there could be a symbol for it, or use an empty string.

In the mean time, should we want to parse true as an empty string for cases like this? Maybe false should warn. Is this behavior safe to generalize on all attributes that don't have the HAS_STRING_BOOLEAN_VALUE flag?

@aweary is this in line what what you were thinking for boolean attributes?


  1. https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#//apple_ref/doc/uid/TP40008058-autocapitalize

Random fun aside: This is my first time using a footnote in a reply on Github. What a time to be alive.

},
DOMAttributeNames: {
acceptCharset: 'accept-charset',
Expand Down
17 changes: 11 additions & 6 deletions src/renderers/dom/shared/SVGDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@

'use strict';

var DOMProperty = require('DOMProperty');

var HAS_BOOLEAN_VALUE = DOMProperty.injection.MUST_USE_PROPERTY;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing this up shortly.


var NS = {
xlink: 'http://www.w3.org/1999/xlink',
xml: 'http://www.w3.org/XML/1998/namespace',
Expand Down Expand Up @@ -113,15 +117,16 @@ var ATTRS = [
'xmlns:xlink',
'xml:lang',
'xml:space',
// The following attributes expect boolean values. They must be in
// the whitelist to allow boolean attribute assignment:
'autoReverse',
'externalResourcesRequired',
'preserveAlpha',
];

var SVGDOMPropertyConfig = {
Properties: {},
Properties: {
// The following attributes expect boolean values. They must be in
// the whitelist to allow boolean attribute assignment:
autoReverse: HAS_BOOLEAN_VALUE,
externalResourcesRequired: HAS_BOOLEAN_VALUE,
preserveAlpha: HAS_BOOLEAN_VALUE,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is curious that these must be here. This is for correct casing? I wonder if we really need to down case attributes:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/DOMProperty.js#L90

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 would be good follow-up work anyway.

Copy link
Collaborator

@gaearon gaearon Aug 30, 2017

Choose a reason for hiding this comment

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

Without this, they don't get set correctly (you can try in an HTML file). I don't know. Maybe not?

DOMAttributeNamespaces: {
xlinkActuate: NS.xlink,
xlinkArcrole: NS.xlink,
Expand Down