Skip to content

Fix switching between dangerouslySetInnerHTML and children #4427

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

Merged
merged 1 commit into from
Jul 20, 2015
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
14 changes: 0 additions & 14 deletions src/renderers/dom/client/ReactDOMIDOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ var ReactMount = require('ReactMount');
var ReactPerf = require('ReactPerf');

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

/**
* Errors for properties that should not be updated with `updatePropertyByID()`.
Expand Down Expand Up @@ -115,18 +114,6 @@ var ReactDOMIDOperations = {
CSSPropertyOperations.setValueForStyles(node, styles);
},

/**
* Updates a DOM node's innerHTML.
*
* @param {string} id ID of the node to update.
* @param {string} html An HTML string.
* @internal
*/
updateInnerHTMLByID: function(id, html) {
var node = ReactMount.getNode(id);
setInnerHTML(node, html);
},

/**
* Updates a DOM node's text content set by `props.content`.
*
Expand Down Expand Up @@ -171,7 +158,6 @@ ReactPerf.measureMethods(ReactDOMIDOperations, 'ReactDOMIDOperations', {
updatePropertyByID: 'updatePropertyByID',
deletePropertyByID: 'deletePropertyByID',
updateStylesByID: 'updateStylesByID',
updateInnerHTMLByID: 'updateInnerHTMLByID',
updateTextContentByID: 'updateTextContentByID',
dangerouslyReplaceNodeWithMarkupByID: 'dangerouslyReplaceNodeWithMarkupByID',
dangerouslyProcessChildrenUpdates: 'dangerouslyProcessChildrenUpdates',
Expand Down
15 changes: 12 additions & 3 deletions src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ describe('ReactDOMIDOperations', function() {
var DOMPropertyOperations = require('DOMPropertyOperations');
var ReactDOMIDOperations = require('ReactDOMIDOperations');
var ReactMount = require('ReactMount');
var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes');
var keyOf = require('keyOf');

it('should disallow updating special properties', function() {
Expand Down Expand Up @@ -44,9 +45,17 @@ describe('ReactDOMIDOperations', function() {

var html = '\n \t <span> \n testContent \t </span> \n \t';

ReactDOMIDOperations.updateInnerHTMLByID(
'testID',
html
ReactDOMIDOperations.dangerouslyProcessChildrenUpdates(
[{
parentID: 'testID',
parentNode: null,
type: ReactMultiChildUpdateTypes.SET_MARKUP,
markupIndex: null,
content: html,
fromIndex: null,
toIndex: null,
}],
[]
);

expect(
Expand Down
9 changes: 8 additions & 1 deletion src/renderers/dom/client/utils/DOMChildrenOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
var Danger = require('Danger');
var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes');

var setInnerHTML = require('setInnerHTML');
var setTextContent = require('setTextContent');
var invariant = require('invariant');

Expand Down Expand Up @@ -123,10 +124,16 @@ var DOMChildrenOperations = {
update.toIndex
);
break;
case ReactMultiChildUpdateTypes.SET_MARKUP:
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to add SET_MARKUP to ReactMultiChildUpdateTypes 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

setInnerHTML(
update.parentNode,
update.content
);
break;
case ReactMultiChildUpdateTypes.TEXT_CONTENT:
setTextContent(
update.parentNode,
update.textContent
update.content
);
break;
case ReactMultiChildUpdateTypes.REMOVE_NODE:
Expand Down
5 changes: 1 addition & 4 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -884,10 +884,7 @@ ReactDOMComponent.Mixin = {
}
} else if (nextHtml != null) {
if (lastHtml !== nextHtml) {
BackendIDOperations.updateInnerHTMLByID(
this._rootNodeID,
nextHtml
);
this.updateMarkup('' + nextHtml);
}
} else if (nextChildren != null) {
this.updateChildren(nextChildren, transaction, context);
Expand Down
24 changes: 24 additions & 0 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,30 @@ describe('ReactDOMComponent', function() {
expect(container.firstChild.innerHTML).toEqual('adieu');
});

it('should transition from innerHTML to children in nested el', function() {
var container = document.createElement('div');
React.render(
<div><div dangerouslySetInnerHTML={{__html: 'bonjour'}} /></div>,
container
);

expect(container.textContent).toEqual('bonjour');
React.render(<div><div><span>adieu</span></div></div>, container);
expect(container.textContent).toEqual('adieu');
});

it('should transition from children to innerHTML in nested el', function() {
var container = document.createElement('div');
React.render(<div><div><span>adieu</span></div></div>, container);

expect(container.textContent).toEqual('adieu');
React.render(
<div><div dangerouslySetInnerHTML={{__html: 'bonjour'}} /></div>,
container
);
expect(container.textContent).toEqual('bonjour');
});

it('should not incur unnecessary DOM mutations', function() {
var container = document.createElement('div');
React.render(<div value="" />, container);
Expand Down
74 changes: 68 additions & 6 deletions src/renderers/shared/reconciler/ReactMultiChild.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ var markupQueue = [];
* @param {number} toIndex Destination index.
* @private
*/
function enqueueMarkup(parentID, markup, toIndex) {
function enqueueInsertMarkup(parentID, markup, toIndex) {
// NOTE: Null values reduce hidden classes.
updateQueue.push({
parentID: parentID,
parentNode: null,
type: ReactMultiChildUpdateTypes.INSERT_MARKUP,
markupIndex: markupQueue.push(markup) - 1,
textContent: null,
content: null,
fromIndex: null,
toIndex: toIndex,
});
Expand All @@ -81,7 +81,7 @@ function enqueueMove(parentID, fromIndex, toIndex) {
parentNode: null,
type: ReactMultiChildUpdateTypes.MOVE_EXISTING,
markupIndex: null,
textContent: null,
content: null,
fromIndex: fromIndex,
toIndex: toIndex,
});
Expand All @@ -101,12 +101,32 @@ function enqueueRemove(parentID, fromIndex) {
parentNode: null,
type: ReactMultiChildUpdateTypes.REMOVE_NODE,
markupIndex: null,
textContent: null,
content: null,
fromIndex: fromIndex,
toIndex: null,
});
}

/**
* Enqueues setting the markup of a node.
*
* @param {string} parentID ID of the parent component.
* @param {string} markup Markup that renders into an element.
* @private
*/
function enqueueSetMarkup(parentID, markup) {
// NOTE: Null values reduce hidden classes.
updateQueue.push({
parentID: parentID,
parentNode: null,
type: ReactMultiChildUpdateTypes.SET_MARKUP,
markupIndex: null,
content: markup,
fromIndex: null,
toIndex: null,
});
}

/**
* Enqueues setting the text content.
*
Expand All @@ -121,7 +141,7 @@ function enqueueTextContent(parentID, textContent) {
parentNode: null,
type: ReactMultiChildUpdateTypes.TEXT_CONTENT,
markupIndex: null,
textContent: textContent,
content: textContent,
fromIndex: null,
toIndex: null,
});
Expand Down Expand Up @@ -237,6 +257,38 @@ var ReactMultiChild = {
}
},

/**
* Replaces any rendered children with a markup string.
*
* @param {string} nextMarkup String of markup.
* @internal
*/
updateMarkup: function(nextMarkup) {
updateDepth++;
var errorThrown = true;
try {
var prevChildren = this._renderedChildren;
// Remove any rendered children.
ReactChildReconciler.unmountChildren(prevChildren);
for (var name in prevChildren) {
if (prevChildren.hasOwnProperty(name)) {
this._unmountChildByName(prevChildren[name], name);
}
}
this.setMarkup(nextMarkup);
errorThrown = false;
} finally {
updateDepth--;
if (!updateDepth) {
if (errorThrown) {
clearQueue();
} else {
processQueue();
}
}
}
},

/**
* Updates the rendered children with new children.
*
Expand Down Expand Up @@ -355,7 +407,7 @@ var ReactMultiChild = {
* @protected
*/
createChild: function(child, mountImage) {
enqueueMarkup(this._rootNodeID, mountImage, child._mountIndex);
enqueueInsertMarkup(this._rootNodeID, mountImage, child._mountIndex);
},

/**
Expand All @@ -378,6 +430,16 @@ var ReactMultiChild = {
enqueueTextContent(this._rootNodeID, textContent);
},

/**
* Sets this markup string.
*
* @param {string} markup Markup to set.
* @protected
*/
setMarkup: function(markup) {
enqueueSetMarkup(this._rootNodeID, markup);
},

/**
* Mounts a child with the supplied name.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var ReactMultiChildUpdateTypes = keyMirror({
INSERT_MARKUP: null,
MOVE_EXISTING: null,
REMOVE_NODE: null,
SET_MARKUP: null,
TEXT_CONTENT: null,
});

Expand Down
2 changes: 1 addition & 1 deletion src/test/ReactDefaultPerfAnalysis.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ var DOM_OPERATION_TYPES = {
INSERT_MARKUP: 'set innerHTML',
MOVE_EXISTING: 'move',
REMOVE_NODE: 'remove',
SET_MARKUP: 'set innerHTML',
TEXT_CONTENT: 'set textContent',
'updatePropertyByID': 'update attribute',
'deletePropertyByID': 'delete attribute',
'updateStylesByID': 'update styles',
'updateInnerHTMLByID': 'set innerHTML',
'dangerouslyReplaceNodeWithMarkupByID': 'replace',
};

Expand Down