Changes to attribute whitelist logic #10564

nhunzaker · 2017-08-29T01:55:24Z

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

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.

When we originally removed attributes from the whitelist, we assumed a few attributes were string booleans, but they are not: Autocomplete ("on", "off") https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#autocomplete Autocapitalize ("none", "sentence", "words", ...) https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#autocapitalize Autocorrect ("on", "off") https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#autocorrect Autosave (string) https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#autosave

nhunzaker · 2017-08-29T01:56:33Z

src/renderers/dom/shared/HTMLDOMPropertyConfig.js

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


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

nhunzaker · 2017-08-29T01:57:38Z

src/renderers/dom/shared/HTMLDOMPropertyConfig.js

-    autoCapitalize: 0,
-    autoCorrect: 0,
-    // autoSave allows WebKit/Blink to persist values of input fields on page reloads
-    autoSave: 0,


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

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.

Just for context, "on" and "off" were deprecated for autocapitalize in iOS5. Acceptable values ₁ 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?

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.

gaearon · 2017-08-29T02:04:09Z

src/renderers/dom/shared/HTMLDOMPropertyConfig.js

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


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?

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

nhunzaker · 2017-08-29T02:14:40Z

src/renderers/dom/shared/SVGDOMPropertyConfig.js

@@ -11,6 +11,10 @@

 'use strict';

+var DOMProperty = require('DOMProperty');
+
+var HAS_BOOLEAN_VALUE = DOMProperty.injection.MUST_USE_PROPERTY;


Fixing this up shortly.

nhunzaker · 2017-08-29T04:02:11Z

src/renderers/dom/shared/HTMLDOMPropertyConfig.js

-    // 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 must be in here for backwards compatibility.

nhunzaker · 2017-08-29T04:02:57Z

src/renderers/dom/shared/DOMMarkupOperations.js

      }
-      return attributeName + '=' + quoteAttributeValueForBrowser(value);


I'd love to just lean on DOMProperty.shouldSetAttribute, but reserved props pass through here, and style is in there. Maybe we could remove style from reserved props.

gaearon · 2017-08-29T09:03:34Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

@@ -597,15 +597,15 @@ describe('ReactDOMServerIntegration', () => {
      });

      // this probably is just masking programmer error, but it is existing behavior.


This comment is now outdated.

gaearon · 2017-08-29T09:03:46Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

-        expect(e.getAttribute('class')).toBe('true');
+      itRenders('no className prop with true value', async render => {
+        const e = await render(<div className={true} />, 1);
+        expect(e.hasAttribute('class')).toBe(false);
      });

      // this probably is just masking programmer error, but it is existing behavior.


gaearon · 2017-08-29T09:04:02Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

@@ -664,15 +664,15 @@ describe('ReactDOMServerIntegration', () => {
      });

      // this probably is just masking programmer error, but it is existing behavior.
-      itRenders('htmlFor prop with true value', async render => {


gaearon · 2017-08-29T09:04:11Z

src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js

-        expect(e.getAttribute('for')).toBe('true');
+      itRenders('no htmlFor prop with true value', async render => {
+        const e = await render(<div htmlFor={true} />, 1);
+        expect(e.hasAttribute('for')).toBe(false);
      });

      // this probably is just masking programmer error, but it is existing behavior.


- Adds exceptions to isCustomAttribute for dashed SVG elements - Use consistent custom element check across all modules

nhunzaker · 2017-08-29T12:18:06Z

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

nhunzaker · 2017-08-29T12:33:16Z

src/renderers/dom/shared/utils/isCustomComponent.js

+  'font-face-src': true,
+  'font-face-uri': true,
+  'missing-glyph': true,
+};


Didn't see the namespace work. Cool. This is inferior, I'll rework this.

nhunzaker · 2017-08-29T13:49:26Z

src/renderers/dom/shared/utils/isCustomComponent.js

+  if (tagName.indexOf('-') >= 0 || props.is != null) {
+    // TODO: We always have a namespace with fiber. Drop the first
+    // check when Stack is removed.
+    return namespace == null || namespace === HTML_NAMESPACE;


@gaearon I moved the namespace check into this module. Stack validates properties before the namespace is assigned to the element, so we don't know if the element is SVG or HTML during initial validations. namespace == null can go away when stack is removed. This also preserves the custom element behavior for 15.x, if that matters.

aweary · 2017-08-29T15:05:15Z

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

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


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.

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

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.

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.

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.

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

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

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.

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?

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"

nhunzaker · 2017-08-29T16:52:50Z

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 · 2017-08-29T16:53:44Z

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

nhunzaker added 2 commits August 28, 2017 21:54

Only HAS_BOOLEAN_VALUE attribute flag can assign booleans

711bafe

facebook-github-bot added the CLA Signed label Aug 29, 2017

nhunzaker commented Aug 29, 2017

View reviewed changes

Use a non-boolean attribute in object assignment tests

4c5dfbb

gaearon reviewed Aug 29, 2017

View reviewed changes

nhunzaker commented Aug 29, 2017

View reviewed changes

nhunzaker added 3 commits August 28, 2017 22:36

Add HAS_STRING_BOOLEAN_VALUE attribute flag

e086ff1

Fix boolean tests, add boolean warning.

0410651

Reserved props should allow booleans

ab75d9a

nhunzaker commented Aug 29, 2017

View reviewed changes

gaearon reviewed Aug 29, 2017

View reviewed changes

gaearon and others added 4 commits August 29, 2017 02:08

Remove outdated comments

6077e0b

Style tweaks

6f913ed

Don't treat dashed SVG tags as custom elements

aca8d9c

SVG elements like font-face are not custom attributes

af7d035

- Adds exceptions to isCustomAttribute for dashed SVG elements - Use consistent custom element check across all modules

nhunzaker commented Aug 29, 2017

View reviewed changes

Move namespace check to isCustomAttribute. Add caveat for stack.

9c0751f

nhunzaker commented Aug 29, 2017

View reviewed changes

Remove unused namespace variable assignment

f8da44e

aweary reviewed Aug 29, 2017

View reviewed changes

gaearon added 3 commits August 29, 2017 09:07

Fix the DEV-only whitelist

b93e093

Don't read property twice

fbcced1

Ignore and warn about non-string is attribute

a270e03

gaearon force-pushed the nh-boolean-updates branch from 55eb136 to a270e03 Compare August 29, 2017 16:49

gaearon deleted the nh-boolean-updates branch August 31, 2017 01:29

nhunzaker mentioned this pull request Aug 31, 2017

Remove HTMLPropertyConfig entries for non-boolean values #10531

Closed

bvaughn mentioned this pull request Sep 7, 2017

React 16 RC #10294

Closed

gaearon mentioned this pull request Sep 8, 2017

Allow custom (nonstandard) attributes. #140

Closed

hph mentioned this pull request Sep 27, 2017

Allow custom attributes by default #7311

Closed

This was referenced Dec 1, 2017

react-dom versions available: 16.0.0 kornicameister/korni#158

Closed

react versions available: 16.0.0 kornicameister/korni#159

Closed

renovate bot mentioned this pull request Feb 22, 2018

Update react monorepo packages to v16.2.0 mozilla/ensemble#28

Closed

This was referenced Mar 21, 2018

Update react to 16.3.2-abandoned sbardian/DebtTrackerAPI#15

Closed

Update react-dom to ^16.0.0-abandoned sbardian/DebtTrackerAPI#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to attribute whitelist logic #10564

Changes to attribute whitelist logic #10564

nhunzaker commented Aug 29, 2017 •

edited by gaearon

nhunzaker Aug 29, 2017

nhunzaker Aug 29, 2017 •

edited

sebmarkbage Aug 30, 2017

nhunzaker Aug 31, 2017 •

edited

gaearon Aug 29, 2017

nhunzaker Aug 29, 2017

nhunzaker Aug 29, 2017

nhunzaker Aug 29, 2017

nhunzaker Aug 29, 2017

gaearon Aug 29, 2017

gaearon Aug 29, 2017

gaearon Aug 29, 2017

gaearon Aug 29, 2017

nhunzaker commented Aug 29, 2017

nhunzaker Aug 29, 2017

nhunzaker Aug 29, 2017

aweary Aug 29, 2017

gaearon Aug 29, 2017

aweary Aug 29, 2017

gaearon Aug 29, 2017 •

edited

aweary Aug 29, 2017 •

edited

sebmarkbage Aug 29, 2017

sebmarkbage Aug 29, 2017

aweary Aug 29, 2017

sebmarkbage Aug 30, 2017

aweary Aug 30, 2017 •

edited

nhunzaker commented Aug 29, 2017

nhunzaker commented Aug 29, 2017

		}
		return attributeName + '=' + quoteAttributeValueForBrowser(value);

		@@ -597,15 +597,15 @@ describe('ReactDOMServerIntegration', () => {
		});

		// this probably is just masking programmer error, but it is existing behavior.

Changes to attribute whitelist logic #10564

Changes to attribute whitelist logic #10564

Conversation

nhunzaker commented Aug 29, 2017 • edited by gaearon

Choose a reason for hiding this comment

nhunzaker Aug 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Aug 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Aug 29, 2017 • edited

Choose a reason for hiding this comment

aweary Aug 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary Aug 30, 2017 • edited

Choose a reason for hiding this comment

nhunzaker commented Aug 29, 2017

nhunzaker commented Aug 29, 2017

nhunzaker commented Aug 29, 2017 •

edited by gaearon

nhunzaker Aug 29, 2017 •

edited

nhunzaker Aug 31, 2017 •

edited

gaearon Aug 29, 2017 •

edited

aweary Aug 29, 2017 •

edited

aweary Aug 30, 2017 •

edited