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

Fixes #9667: allow Edge and IE child windows to use Text Components #10720

Closed
wants to merge 0 commits into from

Conversation

kenotron
Copy link
Contributor

@kenotron kenotron commented Sep 15, 2017

Fixes #9667

Appending a child (insertBefore) with a documentFragment apparently isn't supported in IE / Edge. I've added code to simply skip this node and just let the tree walking algo add the nodes below.

@gaearon
Copy link
Collaborator

gaearon commented Sep 15, 2017

This code is not used on master (React 16 comes out very soon and won't use it). Can you check if the issue still occurs with 16 RC3?

@kenotron
Copy link
Contributor Author

Oh, things are even more busted in React 16 actually. That's bad :( I'm going to try to isolate where we can fix this. I think basically we need to be aware that we're about about to insert an entire documentfragment across documents. Where would THAT code be?

@kenotron
Copy link
Contributor Author

I'm not familiar enough with Fiber to get to the bottom of it all, but basically in
appendInitialChild, the parentInstance and the child are actually coming from different ownerDocuments if it's a child window. Somehow somewhere IE / Edge fell through the cracks and a child gets created inside the parent window document and the reconciler tries to append that child from parent to child window. Other browsers seem to handle this okay, but not in IE / Edge...

@kenotron
Copy link
Contributor Author

kenotron commented Sep 15, 2017

@gaearon a little assist?

@kenotron
Copy link
Contributor Author

Okay, I found a genuine bug inside createTextNode - it is always assuming to create a text node from inside window.document rather than a host instance's ownerDocument...

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.

3 participants