Fixes #9667: Updated createTextInstance to create the text node on correct document #10723

kenotron · 2017-09-16T17:43:17Z

Second try: this time update the Fiber reconciler to create the text node on the correct ownerDocument so that when it is being appendChild()'ed / insertBefore()'ed, it will succeed on IE / Edge.

… ownerDocument

kenotron · 2017-09-16T17:44:08Z

@gaearon Alright, second try! I think this fixes up the issue on Edge / IE...

kenotron · 2017-09-16T17:46:31Z

Related to #9667

…MFiber-test.js

kenotron · 2017-09-16T19:52:13Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -16,6 +16,7 @@ var ReactDOM = require('react-dom');
 var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
 var ReactTestUtils = require('react-dom/test-utils');
 var PropTypes = require('prop-types');
+var JSDom = require('jsdom');


jsdom is needed here to create a brand new document because by default jest-jsdom assumes every test is on the same document.

kenotron · 2017-09-16T19:52:32Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -1117,5 +1118,22 @@ describe('ReactDOMFiber', () => {
          'to empty a container.',
      );
    });
+
+    it('should render a text component with a text DOM node on the same document as the container', () => {


I've verified that this test case fails in the current master.

kenotron · 2017-09-16T22:27:56Z

@gaearon all green!

syranide · 2017-09-18T08:53:36Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

-    var textNode: TextInstance = document.createTextNode(text);
+
+    var doc = rootContainerInstance;
+    if (rootContainerInstance.nodeType !== DOCUMENT_NODE && doc.ownerDocument) {


Can rootContainerInstance even be a document node?

line 101: type Container = Element | Document;

I think so.

syranide · 2017-09-18T08:54:09Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

-    var textNode: TextInstance = document.createTextNode(text);
+
+    var doc = rootContainerInstance;
+    if (rootContainerInstance.nodeType !== DOCUMENT_NODE && doc.ownerDocument) {


Why test for doc.ownerDocument? I don't know why it would ever be falsy and if it is then your code would break anyway, you would be trying to call createTextNode on some random element.

We don't have to, I guess from the same flow typing info, it can only be an Element (which HAS ownerDocument) or it's a Document which is the document itself.

syranide · 2017-09-18T08:54:25Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

-    var textNode: TextInstance = document.createTextNode(text);
+
+    var doc = rootContainerInstance;
+    if (rootContainerInstance.nodeType !== DOCUMENT_NODE && doc.ownerDocument) {


PS. rootContainerInstance should be changed to doc for consistency.

That's a good call out, but I'm actually removing that ownerDocument and changing the name for short.

syranide · 2017-09-18T08:56:52Z

src/renderers/dom/fiber/ReactDOMFiberEntry.js

-    var textNode: TextInstance = document.createTextNode(text);
+
+    var doc = rootContainerInstance;
+    if (rootContainerInstance.nodeType !== DOCUMENT_NODE && doc.ownerDocument) {


Also, is it really guaranteed that rootContainerInstance belongs to the same document you're trying to render the TextNode into? I would think that rendering an iframe and into it would break this assumption... or has this changed?

From the way it WAS written before, Fiber would have broken the iframe case. If you looked at the ReactDOMFiberComponent's createElement(), you'd see that the semantics there for retrieving "ownerDocument" is the same as what I have written here:

var ownerDocument: Document = rootContainerElement.nodeType === DOCUMENT_NODE ? (rootContainerElement: any) : rootContainerElement.ownerDocument;

I'm going to reuse this bit of code for clarity inside fiberentry

…nerInstance

gaearon

I think this looks good and consistent with what we do in ReactDOMFiberComponent.createElement. Maybe we can even get it into 16.

gaearon · 2017-09-18T23:07:04Z

@kenotron Is there a way to rewrite the test to not use jsdom APIs directly? Maybe there’s some cases with an iframe that would reproduce it?

sophiebits

Can we make a helper function ReactDOMFiberComponent.createTextNode like how createElement works? I think the parallel structure would be clearer.

cc @acdlite for conflicts with #10624

acdlite · 2017-09-19T00:08:54Z

Conflicts are fine, I've already accepted I'll have to rebase/rewrite a lot of my work it before it lands :D

kenotron · 2017-09-19T03:09:38Z

@gaearon I have attempted to do this with iframe, but it didn't fail in master. I don't think jsdom truly emulates the edge case behavior of the browser in this case. I have to force the behavior via separate documents as coded.

sophiebits · 2017-09-19T04:49:57Z

I think you can copy what src/renderers/dom/shared/eventPlugins/tests/EnterLeaveEventPlugin-test.js does.

kenotron · 2017-09-19T16:40:51Z

@sophiebits I've tried this route:

it('should render a text component with a text DOM node on the same document as the container', () => {
      // Arrange
      var textContent = ' world ';

      var iframe = document.createElement('iframe');
      document.body.appendChild(iframe);

      var iframeDocument = iframe.contentDocument;

      iframeDocument.write(
        '<!DOCTYPE html><html><head></head><body><div></div></body></html>',
      );
      iframeDocument.close();

      // Act
      ReactDOM.render(
        <div><span id="pre">hello</span>{textContent}<span>again</span></div>,
        iframeDocument.body.getElementsByTagName('div')[0]
      );

      // Assert
      var textNode = iframeDocument.getElementById('pre').nextSibling;

      expect(textNode.textContent).toBe(textContent);
      expect(textNode.ownerDocument).toBe(iframeDocument);
      expect(textNode.ownerDocument).not.toBe(document);
    });

But I noticed that when I ran this test, it didn't fail for react master. It can only mean that jsdom is doing the "right thing" like many all non-MS browsers (sigh!!). So, to simulate the effect of actually unrelated documents not able to appendNode() / insertBefore() from another (related) document, I had to drop down to the level of jsdom directly creating an unrelated document.

sophiebits · 2017-09-19T20:06:46Z

Can you do ReactDOM.render(text, container) and mock out container.appendChild and assert on its argument's .ownerDocument?

…. :the grinder rests:

…cument

gaearon · 2017-09-20T11:40:51Z

Thanks for the updated test. Do you mind also addressing this comment from @sophiebits?

Can we make a helper function ReactDOMFiberComponent.createTextNode like how createElement works? I think the parallel structure would be clearer.

gaearon · 2017-09-20T11:41:56Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+        textNode = node;
+      });
+
+      // Act


Nit: we can remove Arrange/Act/Assert comments as we don't use them in other tests.

I've also addressed the parallel structure comment from @sophiebits. Since now we have repeated code to do that same thing, I've also factored that out into a function.

sophiebits

Looks great, just a few more small comments and then I think we can merge this.

sophiebits · 2017-09-21T04:52:35Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

-      DOCUMENT_NODE
-      ? (rootContainerElement: any)
-      : rootContainerElement.ownerDocument;
+    var ownerDocument: Document = getOwnerDocumentFromRootContainer(


Can you put the annotation on the function getOwnerDocumentFromRootContainer's definition? It's necessary to "cancel out" the : any cast.

would I still need this Document type? I think flow should be able to infer, right?

Right, we don't need this one any more.

sophiebits · 2017-09-21T04:53:25Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+
+      var actualDocument;
+      var textNode;
+      iframeContainer.appendChildBackup = iframeContainer.appendChild;


nit: remove

good catch - goober from a previous attempt at it.

sophiebits · 2017-09-21T04:54:19Z

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

+
+      ReactDOM.render(textContent, iframeContainer);
+
+      expect(textNode.textContent).toBe(textContent);


Can we add an assertion on the number of calls for iframeContainer.appendChild here?

@sophiebits I've added this check.

@sophiebits ping?

sophiebits

This looks good, thanks!

sophiebits · 2017-09-22T23:06:59Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

-      DOCUMENT_NODE
-      ? (rootContainerElement: any)
-      : rootContainerElement.ownerDocument;
+    var ownerDocument: Document = getOwnerDocumentFromRootContainer(


Right, we don't need this one any more.

sophiebits · 2017-09-22T23:08:24Z

Thanks!

kenotron · 2017-09-22T23:16:02Z

YAY!

@gaearon

* Update changelog for unreleased 16.0 changes (#10730) * First shot at updating changelog for 16.0 **what is the change?:** Added an 'unreleased' section to the changelog with info from #10294 **why make this change?:** To get things set for the 16.0 release. **test plan:** Visual inspection **issue:** #8854 * Fix typos and formatting errors in changelog * Add requestAnimationFrame and remove "New helpful warnings" **what is the change?:** In response to helpful code review - - Add mention of dependency on `requestAnimationFrame` and need to polyfill that as well as `Map` and `Set` - Remove "New helpful warnings" section; it was incomplete, and there are so many new and updated warnings that it might not be reasonable to cover them in the changelog. **why make this change?:** Accuracy **test plan:** Visual inspection **issue:** issue #8854 * Improve wording * Improve wording and fix missing links * Add backticks to file names & code; wording tweak * Break "Major Changes" into "New Feature" and "Breaking Changes" * Add server side render changes to 16.0 changelog * Change gist link from mine to gaearons * Add note about returning fragments * fix misc nits * Misc. formatting/wording fixes to changelog **what is the change?:** Thanks to the kind code review comments of @gaearon and @nhunzaker we have - removed the non-deterministic bold styling on some bullet points - improved wording of the bullet points for portals, attribute whitelist changes, and server rendering changes - Add note about error boundaries including a breaking change to error handling behavior. - punctuation and capitalization fixes **why make this change?:** Clarity and correctness **test plan:** Visual inspection **issue:** #8854 * fix broken link * Fixes #9667: Updated createTextInstance to create the text node on correct document (#10723) * Record sizes * Add a changelog for elements having the same key (#10811) * Add a changelog for elements having the same key * Reword * Markdown fixs on "DOM Attributes in React 16" post (#10816) * Include tag name into the table snapshot (#10818) * Update DOM warning wording and link (#10819) * Update DOM warning wording and link * Consistently use "Invalid" for known misspellings * Update license headers BSD+Patents -> MIT Did find and replace in TextMate. ``` find: (?:( \*)( ))?Copyright (?:$c$ )?(\d{4})\b.+Facebook[\s\S]+(?:this source tree|the same directory)\.$ replace: $1$2Copyright (c) $3-present, Facebook, Inc.\n$1\n$1$2This source code is licensed under the MIT license found in the\n$1$2LICENSE file in the root directory of this source tree. ``` * Change license and remove references to PATENTS Only remaining references: ``` docs/_posts/2014-10-28-react-v0.12.md 51:You can read the full text of the [LICENSE](https://github.com/facebook/react/blob/master/LICENSE) and [`PATENTS`](https://github.com/facebook/react/blob/master/PATENTS) files on GitHub. docs/_posts/2015-04-17-react-native-v0.4.md 20:* **Patent Grant**: Many of you had concerns and questions around the PATENTS file. We pushed [a new version of the grant](https://code.facebook.com/posts/1639473982937255/updating-our-open-source-patent-grant/). src/__mocks__/vendor/third_party/WebComponents.js 8: * subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt ``` * Version bumps to use MIT license * Add ReactTestRenderer documentations (#10692) * Add ReactTestRenderer documentations * Add TestRenderer documentations * TestRenderer is not experiment * Add a link for jsdom * Use ES Modules syntax * Twaek * Add a Link component * Use Jest assertions * Move a documentation for createNodeMock to Idea section * Renamed * Tweak * Rename * More explicit * Add a usecase for createNodeMock * Add changelog for 15.6.2 * Add 15.6.2 blog post to master * Add Nate to authors on master * Bump object-assign patch range to match main package.json * Flow should ignore node_modules/create-react-class * Update error codes * Update CHANGELOG for React 16 * v16.0.0 * Doc updates for React 16 + blog post (#10824) * Doc updates for React 16 + blog post * Add link to Sophie's post * Fix React links on the website (#10837) * Fix React links on the website * Fix code editor * Fix code editor, attempt 2 * Doc change for prevContext removal in CDU (#10836) * Doc change for prevContext removal in CDU Ref: #8631 * Minor rewording * Fix note formatting * React.createPortal is not a function (#10843) * Update Portals Documentation (#10840) * Update Portals Documentation Correct some grammar to be more explicit and clear. Update example CodePen to better match code found in documentation. Update code example styles to match other code examples (ie. 'State and Lifecycle', 'Handling Events'). * Clean up comment to be accurate to example There was a small comment overlooked when reviewing the documentation. This fixes it to be accurate to the example as well as grammatically correct. * Update portals.md * More fixes * Update name of property initializer proposal (#10812) The proposal for property initializers is called [Public Class Fields](https://tc39.github.io/proposal-class-public-fields/) now (part of the combined [Class Fields](https://github.com/tc39/proposal-class-fields) proposal). * Fix portal link (#10845) * Update docs for React 16 (#10846) * Minor doc edit * Rename urls

Updated createTextInstance to create the text node off of the correct…

54e6f1d

… ownerDocument

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

kenotron added 2 commits September 16, 2017 12:47

Adding a test to make sure the ownerDocument is consistent with host

d610579

Make sure to test everything rather than just one test in the ReactDO…

716dcd1

…MFiber-test.js

kenotron commented Sep 16, 2017

View reviewed changes

Prettier on added ReactDOMFiber-test

cf0ac10

syranide reviewed Sep 18, 2017

View reviewed changes

Updated to use similar code to retrieve ownerDocument from rootContai…

5509e8c

…nerInstance

gaearon approved these changes Sep 18, 2017

View reviewed changes

sophiebits approved these changes Sep 18, 2017

View reviewed changes

kenotron added 3 commits September 19, 2017 22:19

Updated the test to not use jsdom. Uses iframe to obtain new document…

1ffced7

…. :the grinder rests:

Adding a positive test along with that negative test for the actualDo…

c66397c

…cument

lint fix

459e2ab

gaearon reviewed Sep 20, 2017

View reviewed changes

Addressing PR nits and refactoring the code to be more symmetrical

205e561

sophiebits requested changes Sep 21, 2017

View reviewed changes

addressing PR feedback: remove unused line; add return typing for

4b19c77

sophiebits approved these changes Sep 22, 2017

View reviewed changes

sophiebits merged commit a160f3e into facebook:master Sep 22, 2017


		ReactDOM.render(textContent, iframeContainer);

		expect(textNode.textContent).toBe(textContent);

Fixes #9667: Updated createTextInstance to create the text node on correct document #10723

Fixes #9667: Updated createTextInstance to create the text node on correct document #10723

Conversation

kenotron commented Sep 16, 2017

kenotron commented Sep 16, 2017

kenotron commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenotron commented Sep 16, 2017

syranide Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syranide Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Sep 18, 2017

sophiebits left a comment

Choose a reason for hiding this comment

acdlite commented Sep 19, 2017

kenotron commented Sep 19, 2017 • edited Loading

sophiebits commented Sep 19, 2017

kenotron commented Sep 19, 2017

sophiebits commented Sep 19, 2017

gaearon commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenotron Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented Sep 22, 2017

kenotron commented Sep 22, 2017

syranide Sep 18, 2017 •

edited

Loading

syranide Sep 18, 2017 •

edited

Loading

syranide Sep 18, 2017 •

edited

Loading

syranide Sep 18, 2017 •

edited

Loading

kenotron commented Sep 19, 2017 •

edited

Loading

kenotron Sep 21, 2017 •

edited

Loading