Skip to content

[Fiber] Fix rendering SVG into non-React SVG tree #8638

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

Merged
merged 4 commits into from
Jan 14, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 25, 2016

We used to determine the root tree namespace by the tag name alone in Fiber.

However this was broken for cases when we render an SVG tree into another SVG tree outside React.

This PR introduces a few new tests and fixes this by reading namespace from the root container, and using it in conjunction with the tag to determine child namespace.

Addresses https://github.com/facebook/react/pull/8586/files/414f1ec324bbce828c84900d66d202b5d776b6aa#r93161706.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 26, 2016

For some reason one of the new tests is failing for Stack in innerHTML mode.

@aweary
Copy link
Contributor

aweary commented Dec 27, 2016

For some reason one of the new tests is failing for Stack in innerHTML mode.

@gaearon looks like ajsdom bug to me. Here's a JSFiddle showing that it should return the HTML namespace when using innerHTML: https://jsfiddle.net/n3mqkqfv/

If you run the ~same code using jsdomyou'll see it returns the SVG namespace

var jsdom = require('jsdom').jsdom
var outerSVGRoot = document.createElementNS('http://www.w3.org/2000/svg', 'svg');
var container = document.createElementNS('http://www.w3.org/2000/svg', 'foreignObject');
outerSVGRoot.appendChild(container);
var markup = '<div react-reactroot></div>';
container.innerHTML = markup;
console.log(container.firstChild.namespaceURI);
// 'http://www.w3.org/2000/svg'

jsdom doesn't give HTML namespace to foreignObject.innerHTML children.
This problem doesn't exist in the browsers.

facebook#8638 (comment)

We will skip this test in non-createElement mode considering
that non-createElement mounting is effectively dead code now anyway.
@gaearon
Copy link
Collaborator Author

gaearon commented Dec 27, 2016

I think you're right. Just skipping this test now since it's dead code anyway. We only use non-createElement code path for server rendering, but server rendering doesn't have mounting. The innerHTML is only assigned in the mounting code path.

@gaearon gaearon merged commit 6a48891 into facebook:master Jan 14, 2017
@gaearon gaearon deleted the render-into-svg branch January 14, 2017 13:11
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