-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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 10 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 |
---|---|---|
|
@@ -99,8 +99,12 @@ var DOMMarkupOperations = { | |
(propertyInfo.hasOverloadedBooleanValue && value === true) | ||
) { | ||
return attributeName + '=""'; | ||
} else if ( | ||
typeof value !== 'boolean' || | ||
DOMProperty.shouldAttributeAcceptBooleanValue(name) | ||
) { | ||
return attributeName + '=' + quoteAttributeValueForBrowser(value); | ||
} | ||
return attributeName + '=' + quoteAttributeValueForBrowser(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. I'd love to just lean on |
||
} else if (DOMProperty.shouldSetAttribute(name, value)) { | ||
if (value == null) { | ||
return ''; | ||
|
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, | ||
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. Removed autoComplete. It is actually |
||
// 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, | ||
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. Same deal with 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. Shouldn't we special case these so that they work? It's odd when the form 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. Just for context, This default value appears to be an empty string, at least when I log out Hmm. It is frustrating that In the mean time, should we want to parse @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. |
||
// 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 |
---|---|---|
|
@@ -2003,7 +2003,7 @@ describe('ReactDOMComponent', () => { | |
expect(el.hasAttribute('whatever')).toBe(false); | ||
|
||
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Invalid prop `whatever` on <div> tag', | ||
'Warning: Received `true` for non-boolean attribute `whatever`', | ||
); | ||
}); | ||
|
||
|
@@ -2016,7 +2016,7 @@ describe('ReactDOMComponent', () => { | |
expect(el.hasAttribute('whatever')).toBe(false); | ||
|
||
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Invalid prop `whatever` on <div> tag', | ||
'Warning: Received `true` for non-boolean attribute `whatever`', | ||
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. Why does it warn about being non-boolean for unknown attributes? It seems like we don't know whether an unknown attribute is actually a boolean attribute or not, so this could lead to false positives. 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. Maybe we can tweak the wording here. But it is intentional that behavior is the same for known and unknown attributes. 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. In what way is the behavior the same? Known boolean attributes will not warn because of the 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.
Yes, that's what I meant. The only ones for which we pass booleans through are the booleans we know. We don't pass booleans through for either known non-booleans or unknowns. This keeps it unobservable whether a certain non-boolean attribute is known or unknown. Without this guarantee we can't hide the fact that, for example, Of course that means we can never delete booleans from the whitelist. But that seems like a fair tradeoff. 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. So how can a user correctly use a boolean attribute that we don't have whitelisted? I don't think behavior changes in previously-known attributes implies implementation details are leaked; it just means attributes behavior has changed. Which is why this is part of a major release. Why not instead utilize warnings in this major release that would allow us to treat any unknown attribute with boolean values as a boolean attribute in the next?
I don't agree that having to maintain a boolean whitelist forever is a fair tradeoff, it introduces the same issues we had with the whitelist in the first place, just with a smaller subset of attributes. 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. <div />; // false
<div bool-attr="" />; // true 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. I don't think custom elements should accept booleans coerced to strings neither. 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's easy enough for static attribute values but once you have to start dynamically setting and unsetting boolean attributes you have to start switching between 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. That's when we add them to the whitelist. There's no consistent pattern for what booleans should do (clear attribute vs "off" vs "false" vs "no"). So we'll always need some whitelist. In theory we could make the default behavior be whatever has the most attributes following that rule. Do we know for sure which that is? 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. In my mind the goal is to reduce that inconsistency, which we maintain by coercing booleans to
Looking at this attribute table in the spec it seems clear that there are far more boolean attributes than there are attributes that accept |
||
); | ||
}); | ||
|
||
|
@@ -2096,10 +2096,8 @@ describe('ReactDOMComponent', () => { | |
|
||
describe('Object stringification', function() { | ||
it('allows objects on known properties', function() { | ||
var el = ReactTestUtils.renderIntoDocument( | ||
<div allowTransparency={{}} />, | ||
); | ||
expect(el.getAttribute('allowtransparency')).toBe('[object Object]'); | ||
var el = ReactTestUtils.renderIntoDocument(<div acceptCharset={{}} />); | ||
expect(el.getAttribute('accept-charset')).toBe('[object Object]'); | ||
}); | ||
|
||
it('should pass objects as attributes if they define toString', () => { | ||
|
@@ -2157,4 +2155,67 @@ describe('ReactDOMComponent', () => { | |
expect(el.getAttribute('ajaxify')).toBe('ajaxy'); | ||
}); | ||
}); | ||
|
||
describe('String boolean attributes', function() { | ||
it('does not assign string boolean attributes for custom attributes', function() { | ||
spyOn(console, 'error'); | ||
|
||
var el = ReactTestUtils.renderIntoDocument(<div whatever={true} />); | ||
|
||
expect(el.hasAttribute('whatever')).toBe(false); | ||
|
||
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Received `true` for non-boolean attribute `whatever`.', | ||
); | ||
}); | ||
|
||
it('stringifies the boolean true for allowed attributes', function() { | ||
var el = ReactTestUtils.renderIntoDocument(<div spellCheck={true} />); | ||
|
||
expect(el.getAttribute('spellCheck')).toBe('true'); | ||
}); | ||
|
||
it('stringifies the boolean false for allowed attributes', function() { | ||
var el = ReactTestUtils.renderIntoDocument(<div spellCheck={false} />); | ||
|
||
expect(el.getAttribute('spellCheck')).toBe('false'); | ||
}); | ||
|
||
it('stringifies implicit booleans for allowed attributes', function() { | ||
// eslint-disable-next-line react/jsx-boolean-value | ||
var el = ReactTestUtils.renderIntoDocument(<div spellCheck />); | ||
|
||
expect(el.getAttribute('spellCheck')).toBe('true'); | ||
}); | ||
}); | ||
|
||
describe('Hyphenated SVG elements', function() { | ||
it('the font-face element is not a custom element', function() { | ||
spyOn(console, 'error'); | ||
var el = ReactTestUtils.renderIntoDocument( | ||
<font-face x-height={false} />, | ||
); | ||
|
||
expect(el.hasAttribute('x-height')).toBe(false); | ||
|
||
expectDev(console.error.calls.count()).toBe(1); | ||
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Invalid DOM property `x-height`. Did you mean `xHeight`', | ||
); | ||
}); | ||
|
||
it('the font-face element does not allow unknown boolean values', function() { | ||
spyOn(console, 'error'); | ||
var el = ReactTestUtils.renderIntoDocument( | ||
<font-face whatever={false} />, | ||
); | ||
|
||
expect(el.hasAttribute('whatever')).toBe(false); | ||
|
||
expectDev(console.error.calls.count()).toBe(1); | ||
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Received `false` for non-boolean attribute `whatever`.', | ||
); | ||
}); | ||
}); | ||
}); |
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.
We probably don't intend to merge this part, right?
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.
Nope :P
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.
Lol. I honestly thought that was an edge case test. 😨