Fix false positive hydration warning for SVG attributes #10676

gaearon · 2017-09-11T19:18:00Z

Fixes #10672.
The new test used to fail but now passes.

gaearon · 2017-09-11T19:18:26Z

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

      });

+      itRenders(
+        'svg child element with a namespace attribute',


This is an old test. I just renamed it and moved down.

gaearon · 2017-09-11T19:19:47Z

@nhunzaker Are there any other cases where this could happen? Do you remember why it didn't use name in the first place? Should I worry about this not being lowercase?

nhunzaker · 2017-09-11T21:29:52Z

I do not know why name was not used. I think it is bug. I think we also want to lowercase the spot you've linked because SVG attribute names have casing.

sebmarkbage · 2017-09-11T21:46:40Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

@@ -912,7 +912,7 @@ var ReactDOMFiberComponent = {
          case 'selected':
            break;
          default:
-            extraAttributeNames.add(attributes[i].name);


This was intentional so that you would compare and display it in the case that it was received. For things like SVG where case matters.

I don't think this is right because on SVG we do want things like <svg foobar="" /> vs. <svg fooBar="" /> to show up as an extra attribute.

It might make sense to deal with this at the same time as the "all attributes should be lower-case" warning though.

I am not sure that we can, unless we can control for browsers that report casing from attribute.name inconsistently. For example, IE Edge reports SVG fill and `stroke in all caps:

Test: https://codepen.io/nhunzaker/pen/qXRPvY

Screenshot of IE Edge 15:

Screenshot of Chrome:

@nhunzaker Can we special case the known ones since they will also have the warning about invalid case of the prop?

I can start to compile a list of special cases. I'm sort of worried that this will be tedious and it'll end up being most of SVG, but I don't know that yet. I know of differences in casing of SVG in IE Edge. I didn't know about the cases alluded to in Chrome.

Should I just open up the attribute table in every browser using BrowserStack? How many versions back should I go?

Kind of makes me wonder if it could be automated using their remote API.

I have a few questions, and I apologize in advance if they sound cynical, but I don't want to assume anything so I can better understand the problem:

1: What is the advantage to case-sensitive hydration? Casing warnings for known attributes will raise server-side during markup generation (is this correct?), and then and on update client-side.

2: Wouldn't the same components consistently misspell attribute names server and client side?

3: Even if the casing is produced differently server/client-side, browsers don't appear to care about casing. I'm curious if this is generally true, or if my tests are flawed. They certainly aren't exhaustive.

My concern is that just ignoring case doesn't take the core of the problem in to consideration because it's not the case that browsers completely ignore case. There are specific rules for it.

Specifically, the rules (other than bugs/quirks like the IE thing) is that the HTML parser of a server response makes them lower-case except for a whitelist. https://www.w3.org/TR/html5/syntax.html#adjust-svg-attributes

However, this whitelist doesn't apply to client code. Specifically cases like this is what I'm concerned about:

var svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); svg.setAttribute('viewbox', '0 0 1 1'); console.log(svg.viewBox); var container = document.createElement('div'); container.innerHTML = '<svg viewbox="0 0 1 1"></svg>'; console.log(container.firstChild.viewBox);

Rendering the "viewbox" attribute on the server and the client behaves differently because it is case-sensitive on the client but not on the server.

So simple saying that they're not case-sensitive is not the full story. It might be the case that this is the best compromise anyway but I want to make sure we've considered all the aspects because "DOM is case-insensitive" is an oversimplification so we might be missing something.

Ah got it. So there are attributes that care about casing. That's frustrating...

Thank you for spelling this out for me. Prior to this point, I was not aware of the differences between HTML/SVG parsing and the DOM attribute manipulation. What a mess!

sebmarkbage · 2017-09-11T21:52:07Z

FYI, this is tangental to this PR but there are a few attributes in the attribute tables that correctly warns because they're buggy in Chrome. Chrome doesn't turn them into camelcase like it should. There are also some that are incorrectly left out of the HTML parsing spec. If we get a report for that, be careful to look into it.

sebmarkbage · 2017-09-11T21:55:01Z

Oh, and if you render viewbox on the server it will be automatically turned into viewBox during hydration, but if you render it on the client that will not work because setAttribute is case sensitive.

gaearon · 2017-09-11T22:04:07Z

Hmm. Is new test I added wrong then? It used to fail before this PR. But it uses correct casing (viewBox) and is a common attribute.

sebmarkbage · 2017-09-11T22:15:18Z

I think your test is correct because it is a known one. However, we have to be careful when we look at these things because if it wasn't known then we'd warn that you should use lower case (if we add the lowercase warning). That would work with SSR and hydration but not if it was only rendered on the client. E.g. during an update.

sebmarkbage

I think the real bug is at line 1011. https://github.com/facebook/react/pull/10676/files#diff-9756d35a2f408667aa5e1cfb065b4fdcR1011

It should do toLowerCase for HTML, but not for SVG and it probably needs to be special case aware.

gaearon · 2017-09-12T18:31:45Z

I started looking into it but didn’t get far. It will take me a while to understand the constraints here again. If either of you have better insight into the best fix feel free to take this; otherwise I will continue tomorrow.

I could also apply the fix in #10676 (review) but I don’t know which special cases the fix should be aware of.

gaearon · 2017-09-12T19:06:29Z

Pushed up a version that threads the parent namespace through for the check. Could also read it from the element. Is it enough for the most narrow fix to unblock this? We can fix edge cases with IE later.

nhunzaker · 2017-09-12T20:21:33Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

-            // $FlowFixMe - Should be inferred as not undefined.
-            extraAttributeNames.delete(propKey.toLowerCase());
+            let ownNamespace = parentNamespace;
+            if (ownNamespace === HTML_NAMESPACE) {


Why rename parentNamespace to ownNamespace here instead of using parentNamespace directly or setting ownNamespace as the argument?

Disregard. Sorry I didn't catch the code below.

nhunzaker · 2017-09-12T20:24:38Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+            }
+            if (ownNamespace === HTML_NAMESPACE) {
+              // $FlowFixMe - Should be inferred as not undefined.
+              extraAttributeNames.delete(propKey.toLowerCase());


I wonder if it's worth adding a comment for why we need to downcase the propKey (because HTML reports lowercase, right?)

nhunzaker

This makes sense to me, the only edge case I can think of is if the casing reporting behavior is inconsistent across SVG attribute names.

I would be okay with making that a follow up to this PR.

sebmarkbage

Seems fine but I think we'll also need a follow up (which may very well be something case insensitive for unknown but case sensitive for known or something like that).

sebmarkbage · 2017-09-13T15:25:57Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+            } else {
+              // $FlowFixMe - Should be inferred as not undefined.
+              extraAttributeNames.delete(propKey);
+            }


This made me realize that we probably don't validate that two components with the same name but different namespaces line up in the hydration. I'm not sure there's a way to get that wrong in another way than innerHTML SVG content in a HTML root.

gaearon · 2017-09-13T15:32:19Z

Would that follow up be blocking to 16 release?

Fix false positive hydration warning for SVG attributes

4e900de

facebook-github-bot added the CLA Signed label Sep 11, 2017

gaearon requested review from sebmarkbage and nhunzaker September 11, 2017 19:18

gaearon commented Sep 11, 2017

View reviewed changes

gaearon added this to the 16.0 milestone Sep 11, 2017

gaearon mentioned this pull request Sep 11, 2017

Wrong SSR diff warning due to non lowercase attributes #10672

Closed

sebmarkbage reviewed Sep 11, 2017

View reviewed changes

sebmarkbage requested changes Sep 11, 2017

View reviewed changes

gaearon added 2 commits September 12, 2017 19:47

Undo the generic fix

ce09e18

Pass namespace through and use it to determine sensitivity

8c30d5b

nhunzaker reviewed Sep 12, 2017

View reviewed changes

nhunzaker approved these changes Sep 12, 2017

View reviewed changes

sebmarkbage approved these changes Sep 13, 2017

View reviewed changes

gaearon merged commit c55ffb3 into facebook:master Sep 13, 2017

gaearon deleted the fix-ssr-war branch September 13, 2017 15:31

bvaughn mentioned this pull request Sep 14, 2017

React 16 RC #10294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false positive hydration warning for SVG attributes #10676

Fix false positive hydration warning for SVG attributes #10676

gaearon commented Sep 11, 2017

gaearon Sep 11, 2017 •

edited

gaearon commented Sep 11, 2017

nhunzaker commented Sep 11, 2017

sebmarkbage Sep 11, 2017

sebmarkbage Sep 11, 2017 •

edited

nhunzaker Sep 11, 2017 •

edited

sebmarkbage Sep 11, 2017

nhunzaker Sep 12, 2017 •

edited

sebmarkbage Sep 12, 2017 •

edited

nhunzaker Sep 12, 2017

sebmarkbage commented Sep 11, 2017

sebmarkbage commented Sep 11, 2017 •

edited

gaearon commented Sep 11, 2017

sebmarkbage commented Sep 11, 2017

sebmarkbage left a comment

gaearon commented Sep 12, 2017

gaearon commented Sep 12, 2017

nhunzaker Sep 12, 2017 •

edited

nhunzaker Sep 12, 2017

nhunzaker Sep 12, 2017

nhunzaker left a comment

sebmarkbage left a comment

sebmarkbage Sep 13, 2017

gaearon commented Sep 13, 2017

Navigation Menu

Fix false positive hydration warning for SVG attributes #10676

Fix false positive hydration warning for SVG attributes #10676

Conversation

gaearon commented Sep 11, 2017

gaearon Sep 11, 2017 • edited

Choose a reason for hiding this comment

gaearon commented Sep 11, 2017

nhunzaker commented Sep 11, 2017

sebmarkbage Sep 11, 2017

Choose a reason for hiding this comment

sebmarkbage Sep 11, 2017 • edited

Choose a reason for hiding this comment

nhunzaker Sep 11, 2017 • edited

Choose a reason for hiding this comment

sebmarkbage Sep 11, 2017

Choose a reason for hiding this comment

nhunzaker Sep 12, 2017 • edited

Choose a reason for hiding this comment

sebmarkbage Sep 12, 2017 • edited

Choose a reason for hiding this comment

nhunzaker Sep 12, 2017

Choose a reason for hiding this comment

sebmarkbage commented Sep 11, 2017

sebmarkbage commented Sep 11, 2017 • edited

gaearon commented Sep 11, 2017

sebmarkbage commented Sep 11, 2017

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Sep 12, 2017

gaearon commented Sep 12, 2017

nhunzaker Sep 12, 2017 • edited

Choose a reason for hiding this comment

nhunzaker Sep 12, 2017

Choose a reason for hiding this comment

nhunzaker Sep 12, 2017

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

sebmarkbage Sep 13, 2017

Choose a reason for hiding this comment

gaearon commented Sep 13, 2017

gaearon Sep 11, 2017 •

edited

sebmarkbage Sep 11, 2017 •

edited

nhunzaker Sep 11, 2017 •

edited

nhunzaker Sep 12, 2017 •

edited

sebmarkbage Sep 12, 2017 •

edited

sebmarkbage commented Sep 11, 2017 •

edited

nhunzaker Sep 12, 2017 •

edited