Skip to content
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

Cased data attributes should properly diff on server hydration #10394

Closed
wants to merge 3 commits into from

Conversation

nhunzaker
Copy link
Contributor

@gaearon noticed this in my custom attribute PR (#7311 (comment))

Attribute names are reported back from the DOM as lower case. If a cased data attribute is given, the diffing process gets tripped up because a propKey of data-myAttribute reports back as data-myattribute. This results in an error along the lines of:

We expected 0 warning(s), but saw 1 warning(s).
We saw these warnings:
Warning: Extra attributes from the server: data-myattribute

The diffing process compares the component props to a list of attributes directly from the DOM element. The DOM list always has lower case names. The React property object uses whatever was given to the component. So there's an inconsistency between the data-myAttribute React prop, and the data-myattribute from the DOM.

This isn't a problem for attributes in the whitelist. When properties are injected into DOMProperties.properties, React adds an attributeName that is the lower case version of the React prop key name. This attribute name is what gets removed from the list of diffed props.

Also it looks like cased data attributes are technically supported in React 15.6.1:
https://jsfiddle.net/84v837e9/185/

The casing goes away when the attribute is put into the DOM.

@nhunzaker
Copy link
Contributor Author

Added an extra test case for classname just to verify test coverage for case sensitive aliased attributes.

@gaearon
Copy link
Collaborator

gaearon commented Aug 5, 2017

What causes them to become lowercase in the first place? Is it per HTML spec or something?

@syranide
Copy link
Contributor

syranide commented Aug 6, 2017

Attribute names are case-insensitive in the HTML-standard and according to MDN are always lower-cased.
https://html.spec.whatwg.org/multipage/syntax.html#attributes-2
https://developer.mozilla.org/en/docs/Web/API/Element/setAttribute

So perhaps we should even warn when trying to emit them?

Also FYI, data-myAttribute isn't actually a "valid" name for a data-attribute due to the associated hyphenation-conversion (because everything is lowercase). dataset['myAttribute'] converts to <tag data-my-attribute>, so uppercase cannot be represented as attributes.

@gaearon
Copy link
Collaborator

gaearon commented Aug 6, 2017

So perhaps we should even warn when trying to emit them?

This sounds good to me, but does this means we can never get rid of SVG whitelist? Or is there some other solution that lets us enforce lowercase attributes except SVG?

@syranide
Copy link
Contributor

syranide commented Aug 6, 2017

@gaearon I'm not super read-up on all this, but it seems like standard HTML and the special namespaces obide by two different rule sets. HTML is a large collection of special-cases (self-closing void elements being one obvious) so I don't think you can ever be free of it all.

@nhunzaker nhunzaker changed the title Cased data attributes should probably diff on server hydration Cased data attributes should properly diff on server hydration Aug 6, 2017
@nhunzaker
Copy link
Contributor Author

Sorry for the delay. I wanted to figure a few things out between this PR and #7311

SVG reads case-sensitive, but it doesn't care about authoring case sensitivity. Using the following test: https://codepen.io/nhunzaker/pen/yogOXe?editors=1010

No matter what casing the attributes are written in, the browser transforms them into the correct casing no matter what. This is demonstrated by every browser I've tested:

  • IE9-11
  • IE Edge 14-15. Important: Edge reports fill, stroke, and stroke-dasharray as all caps, the attributes still work though. Weird.
  • Firefox 32-54. Couldn't go back further than 32.
  • Desktop Safari 5-10. Safari 4 doesn't appear to support inline SVG.
  • iOS Safari 5.1-10
  • Chrome 16-59. I went back in 5 version intervals to 40, then 10 down to 16. I can go more specific if you'd like
  • Chrome for Android 5-7
  • Samsung Android Browser 5-7
  • Opera 12-46. Below this does not support inline SVG
  • Yandex 14.12-17

I've tried to be as exhaustive as possible :).

  1. Every browser reports case sensitive SVG the way the are represented in the spec (IE: textLength)
  2. Every browser handles bad casing of SVG and HTML attributes
  3. All HTML attributes are downcased

Drawing from this:

  1. IE Edge reports SVG attribute names for stroke, fill, etc as all caps. This means we should probably downcase the attribute names when we fetch them out of the DOM for SSR diffing.
  2. Casing of attributes when authoring HTML and SVG does not matter. It is safe to allow any casing of attributes.

Specific to this issue, I think we should always compare attributes in a case insensitive way when diffing properties. Inconsistent casing should be a developer warning handled by React during the rendering process, not the diffing process.

@nhunzaker
Copy link
Contributor Author

Sorry, additionally, if we don't make the diffing process case-insensitive, we need to figure out a way to reconcile the all caps reporting issues in IE Edge.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 7, 2017

I pushed a commit that downcases the DOM attribute name reference (480e41f). I think this has to be manually tested to ensure IE Edge properly hydrates SSR SVG. I can work on that later today.

If we merge either #10397 or #10385, this PR can go away. Case-insensitive diffing is necessary for custom attributes with uppercase letters.

IE Edge reports some SVG attributes, like stroke and fill, in all
caps. This commit downcases all attribute names so that comparison
when diffing is case in-sensitive.
@nhunzaker
Copy link
Contributor Author

Assuming it sticks, the custom attribute pr overlapped with this one:
https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L886

Closing this out.

@nhunzaker nhunzaker closed this Aug 16, 2017
@gaearon gaearon deleted the cased-data-attributes branch October 6, 2017 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants