Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. by nhunzaker · Pull Request #10385 · react/react

nhunzaker · 2017-08-04T17:20:48Z

Follow-up work from #7311. This PR implements scenario 2 for custom attributes, as described in #7311 (comment).

Badly cased attribute names still write to the DOM.
Cases like classname, htmlfor, and arabicform, do not alias. They write as classname, htmlfor, and arabicform.
All attribute whitelist entries that enforce a specific casing have been removed.
The ARIA whitelist has been moved to a developer-only enforcement (but the logic is the same as master)

Build size:

react-dom.production.min.js: 109.01kb (34.40kb gz)

gaearon · 2017-08-04T17:30:05Z

:O

gaearon · 2017-08-04T17:38:45Z

    // Style must be explicitely set in the attribute list. React components
    // expect a style object
    start: HAS_NUMERIC_VALUE,
+    // Style must be explicitely set in the attribute list. React components


Typo: explicitly

aweary · 2017-08-04T18:06:15Z

-    srcLang: 0,
-    srcSet: 0,
+    // Style must be explicitely set in the attribute list. React components
+    // expect a style object


Duplicate comment?

Yikes! Yes.

Fixed in 3a50c07

gaearon · 2017-08-04T18:31:13Z

How did you verify the changes? Is it worth creating a test page with all these attributes to ensure their support didn’t regress? (At least once.)

nhunzaker · 2017-08-04T18:33:38Z

My testing could be more exhaustive. I'll make a test page.

gaearon · 2017-08-04T18:47:49Z

-    label: 0,
-    lang: 0,
-    list: 0,
-    loop: HAS_BOOLEAN_VALUE,


Is there anything special about it?

Ack! This is a mistake 😢

Added back in a218130

Looks like this is still missing in the latest version here, unless I'm missing something?

Oh I see it moved down.

gaearon · 2017-08-04T18:49:44Z

-    srcSet: 0,
-    start: HAS_NUMERIC_VALUE,
-    step: 0,
+    // Style must be explicitly set in the attribute list. React components


Is this a note about server renderer? I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

Excellent questions. I will verify.

I imagine we don't get into style branch on the client because there’s a hardcoded one earlier.

We don't get to the style branch, but we validate all properties here:
https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L406

The UnknownPropertyHook calls DOMProperty.shouldSetAttribute and sees that the style property is using an object, so it warns. This check doesn't happen if the style property is in the attribute config.

So keeping style: 0 here prevents the warning.

What about custom elements? Does style still work correctly on those? (And did it at all in the past?)

What is the intended behavior of passing style to custom elements? Should it pass the value "as is", or provide style pre-processing? Right now it looks like it's sending down a style string.

I think for now we're okay with doing whatever we used to be doing.

gaearon · 2017-08-07T11:15:39Z

    itemScope: HAS_BOOLEAN_VALUE,
    // Facebook internal attribute. This is an object attribute that
-    // impliments a custom toString() method
+    // impliments a custom toString() method. Objects are not allowed as


Typo: implements

Man I need a git commit hook for impliments. Every. Single. Time. Fixed in aaafa7f

nhunzaker · 2017-08-07T11:33:39Z

+
+    return DOMProperty.properties.hasOwnProperty(lowerCased)
+      ? DOMProperty.properties[lowerCased]
+      : null;


Just a final word here. In this scenario, all attributes are referenced as their lowercase form internally. I've hidden that within getPropertyInfo to avoid needing to know about lowercasing attributes outside of DOMProperty.js.

nhunzaker · 2017-08-07T11:34:37Z

-  zoomAndPan: 'zoomAndPan',
 };

 var SVGDOMPropertyConfig = {


I feel like there's another step here where we could string manipulate xml:lang or vert-origin-y into camel-case, that might cut the overall gzip size.

Ah I want to try it. I'll take 5.

nhunzaker · 2017-08-07T11:54:53Z


-      if (DOMPropertyNames.hasOwnProperty(propName)) {
-        propertyInfo.propertyName = DOMPropertyNames[propName];
-      }


DOMPropertyNames is not used by any injection.

gaearon

This is amazing work.

There's a (hopefully!) last change in semantics we need to make that I wrote about in #10399 (comment).

Thanks again for working through this! I really hope this is the last change we need to make. If not, I’ll try to address the rest myself tomorrow.

nhunzaker · 2017-08-07T23:20:16Z

-  additive: 0,
-  alignmentBaseline: 'alignment-baseline',
-  allowReorder: 'allowReorder',
-  alphabetic: 0,


I can't figure out if this is a boolean attribute or not.

Number. https://www.w3.org/TR/SVG/fonts.html#FontFaceElementAlphabeticAttribute

I just discovered this page. True life saver.

nhunzaker · 2017-08-07T23:21:38Z

-  ascent: 0,
-  attributeName: 'attributeName',
-  attributeType: 'attributeType',
-  autoReverse: 'autoReverse',


This one either.

Boolean. https://www.w3.org/TR/smil/smil-timemanip.html#TimeManip-autoReverseSyntax

This is not in the spec.

nhunzaker · 2017-08-07T23:49:32Z

-  hanging: 0,
-  horizAdvX: 'horiz-adv-x',
-  horizOriginX: 'horiz-origin-x',
-  ideographic: 0,


This is a value for dominantBaseline, and I can't find a description of it anywhere.

Number.
https://www.w3.org/TR/SVG/fonts.html#FontFaceElementIdeographicAttribute

Jeez Nate. Why don't you read the spec?

Number. https://www.w3.org/TR/SVG/fonts.html#FontFaceElementIdeographicAttribute

Haha :-) It's a bit tough to find.

gaearon · 2017-08-08T00:04:33Z

We'll really need some automated way to test we didn't regress here (just once). I'm thinking maybe we could take all existing whitelists, make a page that renders all these properties with React 15, and then verify these values are still there with 16. I'll try to do it tomorrow.

nhunzaker · 2017-08-08T00:07:35Z

@gaearon Okay. This is not in a fully tested, and possibly not in a working state, but I've pushed up commits that add back what I believe are all the boolean properties. In addition to the ones you identified, I went through the entire SVG spec (okay okay I cmd + f'ed through it) and the only boolean SVG attributes I could find were:

autoReverse
externalResourcesRequired
preserveAlpha

autoReverse is technically from SMIL. I'll browse through and see if we aren't missing anything.

nhunzaker · 2017-08-08T00:18:04Z

SMIL state module defines a few more:
https://www.w3.org/TR/smil/smil-DTD.html

'skipContent'
'external'
'syncMaster'

I think that's it.

…st of whitelist. (react#10385) * Allow custom attributes. Add flag to toggle custom attribute behavior * Update custom attribute logic - Only allow string attributes - Remove custom attribute feature flag - Add additional tests for data, aria, and custom attributes * Allow numbers and booleans custom attributes. Cut isCustomAttribute * Cover objects with custom attributes in warning test * Rename DOMProperty.isWriteable to shouldSetAttribute * Rework conditions in shouldSetProperty to avoid edge cases * Update unknown property warning to include custom attribute information * Remove ref and key from reserved props * Ensure SSR test coverage for DOMProperty injections * Add ajaxify attribute for internal FB support * Ajaxify is a stringifiable object attribute * Remove non-case sensitive standard attributes. Make ARIA hook dev only. * Update test name for custom attributes on custom elements * Remove SSR custom injection test * Remove case sensitive props * Remove onAfterResetModules hooks in SSR render tests * Add back a few attributes and explain why they are needed * Remove possibleStandardNames from DOMProperty.js * Fix typo in HTMLPropertyConfig comment * Remove duplicative comment * Add back loop boolean property * Do not allow assignment of attributes that are aliased * Update custom attribute test to check value, not just presence * Address case where class is assigned as an attribute on custom elements. Improve SSR tests * Cover cases where className and for are given to custom elements * Remove unnecessary spys on console.error. Reduce extra space in tests * Cover cased custom attributes in SSR tests * Custom attributes are case sensitive * Allow uppercase letters in custom attributes. Address associated edge cases * Allow improperly cased aliased attributes. Add additional tests * Handle special properties like onFocusOut * Add some comments to document where casing matters. Remove DOMPropertyNames * Fix spelling mistake in ajaxify HTML property comment * Make ARIA enforcement dev-only * Remove alias test that covers multiple aliases for one property * Fix typo in comment * Build SVG aliases dynamically * Remove unused DOMPropertyNames reference * Do not translate bad casings of aliased attributes - classname writes to the DOM as classname - class does not write to the DOM - cLASS does not write to the DOM - arabic-form does not write to the DOM * Revise the way custom booleans are treated - Custom attributes can not have boolean values unless they are aria or data attributes - Attributes with boolean values have been added back to the whitelist - Warnings now exclude booleans from supported types - Associated test coverage * Add developer warnings for NaN and ARIA hooks * Use string comparison instead of regex to check for data and aria attributes. * Warn about unsupported properties without case sensitivity * Remove attributes that are updated to invalid values * Support object property values with toString methods. Allow boolean props to coerce objects * Add back ajaxify test * Address bad references in ReactDOMComponent-test. Format. * Revert changes to the docs We'll update them separately * Allow all objects and pass incorrect aliases

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

gaearon reviewed Aug 4, 2017

View reviewed changes

aweary reviewed Aug 4, 2017

View reviewed changes

gaearon reviewed Aug 4, 2017

View reviewed changes

nhunzaker mentioned this pull request Aug 5, 2017

Allow custom attributes by default #7311

Closed

nhunzaker force-pushed the nh-gut-whitelist branch from a218130 to 019de2b Compare August 7, 2017 02:35

nhunzaker changed the title ~~Remove remaining entries in whitelist that are only needed for developer warnings~~ Custom Attributes Scenario 2 Aug 7, 2017

gaearon reviewed Aug 7, 2017

View reviewed changes

nhunzaker changed the title ~~Custom Attributes Scenario 2~~ Custom Attributes Scenario 1: Write badly cased attributes. Remove most of whitelist. Aug 7, 2017

nhunzaker changed the title ~~Custom Attributes Scenario 1: Write badly cased attributes. Remove most of whitelist.~~ Custom Attributes Scenario 2: Write badly cased attributes. Remove most of whitelist. Aug 7, 2017

nhunzaker commented Aug 7, 2017

View reviewed changes

nhunzaker force-pushed the nh-gut-whitelist branch from aaafa7f to d51ee9d Compare August 7, 2017 11:54

nhunzaker commented Aug 7, 2017

View reviewed changes

nhunzaker changed the base branch from nh-strip-whitelist to master August 7, 2017 12:02

nhunzaker mentioned this pull request Aug 7, 2017

Cased data attributes should properly diff on server hydration #10394

Closed

This was referenced Aug 7, 2017

RFC: Plan for Attributes in React 16 #10399

Closed

Implement Setting Unknown Attributes #9477

Closed

gaearon previously requested changes Aug 7, 2017

View reviewed changes

nhunzaker mentioned this pull request Aug 7, 2017

Custom Attributes Scenario 1: Do not write badly cased attributes. Keep Whitelist. #10397

Closed

nhunzaker commented Aug 7, 2017

View reviewed changes

gaearon mentioned this pull request Oct 2, 2017

Fix tabIndex attribute for SVG #11033

Merged

gaearon deleted the nh-gut-whitelist branch October 6, 2017 21:57

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

gaearon mentioned this pull request Jan 9, 2018

add srcObject for getUserMedia #9146

Closed

aweary mentioned this pull request Feb 7, 2018

Feature request: Support server-side rendering of non-standard DOM attribute names (eg. AMP's [prop]="value") #10064

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

Uh oh!

Conversation

nhunzaker commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Aug 4, 2017

Uh oh!

nhunzaker commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Aug 8, 2017

Uh oh!

nhunzaker commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nhunzaker commented Aug 4, 2017 •

edited

Loading

nhunzaker commented Aug 8, 2017 •

edited

Loading