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.

Sign up for GitHub

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

Fix false positive hydration warning for SVG attributes #10676

Merged
merged 3 commits into from
Sep 13, 2017
Merged

Fix false positive hydration warning for SVG attributes #10676

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e900de
Fix false positive hydration warning for SVG attributes
gaearon Sep 11, 2017
ce09e18
Undo the generic fix
gaearon Sep 12, 2017
8c30d5b
Pass namespace through and use it to determine sensitivity
gaearon Sep 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ var ReactDOMFiberComponent = {
case 'selected':
break;
default:
extraAttributeNames.add(attributes[i].name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nhunzaker nhunzaker Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

screen shot 2017-09-11 at 6 47 29 pm

Screenshot of Chrome:

screen shot 2017-09-11 at 6 48 30 pm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nhunzaker nhunzaker Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

extraAttributeNames.add(name);
}
}
}
Expand Down
29 changes: 20 additions & 9 deletions src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,19 +1325,30 @@ describe('ReactDOMServerIntegration', () => {
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
});

itRenders('svg child element', async render => {
let e = await render(
<svg><image xlinkHref="http://i.imgur.com/w7GCRPb.png" /></svg>,
);
e = e.firstChild;
itRenders('svg child element with an attribute', async render => {
let e = await render(<svg viewBox="0 0 0 0" />);
expect(e.childNodes.length).toBe(0);
expect(e.tagName).toBe('image');
expect(e.tagName).toBe('svg');
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe(
'http://i.imgur.com/w7GCRPb.png',
);
expect(e.getAttribute('viewBox')).toBe('0 0 0 0');
});

itRenders(
'svg child element with a namespace attribute',
Copy link
Collaborator Author

@gaearon gaearon Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

async render => {
let e = await render(
<svg><image xlinkHref="http://i.imgur.com/w7GCRPb.png" /></svg>,
);
e = e.firstChild;
expect(e.childNodes.length).toBe(0);
expect(e.tagName).toBe('image');
expect(e.namespaceURI).toBe('http://www.w3.org/2000/svg');
expect(e.getAttributeNS('http://www.w3.org/1999/xlink', 'href')).toBe(
'http://i.imgur.com/w7GCRPb.png',
);
},
);

itRenders('svg child element with a badly cased alias', async render => {
let e = await render(
<svg><image xlinkhref="http://i.imgur.com/w7GCRPb.png" /></svg>,
Expand Down