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

Don't wrap text in <span> elements #5753

Merged
merged 1 commit into from
Feb 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 6 additions & 4 deletions src/renderers/dom/client/ReactDOMComponentTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,12 @@ function precacheChildNodes(inst, node) {
}
// We assume the child nodes are in the same order as the child instances.
for (; childNode !== null; childNode = childNode.nextSibling) {
if (childNode.nodeType === 1 &&
childNode.getAttribute(ATTR_NAME) === String(childID) ||
childNode.nodeType === 8 &&
childNode.nodeValue === ' react-empty: ' + childID + ' ') {
if ((childNode.nodeType === 1 &&
childNode.getAttribute(ATTR_NAME) === String(childID)) ||
(childNode.nodeType === 8 &&
childNode.nodeValue === ' react-text: ' + childID + ' ') ||
(childNode.nodeType === 8 &&
childNode.nodeValue === ' react-empty: ' + childID + ' ')) {
precacheNode(childInst, childNode);
continue outer;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ describe('ReactDOMComponentTree', function() {
expect(renderAndGetInstance('main')._currentElement.type).toBe('main');

// This one's a text component!
expect(renderAndGetInstance('span')._stringText).toBe('goodbye.');
var root = renderAndQuery(null);
var inst = ReactDOMComponentTree.getInstanceFromNode(root.children[0].childNodes[2]);
expect(inst._stringText).toBe('goodbye.');

expect(renderAndGetClosest('b')._currentElement.type).toBe('main');
expect(renderAndGetClosest('img')._currentElement.type).toBe('main');
Expand Down
84 changes: 82 additions & 2 deletions src/renderers/dom/client/utils/DOMChildrenOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ var setInnerHTML = require('setInnerHTML');
var setTextContent = require('setTextContent');

function getNodeAfter(parentNode, node) {
// Special case for text components, which return [open, close] comments
// from getNativeNode.
if (Array.isArray(node)) {
node = node[1];
}
return node ? node.nextSibling : parentNode.firstChild;
}

Expand Down Expand Up @@ -51,6 +56,78 @@ function insertLazyTreeChildAt(parentNode, childTree, referenceNode) {
DOMLazyTree.insertTreeBefore(parentNode, childTree, referenceNode);
}

function moveChild(parentNode, childNode, referenceNode) {
if (Array.isArray(childNode)) {
moveDelimitedText(parentNode, childNode[0], childNode[1], referenceNode);
} else {
insertChildAt(parentNode, childNode, referenceNode);
}
}

function removeChild(parentNode, childNode) {
if (Array.isArray(childNode)) {
var closingComment = childNode[1];
childNode = childNode[0];
removeDelimitedText(parentNode, childNode, closingComment);
parentNode.removeChild(closingComment);
}
parentNode.removeChild(childNode);
}

function moveDelimitedText(
parentNode,
openingComment,
closingComment,
referenceNode
) {
var node = openingComment;
while (true) {
var nextNode = node.nextSibling;
insertChildAt(parentNode, node, referenceNode);
if (node === closingComment) {
break;
}
node = nextNode;
}
}

function removeDelimitedText(parentNode, startNode, closingComment) {
while (true) {
var node = startNode.nextSibling;
if (node === closingComment) {
// The closing comment is removed by ReactMultiChild.
break;
} else {
parentNode.removeChild(node);
}
}
}

function replaceDelimitedText(openingComment, closingComment, stringText) {
var parentNode = openingComment.parentNode;
var nodeAfterComment = openingComment.nextSibling;
if (nodeAfterComment === closingComment) {
// There are no text nodes between the opening and closing comments; insert
// a new one if stringText isn't empty.
if (stringText) {
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 made it so that if it's an empty string it doesn't insert an empty text node between the comments. Not really sure which is preferable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having no text node seems reasonable.

insertChildAt(
parentNode,
document.createTextNode(stringText),
nodeAfterComment
);
}
} else {
if (stringText) {
// Set the text content of the first node after the opening comment, and
// remove all following nodes up until the closing comment.
setTextContent(nodeAfterComment, stringText);
removeDelimitedText(parentNode, nodeAfterComment, closingComment);
} else {
removeDelimitedText(parentNode, openingComment, closingComment);
}
}
}

/**
* Operations for updating with DOM children.
*/
Expand All @@ -60,6 +137,8 @@ var DOMChildrenOperations = {

updateTextContent: setTextContent,

replaceDelimitedText: replaceDelimitedText,

/**
* Updates a component's children by processing a series of updates. The
* update configurations are each expected to have a `parentNode` property.
Expand All @@ -79,7 +158,7 @@ var DOMChildrenOperations = {
);
break;
case ReactMultiChildUpdateTypes.MOVE_EXISTING:
insertChildAt(
moveChild(
parentNode,
update.fromNode,
getNodeAfter(parentNode, update.afterNode)
Expand All @@ -98,7 +177,7 @@ var DOMChildrenOperations = {
);
break;
case ReactMultiChildUpdateTypes.REMOVE_NODE:
parentNode.removeChild(update.fromNode);
removeChild(parentNode, update.fromNode);
break;
}
}
Expand All @@ -108,6 +187,7 @@ var DOMChildrenOperations = {

ReactPerf.measureMethods(DOMChildrenOperations, 'DOMChildrenOperations', {
updateTextContent: 'updateTextContent',
replaceDelimitedText: 'replaceDelimitedText',
});

module.exports = DOMChildrenOperations;
13 changes: 9 additions & 4 deletions src/renderers/dom/client/validateDOMNesting.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,11 @@ if (__DEV__) {
}
didWarn[warnKey] = true;

var tagDisplayName = childTag;
if (childTag !== '#text') {
tagDisplayName = '<' + childTag + '>';
}

if (invalidParent) {
var info = '';
if (ancestorTag === 'table' && childTag === 'tr') {
Expand All @@ -393,19 +398,19 @@ if (__DEV__) {
}
warning(
false,
'validateDOMNesting(...): <%s> cannot appear as a child of <%s>. ' +
'validateDOMNesting(...): %s cannot appear as a child of <%s>. ' +
'See %s.%s',
childTag,
tagDisplayName,
ancestorTag,
ownerInfo,
info
);
} else {
warning(
false,
'validateDOMNesting(...): <%s> cannot appear as a descendant of ' +
'validateDOMNesting(...): %s cannot appear as a descendant of ' +
'<%s>. See %s.',
childTag,
tagDisplayName,
ancestorTag,
ownerInfo
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ describe('ReactServerRendering', function() {
ID_ATTRIBUTE_NAME + '="[^"]+" ' +
ReactMarkupChecksum.CHECKSUM_ATTR_NAME + '="[^"]+">' +
'<span ' + ID_ATTRIBUTE_NAME + '="[^"]+">' +
'<span ' + ID_ATTRIBUTE_NAME + '="[^"]+">My name is </span>' +
'<span ' + ID_ATTRIBUTE_NAME + '="[^"]+">child</span>' +
'<!-- react-text: [0-9]+ -->My name is <!-- /react-text -->' +
'<!-- react-text: [0-9]+ -->child<!-- /react-text -->' +
'</span>' +
'</div>'
);
Expand Down Expand Up @@ -153,8 +153,8 @@ describe('ReactServerRendering', function() {
'<span ' + ROOT_ATTRIBUTE_NAME + '="" ' +
ID_ATTRIBUTE_NAME + '="[^"]+" ' +
ReactMarkupChecksum.CHECKSUM_ATTR_NAME + '="[^"]+">' +
'<span ' + ID_ATTRIBUTE_NAME + '="[^"]+">Component name: </span>' +
'<span ' + ID_ATTRIBUTE_NAME + '="[^"]+">TestComponent</span>' +
'<!-- react-text: [0-9]+ -->Component name: <!-- /react-text -->' +
'<!-- react-text: [0-9]+ -->TestComponent<!-- /react-text -->' +
'</span>'
);
expect(lifecycle).toEqual(
Expand Down
58 changes: 40 additions & 18 deletions src/renderers/dom/shared/ReactDOMTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,21 @@

var DOMChildrenOperations = require('DOMChildrenOperations');
var DOMLazyTree = require('DOMLazyTree');
var DOMPropertyOperations = require('DOMPropertyOperations');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactPerf = require('ReactPerf');

var assign = require('Object.assign');
var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var validateDOMNesting = require('validateDOMNesting');

var getNode = ReactDOMComponentTree.getNodeFromInstance;

/**
* Text nodes violate a couple assumptions that React makes about components:
*
* - When mounting text into the DOM, adjacent text nodes are merged.
* - Text nodes cannot be assigned a React root ID.
*
* This component is used to wrap strings in elements so that they can undergo
* the same reconciliation that is applied to elements.
* This component is used to wrap strings between comment nodes so that they
* can undergo the same reconciliation that is applied to elements.
*
* TODO: Investigate representing React components in the DOM with text nodes.
*
Expand All @@ -49,6 +46,8 @@ var ReactDOMTextComponent = function(text) {
// Properties
this._domID = null;
this._mountIndex = 0;
this._openingComment = null;
this._commentNodes = null;
};

assign(ReactDOMTextComponent.prototype, {
Expand Down Expand Up @@ -77,34 +76,44 @@ assign(ReactDOMTextComponent.prototype, {
if (parentInfo) {
// parentInfo should always be present except for the top-level
// component when server rendering
validateDOMNesting('span', this, parentInfo);
validateDOMNesting('#text', this, parentInfo);
}
}

var domID = nativeContainerInfo._idCounter++;
var openingValue = ' react-text: ' + domID + ' ';
var closingValue = ' /react-text ';
this._domID = domID;
this._nativeParent = nativeParent;
if (transaction.useCreateElement) {
var ownerDocument = nativeContainerInfo._ownerDocument;
var el = ownerDocument.createElement('span');
ReactDOMComponentTree.precacheNode(this, el);
var lazyTree = DOMLazyTree(el);
DOMLazyTree.queueText(lazyTree, this._stringText);
var openingComment = ownerDocument.createComment(openingValue);
var closingComment = ownerDocument.createComment(closingValue);
var lazyTree = DOMLazyTree(ownerDocument.createDocumentFragment());
DOMLazyTree.queueChild(lazyTree, DOMLazyTree(openingComment));
if (this._stringText) {
DOMLazyTree.queueChild(
lazyTree,
DOMLazyTree(ownerDocument.createTextNode(this._stringText))
);
}
DOMLazyTree.queueChild(lazyTree, DOMLazyTree(closingComment));
this._openingComment = openingComment;
ReactDOMComponentTree.precacheNode(this, closingComment);
return lazyTree;
} else {
var escapedText = escapeTextContentForBrowser(this._stringText);

if (transaction.renderToStaticMarkup) {
// Normally we'd wrap this in a `span` for the reasons stated above, but
// since this is a situation where React won't take over (static pages),
// we can simply return the text as it is.
// Normally we'd wrap this between comment nodes for the reasons stated
// above, but since this is a situation where React won't take over
// (static pages), we can simply return the text as it is.
return escapedText;
}

return (
'<span ' + DOMPropertyOperations.createMarkupForID(domID) + '>' +
escapedText +
'</span>'
'<!--' + openingValue + '-->' + escapedText +
'<!--' + closingValue + '-->'
);
}
},
Expand All @@ -125,16 +134,29 @@ assign(ReactDOMTextComponent.prototype, {
// and/or updateComponent to do the actual update for consistency with
// other component types?
this._stringText = nextStringText;
DOMChildrenOperations.updateTextContent(getNode(this), nextStringText);
var commentNodes = this.getNativeNode();
DOMChildrenOperations.replaceDelimitedText(
commentNodes[0],
commentNodes[1],
nextStringText
);
}
}
},

getNativeNode: function() {
return getNode(this);
var nativeNode = this._commentNodes;
if (nativeNode) {
return nativeNode;
}
nativeNode = [this._openingComment, this._nativeNode];
this._commentNodes = nativeNode;
return nativeNode;
},

unmountComponent: function() {
this._openingComment = null;
this._commentNodes = null;
ReactDOMComponentTree.uncacheNode(this);
},

Expand Down
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1260,8 +1260,8 @@ describe('ReactDOMComponent', function() {
'match the DOM tree generated by the browser.'
);
expect(console.error.argsForCall[1][0]).toBe(
'Warning: validateDOMNesting(...): <span> cannot appear as a child ' +
'of <table>. See Foo > table > span.'
'Warning: validateDOMNesting(...): #text cannot appear as a child ' +
'of <table>. See Foo > table > #text.'
);
});

Expand Down
Loading