-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Changes from all commits
e10694a
711bafe
4c5dfbb
e086ff1
0410651
ab75d9a
6077e0b
6f913ed
aca8d9c
af7d035
9c0751f
f8da44e
b93e093
fbcced1
a270e03
ed92af0
5a339a1
0b2ba65
3f85316
76a6318
b29bb74
0a2aec4
501e86d
8d1f487
72666fa
cb687ed
1d61379
2b0f61a
1590c2a
cb883a6
32f6321
6d9c0e0
10e0c09
ba71ec1
dc760af
10950c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,17 @@ var HAS_POSITIVE_NUMERIC_VALUE = | |
DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE; | ||
var HAS_OVERLOADED_BOOLEAN_VALUE = | ||
DOMProperty.injection.HAS_OVERLOADED_BOOLEAN_VALUE; | ||
var HAS_STRING_BOOLEAN_VALUE = DOMProperty.injection.HAS_STRING_BOOLEAN_VALUE; | ||
|
||
var HTMLDOMPropertyConfig = { | ||
// When adding attributes to this list, be sure to also add them to | ||
// the `possibleStandardNames` module to ensure casing and incorrect | ||
// 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_STRING_BOOLEAN_VALUE, | ||
// specifies target context for links with `preload` type | ||
async: HAS_BOOLEAN_VALUE, | ||
// autoFocus is polyfilled/normalized by AutoFocusUtils | ||
|
@@ -35,11 +39,13 @@ var HTMLDOMPropertyConfig = { | |
capture: HAS_BOOLEAN_VALUE, | ||
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, | ||
cols: HAS_POSITIVE_NUMERIC_VALUE, | ||
contentEditable: HAS_STRING_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_STRING_BOOLEAN_VALUE, | ||
formNoValidate: HAS_BOOLEAN_VALUE, | ||
hidden: HAS_BOOLEAN_VALUE, | ||
loop: HAS_BOOLEAN_VALUE, | ||
|
@@ -62,6 +68,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_STRING_BOOLEAN_VALUE, | ||
// Style must be explicitly set in the attribute list. React components | ||
// expect a style object | ||
style: 0, | ||
|
@@ -75,22 +82,8 @@ var HTMLDOMPropertyConfig = { | |
htmlFor: 0, | ||
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, | ||
// 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, | ||
// Set the string boolean flag to allow the behavior | ||
value: HAS_STRING_BOOLEAN_VALUE, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This must be in here for backwards compatibility. |
||
}, | ||
DOMAttributeNames: { | ||
acceptCharset: 'accept-charset', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ | |
|
||
'use strict'; | ||
|
||
var DOMProperty = require('DOMProperty'); | ||
|
||
var {HAS_STRING_BOOLEAN_VALUE} = DOMProperty.injection; | ||
|
||
var NS = { | ||
xlink: 'http://www.w3.org/1999/xlink', | ||
xml: 'http://www.w3.org/XML/1998/namespace', | ||
|
@@ -113,15 +117,19 @@ 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: { | ||
autoReverse: HAS_STRING_BOOLEAN_VALUE, | ||
externalResourcesRequired: HAS_STRING_BOOLEAN_VALUE, | ||
preserveAlpha: HAS_STRING_BOOLEAN_VALUE, | ||
}, | ||
DOMAttributeNames: { | ||
autoReverse: 'autoReverse', | ||
externalResourcesRequired: 'externalResourcesRequired', | ||
preserveAlpha: 'preserveAlpha', | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be good follow-up work anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -134,7 +142,6 @@ var SVGDOMPropertyConfig = { | |
xmlLang: NS.xml, | ||
xmlSpace: NS.xml, | ||
}, | ||
DOMAttributeNames: {}, | ||
}; | ||
|
||
var CAMELIZE = /[\-\:]([a-z])/g; | ||
|
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.
Same deal with
autoSave
,autoCorrect
, andautoCapitalize
. I pulled this over from: #10531There 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.
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.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.
Just for context,
"on"
and"off"
were deprecated forautocapitalize
in iOS5. Acceptable values 1 are"none"
,"sentences"
,"words"
, and"characters"
. When no value is given, it defaults to "sentence" forform
tags and "none" for passwordinput
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? Maybefalse
should warn. Is this behavior safe to generalize on all attributes that don't have theHAS_STRING_BOOLEAN_VALUE
flag?@aweary is this in line what what you were thinking for boolean attributes?
Random fun aside: This is my first time using a footnote in a reply on Github. What a time to be alive.