Skip to content

Commit

Permalink
Don't wrap text in <span> elements
Browse files Browse the repository at this point in the history
Also handle cases where the browser may have split or merged adjacent text
nodes.
  • Loading branch information
mwiencek committed Jan 9, 2016
1 parent a158405 commit 3868c25
Show file tree
Hide file tree
Showing 11 changed files with 192 additions and 56 deletions.
19 changes: 15 additions & 4 deletions src/renderers/dom/client/ReactDOMComponentTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

var DOMProperty = require('DOMProperty');
var ReactDOMComponentFlags = require('ReactDOMComponentFlags');
var ReactNativeComponent = require('ReactNativeComponent');

var invariant = require('invariant');

Expand Down Expand Up @@ -87,10 +88,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 === 3 &&
ReactNativeComponent.isTextComponent(childInst)) ||
(childNode.nodeType === 8 &&
childNode.nodeValue === ' react-empty: ' + childID + ' ')) {
precacheNode(childInst, childNode);
continue outer;
}
Expand Down Expand Up @@ -148,6 +151,13 @@ function getInstanceFromNode(node) {
}
}

/**
* Given a DOM node, return true if it has a direct internal instance.
*/
function nodeHasInstance(node) {
return !!node[internalInstanceKey];
}

/**
* Given a ReactDOMComponent or ReactDOMTextComponent, return the corresponding
* DOM node.
Expand Down Expand Up @@ -188,6 +198,7 @@ var ReactDOMComponentTree = {
getClosestInstanceFromNode: getClosestInstanceFromNode,
getInstanceFromNode: getInstanceFromNode,
getNodeFromInstance: getNodeFromInstance,
nodeHasInstance: nodeHasInstance,
precacheChildNodes: precacheChildNodes,
precacheNode: precacheNode,
uncacheNode: uncacheNode,
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
29 changes: 29 additions & 0 deletions src/renderers/dom/client/utils/setTextContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,32 @@
'use strict';

var ExecutionEnvironment = require('ExecutionEnvironment');
var ReactDOMComponentTree = require('ReactDOMComponentTree');

var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
var setInnerHTML = require('setInnerHTML');

var nodeHasInstance = ReactDOMComponentTree.nodeHasInstance;

// If setTextContent is called on a text node, we first remove adjacent text
// nodes that may have been split off from the original by the browser. See
// e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=194231
function removeAdjacentTextNodesImpl(node, accessor) {
while (true) {
var sibling = node[accessor];
if (sibling && sibling.nodeType === 3 && !nodeHasInstance(sibling)) {
node.parentNode.removeChild(sibling);
} else {
break;
}
}
}

function removeAdjacentTextNodes(node) {
removeAdjacentTextNodesImpl(node, 'prevSibling');
removeAdjacentTextNodesImpl(node, 'nextSibling');
}

/**
* Set the textContent property of a node, ensuring that whitespace is preserved
* even in IE8. innerText is a poor substitute for textContent and, among many
Expand All @@ -26,12 +49,18 @@ var setInnerHTML = require('setInnerHTML');
* @internal
*/
var setTextContent = function(node, text) {
if (node.nodeType === 3) {
removeAdjacentTextNodes(node);
}
node.textContent = text;
};

if (ExecutionEnvironment.canUseDOM) {
if (!('textContent' in document.documentElement)) {
setTextContent = function(node, text) {
if (node.nodeType === 3) {
removeAdjacentTextNodes(node);
}
setInnerHTML(node, escapeTextContentForBrowser(text));
};
}
Expand Down
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,7 @@ 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>' +
'My name is child' +
'</span>' +
'</div>'
);
Expand Down Expand Up @@ -153,8 +152,7 @@ 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>' +
'Component name: TestComponent' +
'</span>'
);
expect(lifecycle).toEqual(
Expand Down
24 changes: 4 additions & 20 deletions src/renderers/dom/shared/ReactDOMTextComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

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

Expand Down Expand Up @@ -85,7 +84,7 @@ 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);
}
}

Expand All @@ -94,26 +93,11 @@ assign(ReactDOMTextComponent.prototype, {
this._nativeParent = nativeParent;
if (transaction.useCreateElement) {
var ownerDocument = nativeContainerInfo._ownerDocument;
var el = ownerDocument.createElement('span');
var el = ownerDocument.createTextNode(this._stringText);
ReactDOMComponentTree.precacheNode(this, el);
var lazyTree = DOMLazyTree(el);
DOMLazyTree.queueText(lazyTree, this._stringText);
return lazyTree;
return DOMLazyTree(el);
} 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.
return escapedText;
}

return (
'<span ' + DOMPropertyOperations.createMarkupForID(domID) + '>' +
escapedText +
'</span>'
);
return escapeTextContentForBrowser(this._stringText);
}
},

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 @@ -1172,8 +1172,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
93 changes: 81 additions & 12 deletions src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,95 @@

var React;
var ReactDOM;
var ReactDOMServer;

describe('ReactDOMTextComponent', function() {
beforeEach(function() {
React = require('React');
ReactDOM = require('ReactDOM');
ReactDOMServer = require('ReactDOMServer');
});

it('updates a mounted text component in place', function() {
var el = document.createElement('div');
var inst = ReactDOM.render(<div>{'foo'}{'bar'}</div>, el);

var foo = ReactDOM.findDOMNode(inst).children[0];
var bar = ReactDOM.findDOMNode(inst).children[1];
expect(foo.tagName).toBe('SPAN');
expect(bar.tagName).toBe('SPAN');

inst = ReactDOM.render(<div>{'baz'}{'qux'}</div>, el);
// After the update, the spans should have stayed in place (as opposed to
// getting unmounted and remounted)
expect(ReactDOM.findDOMNode(inst).children[0]).toBe(foo);
expect(ReactDOM.findDOMNode(inst).children[1]).toBe(bar);
var inst = ReactDOM.render(<div><span />{'foo'}{'bar'}</div>, el);

var foo = ReactDOM.findDOMNode(inst).childNodes[1];
var bar = ReactDOM.findDOMNode(inst).childNodes[2];
expect(foo.data).toBe('foo');
expect(bar.data).toBe('bar');

inst = ReactDOM.render(<div><span />{'baz'}{'qux'}</div>, el);
// After the update, the text nodes should have stayed in place (as opposed
// to getting unmounted and remounted)
expect(ReactDOM.findDOMNode(inst).childNodes[1]).toBe(foo);
expect(ReactDOM.findDOMNode(inst).childNodes[2]).toBe(bar);
expect(foo.data).toBe('baz');
expect(bar.data).toBe('qux');
});

it('can be toggled in and out of the markup', function() {
var el = document.createElement('div');
var inst = ReactDOM.render(<div>{'foo'}<div />{'bar'}</div>, el);

var container = ReactDOM.findDOMNode(inst);
var childDiv = container.childNodes[1];
var childNodes;

inst = ReactDOM.render(<div>{null}<div />{null}</div>, el);
container = ReactDOM.findDOMNode(inst);
childNodes = container.childNodes;
expect(childNodes.length).toBe(1);
expect(childNodes[0]).toBe(childDiv);

inst = ReactDOM.render(<div>{'foo'}<div />{'bar'}</div>, el);
container = ReactDOM.findDOMNode(inst);
childNodes = container.childNodes;
expect(childNodes.length).toBe(3);
expect(childNodes[0].data).toBe('foo');
expect(childNodes[1]).toBe(childDiv);
expect(childNodes[2].data).toBe('bar');
});

it('can reconcile text merged by Node.normalize()', function() {
var el = document.createElement('div');
var inst = ReactDOM.render(<div>{'foo'}{'bar'}{'baz'}</div>, el);

var container = ReactDOM.findDOMNode(inst);
container.normalize();

inst = ReactDOM.render(<div>{'bar'}{'baz'}{'qux'}</div>, el);
container = ReactDOM.findDOMNode(inst);
expect(container.textContent).toBe('barbazqux');
});

it('can reconcile text from pre-rendered markup', function() {
var el = document.createElement('div');
var reactEl = <div>{'foo'}{'bar'}{'baz'}</div>;
el.innerHTML = ReactDOMServer.renderToString(reactEl);

ReactDOM.render(reactEl, el);
expect(el.textContent).toBe('foobarbaz');

reactEl = <div>{''}{''}{''}</div>;
el.innerHTML = ReactDOMServer.renderToString(reactEl);

ReactDOM.render(reactEl, el);
expect(el.textContent).toBe('');
});

it('can reconcile text arbitrarily split into multiple nodes', function() {
var el = document.createElement('div');
var inst = ReactDOM.render(<div><span />{'foobarbaz'}</div>, el);

var container = ReactDOM.findDOMNode(inst);
var textNode = container.childNodes[1];
textNode.textContent = 'foo';
container.appendChild(document.createTextNode('bar'));
container.appendChild(document.createTextNode('baz'));

inst = ReactDOM.render(<div><span />{'barbazqux'}</div>, el);
container = ReactDOM.findDOMNode(inst);
expect(container.textContent).toBe('barbazqux');
});
});
4 changes: 4 additions & 0 deletions src/renderers/shared/reconciler/ReactChildReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
var ReactReconciler = require('ReactReconciler');

var instantiateReactComponent = require('instantiateReactComponent');
var isOrphanedTextComponent = require('isOrphanedTextComponent');
var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
var traverseAllChildren = require('traverseAllChildren');
var warning = require('warning');
Expand Down Expand Up @@ -89,6 +90,9 @@ var ReactChildReconciler = {
continue;
}
prevChild = prevChildren && prevChildren[name];
if (isOrphanedTextComponent(prevChild)) {
prevChild = null;
}
var prevElement = prevChild && prevChild._currentElement;
var nextElement = nextChildren[name];
if (prevChild != null &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,8 @@ var expectChildren = function(d, children) {
var child = children[i];

if (typeof child === 'string') {
textNode = outerNode.childNodes[i].firstChild;

if (child === '') {
expect(textNode).toBe(null);
} else {
expect(textNode).not.toBe(null);
expect(textNode.nodeType).toBe(3);
expect(textNode.data).toBe('' + child);
}
textNode = outerNode.childNodes[i];
expect(textNode.data).toBe('' + child);
} else {
var elementDOMNode = outerNode.childNodes[i];
expect(elementDOMNode.tagName).toBe('DIV');
Expand Down
Loading

0 comments on commit 3868c25

Please sign in to comment.