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

Conversation

Projects
None yet
6 participants
@nhunzaker
Collaborator

nhunzaker commented Aug 29, 2017

This is a work in progress PR so that @gaearon and I can coordinate.

  • Block and warn about non-boolean attributes getting true and false.
    • Bug: <svg><font-face x-height={false} /></svg> passes through.
  • NaN should always be stringified, and should always warn.
  • Functions should always be stringified, and should always warn, except for events.
  • Symbols should always be ignored, and should always warn.
    • Bug: <svg><font-face-format string={Symbol()} /></svg> throws
  • aria (exactly) and anything starting with on (except events) should always be blocked and warn.
  • <div onClick={0}> should warn. Also NaN, false.
  • It turns out we did support data before. Should we allow it again?
  • Make unknown event message better (it currently advises to use strings which won't work)
  • Pass badly cased reserved attributes through

Possible future work:

  • Fix SVG <font-face> taking custom element path
  • Warn if numeric attributes get non-numeric values
  • Warn if Symbol is passed to special props like value, dangerouslySetInnerHTML
  • Warn if function is passed to special prop like suppressContentEditableWarning
  • Don't pass function to special props like <textarea value>, etc
  • Should we blacklist selectedIndex? It used to warn, now it confusingly gets set.
  • Let's make off/on and yes/no attributes work as intended.
  • Same for crossorigin.
autoCapitalize: 0,
autoCorrect: 0,
// autoSave allows WebKit/Blink to persist values of input fields on page reloads
autoSave: 0,

This comment has been minimized.

@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

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

@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

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

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 30, 2017

Member

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.

@sebmarkbage

sebmarkbage Aug 30, 2017

Member

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.

This comment has been minimized.

@nhunzaker

nhunzaker Aug 31, 2017

Collaborator

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.

@nhunzaker

nhunzaker Aug 31, 2017

Collaborator

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.

nhunzaker added some commits Aug 29, 2017

// 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,

This comment has been minimized.

@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

This must be in here for backwards compatibility.

@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

This must be in here for backwards compatibility.

gaearon and others added some commits Aug 29, 2017

SVG elements like font-face are not custom attributes
- Adds exceptions to isCustomAttribute for dashed SVG elements
- Use consistent custom element check across all modules
@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

Need to decide if aria- and data- should take booleans. (Presumably yes?)

My vote is yes. There should be no danger here, and it is the behavior on 15.x

Collaborator

nhunzaker commented Aug 29, 2017

Need to decide if aria- and data- should take booleans. (Presumably yes?)

My vote is yes. There should be no danger here, and it is the behavior on 15.x

@@ -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`',

This comment has been minimized.

@aweary

aweary Aug 29, 2017

Member

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.

@aweary

aweary Aug 29, 2017

Member

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.

This comment has been minimized.

@gaearon

gaearon Aug 29, 2017

Member

Maybe we can tweak the wording here. But it is intentional that behavior is the same for known and unknown attributes.

@gaearon

gaearon Aug 29, 2017

Member

Maybe we can tweak the wording here. But it is intentional that behavior is the same for known and unknown attributes.

This comment has been minimized.

@aweary

aweary Aug 29, 2017

Member

In what way is the behavior the same? Known boolean attributes will not warn because of the propertyInfo checks. I'm not sure I understand why we assume unknown attributes being passed booleans are not boolean attributes.

@aweary

aweary Aug 29, 2017

Member

In what way is the behavior the same? Known boolean attributes will not warn because of the propertyInfo checks. I'm not sure I understand why we assume unknown attributes being passed booleans are not boolean attributes.

This comment has been minimized.

@gaearon

gaearon Aug 29, 2017

Member

Known boolean attributes will not warn because of the propertyInfo checks.

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, src was cut from the whitelist. Implementation details start to leak out in the behavior.

Of course that means we can never delete booleans from the whitelist. But that seems like a fair tradeoff.

@gaearon

gaearon Aug 29, 2017

Member

Known boolean attributes will not warn because of the propertyInfo checks.

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, src was cut from the whitelist. Implementation details start to leak out in the behavior.

Of course that means we can never delete booleans from the whitelist. But that seems like a fair tradeoff.

This comment has been minimized.

@aweary

aweary Aug 29, 2017

Member

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?

Of course that means we can never delete booleans from the whitelist. But that seems like a fair tradeoff.

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.

@aweary

aweary Aug 29, 2017

Member

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?

Of course that means we can never delete booleans from the whitelist. But that seems like a fair tradeoff.

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.

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 29, 2017

Member
<div />; // false 
<div bool-attr="" />; // true
@sebmarkbage

sebmarkbage Aug 29, 2017

Member
<div />; // false 
<div bool-attr="" />; // true

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 29, 2017

Member

I don't think custom elements should accept booleans coerced to strings neither.

@sebmarkbage

sebmarkbage Aug 29, 2017

Member

I don't think custom elements should accept booleans coerced to strings neither.

This comment has been minimized.

@aweary

aweary Aug 29, 2017

Member

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 "" and null and remember those map to true and false, which is unfortunate.

@aweary

aweary Aug 29, 2017

Member

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 "" and null and remember those map to true and false, which is unfortunate.

This comment has been minimized.

@sebmarkbage

sebmarkbage Aug 30, 2017

Member

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?

@sebmarkbage

sebmarkbage Aug 30, 2017

Member

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?

This comment has been minimized.

@aweary

aweary Aug 30, 2017

Member

In my mind the goal is to reduce that inconsistency, which we maintain by coercing booleans to "true" or "on". In the long-term I think it makes sense to require users to be explicit about those enumerated attributes that expect boolean-ish values, and that's something we can add warnings for.

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?

Looking at this attribute table in the spec it seems clear that there are far more boolean attributes than there are attributes that accept "true"/"false" or "on"/"off"

@aweary

aweary Aug 30, 2017

Member

In my mind the goal is to reduce that inconsistency, which we maintain by coercing booleans to "true" or "on". In the long-term I think it makes sense to require users to be explicit about those enumerated attributes that expect boolean-ish values, and that's something we can add warnings for.

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?

Looking at this attribute table in the spec it seems clear that there are far more boolean attributes than there are attributes that accept "true"/"false" or "on"/"off"

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 29, 2017

Member

I intentionally didn't do this because we didn't need to read it from the DOM except the rare case when the first check is positive. Can we restructure so that we don't read it unless necessary?

I intentionally didn't do this because we didn't need to read it from the DOM except the rare case when the first check is positive. Can we restructure so that we don't read it unless necessary?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

Ah I see now. That's a bit of a bummer because we get the namespace URI from an argument elsewhere. We only read it directly in setInitialProperties... hmm.

Collaborator

nhunzaker commented Aug 29, 2017

Ah I see now. That's a bit of a bummer because we get the namespace URI from an argument elsewhere. We only read it directly in setInitialProperties... hmm.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 29, 2017

Collaborator

Nope. I'm totally wrong. I'll take a look either way.

Collaborator

nhunzaker commented Aug 29, 2017

Nope. I'm totally wrong. I'll take a look either way.

@bvaughn bvaughn referenced this pull request Sep 7, 2017

Closed

React 16 RC #10294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment