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

Jump to bottom

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

Merged
merged 10 commits into from Sep 22, 2017
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
54e6f1d
Updated createTextInstance to create the text node off of the correct…
kenotron Sep 16, 2017
d610579
Adding a test to make sure the ownerDocument is consistent with host
kenotron Sep 16, 2017
716dcd1
Make sure to test everything rather than just one test in the ReactDO…
kenotron Sep 16, 2017
cf0ac10
Prettier on added ReactDOMFiber-test
kenotron Sep 16, 2017
5509e8c
Updated to use similar code to retrieve ownerDocument from rootContai…
kenotron Sep 18, 2017
1ffced7
Updated the test to not use jsdom. Uses iframe to obtain new document…
kenotron Sep 20, 2017
c66397c
Adding a positive test along with that negative test for the actualDo…
kenotron Sep 20, 2017
459e2ab
lint fix
kenotron Sep 20, 2017
205e561
Addressing PR nits and refactoring the code to be more symmetrical
kenotron Sep 21, 2017
4b19c77
addressing PR feedback: remove unused line; add return typing for
kenotron Sep 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Expand Up @@ -157,6 +157,14 @@ function ensureListeningTo(rootContainerElement, registrationName) {
listenTo(registrationName, doc);
}

function getOwnerDocumentFromRootContainer(
rootContainerElement: Element | Document,
) {
return rootContainerElement.nodeType === DOCUMENT_NODE
? (rootContainerElement: any)
: rootContainerElement.ownerDocument;
}

// There are so many media events, it makes sense to just
// maintain a list rather than create a `trapBubbledEvent` for each
var mediaEvents = {
Expand Down Expand Up @@ -296,10 +304,9 @@ var ReactDOMFiberComponent = {
): Element {
// We create tags in the namespace of their parent container, except HTML
// tags get no namespace.
var ownerDocument: Document = rootContainerElement.nodeType ===
DOCUMENT_NODE
? (rootContainerElement: any)
: rootContainerElement.ownerDocument;
var ownerDocument: Document = getOwnerDocumentFromRootContainer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

rootContainerElement,
);
var domElement: Element;
var namespaceURI = parentNamespace;
if (namespaceURI === HTML_NAMESPACE) {
Expand Down Expand Up @@ -362,6 +369,12 @@ var ReactDOMFiberComponent = {
return domElement;
},

createTextNode(text: string, rootContainerElement: Element | Document): Text {
return getOwnerDocumentFromRootContainer(
rootContainerElement,
).createTextNode(text);
},

setInitialProperties(
domElement: Element,
tag: string,
Expand Down
3 changes: 2 additions & 1 deletion src/renderers/dom/fiber/ReactDOMFiberEntry.js
View file
Open in desktop
Expand Up @@ -46,6 +46,7 @@ var invariant = require('fbjs/lib/invariant');
var {getChildNamespace} = DOMNamespaces;
var {
createElement,
createTextNode,
setInitialProperties,
diffProperties,
updateProperties,
Expand Down Expand Up @@ -368,7 +369,7 @@ var DOMRenderer = ReactFiberReconciler({
const hostContextDev = ((hostContext: any): HostContextDev);
validateDOMNesting(null, text, null, hostContextDev.ancestorInfo);
}
var textNode: TextInstance = document.createTextNode(text);
var textNode: TextInstance = createTextNode(text, rootContainerInstance);
precacheFiberNode(internalInstanceHandle, textNode);
return textNode;
},
Expand Down
30 changes: 30 additions & 0 deletions src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
View file
Open in desktop
Expand Up @@ -1117,5 +1117,35 @@ describe('ReactDOMFiber', () => {
'to empty a container.',
);
});

it('should render a text component with a text DOM node on the same document as the container', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// 1. Create a new document through the use of iframe
// 2. Set up the spy to make asserts when a text component
// is rendered inside the iframe container
var textContent = 'Hello 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();
var iframeContainer = iframeDocument.body.firstChild;

var actualDocument;
var textNode;
iframeContainer.appendChildBackup = iframeContainer.appendChild;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - goober from a previous attempt at it.


spyOn(iframeContainer, 'appendChild').and.callFake(node => {
actualDocument = node.ownerDocument;
textNode = node;
});

ReactDOM.render(textContent, iframeContainer);

expect(textNode.textContent).toBe(textContent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kenotron kenotron Sep 21, 2017 •
edited

Choose a reason for hiding this comment

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

@sophiebits I've added this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sophiebits ping?

expect(actualDocument).not.toBe(document);
expect(actualDocument).toBe(iframeDocument);
});
}
});