From caae627cd557812d28d11237b34bff6c661ea8bc Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sun, 19 Jul 2015 08:19:07 -0700 Subject: [PATCH] Fix switching between dangerouslySetInnerHTML and children With this, ReactMultiChild handles all of the children-related operations for ReactDOMComponent so that we don't process operations out of order. This is necessary because ReactMultiChild does its own batching so there's no way without its cooperation to get the timing right here. Ideally we'll factor this logic out a bit better in subsequent updates but this is the simplest way to fix #1232 which has embarrassingly been open for over a year. --- .../dom/client/ReactDOMIDOperations.js | 14 ---- .../__tests__/ReactDOMIDOperations-test.js | 15 +++- .../dom/client/utils/DOMChildrenOperations.js | 9 ++- src/renderers/dom/shared/ReactDOMComponent.js | 5 +- .../__tests__/ReactDOMComponent-test.js | 24 ++++++ .../shared/reconciler/ReactMultiChild.js | 74 +++++++++++++++++-- .../reconciler/ReactMultiChildUpdateTypes.js | 1 + src/test/ReactDefaultPerfAnalysis.js | 2 +- 8 files changed, 115 insertions(+), 29 deletions(-) diff --git a/src/renderers/dom/client/ReactDOMIDOperations.js b/src/renderers/dom/client/ReactDOMIDOperations.js index 637035fcd8a17..bec8d3e6192a5 100644 --- a/src/renderers/dom/client/ReactDOMIDOperations.js +++ b/src/renderers/dom/client/ReactDOMIDOperations.js @@ -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()`. @@ -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`. * @@ -171,7 +158,6 @@ ReactPerf.measureMethods(ReactDOMIDOperations, 'ReactDOMIDOperations', { updatePropertyByID: 'updatePropertyByID', deletePropertyByID: 'deletePropertyByID', updateStylesByID: 'updateStylesByID', - updateInnerHTMLByID: 'updateInnerHTMLByID', updateTextContentByID: 'updateTextContentByID', dangerouslyReplaceNodeWithMarkupByID: 'dangerouslyReplaceNodeWithMarkupByID', dangerouslyProcessChildrenUpdates: 'dangerouslyProcessChildrenUpdates', diff --git a/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js b/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js index be744c7a5fd8f..18eb7378a2ee5 100644 --- a/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js +++ b/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js @@ -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() { @@ -44,9 +45,17 @@ describe('ReactDOMIDOperations', function() { var html = '\n \t \n testContent \t \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( diff --git a/src/renderers/dom/client/utils/DOMChildrenOperations.js b/src/renderers/dom/client/utils/DOMChildrenOperations.js index cd739d875dbaf..ad6bea4057ba9 100644 --- a/src/renderers/dom/client/utils/DOMChildrenOperations.js +++ b/src/renderers/dom/client/utils/DOMChildrenOperations.js @@ -15,6 +15,7 @@ var Danger = require('Danger'); var ReactMultiChildUpdateTypes = require('ReactMultiChildUpdateTypes'); +var setInnerHTML = require('setInnerHTML'); var setTextContent = require('setTextContent'); var invariant = require('invariant'); @@ -123,10 +124,16 @@ var DOMChildrenOperations = { update.toIndex ); break; + case ReactMultiChildUpdateTypes.SET_MARKUP: + setInnerHTML( + update.parentNode, + update.content + ); + break; case ReactMultiChildUpdateTypes.TEXT_CONTENT: setTextContent( update.parentNode, - update.textContent + update.content ); break; case ReactMultiChildUpdateTypes.REMOVE_NODE: diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index 9f3af5cfc0474..4ed845c5b8b0b 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -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); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index d658a862370c5..445fd2f04b19f 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -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( +
, + container + ); + + expect(container.textContent).toEqual('bonjour'); + React.render(
adieu
, container); + expect(container.textContent).toEqual('adieu'); + }); + + it('should transition from children to innerHTML in nested el', function() { + var container = document.createElement('div'); + React.render(
adieu
, container); + + expect(container.textContent).toEqual('adieu'); + React.render( +
, + container + ); + expect(container.textContent).toEqual('bonjour'); + }); + it('should not incur unnecessary DOM mutations', function() { var container = document.createElement('div'); React.render(
, container); diff --git a/src/renderers/shared/reconciler/ReactMultiChild.js b/src/renderers/shared/reconciler/ReactMultiChild.js index af5e42f503bad..3ae6ddd790b18 100644 --- a/src/renderers/shared/reconciler/ReactMultiChild.js +++ b/src/renderers/shared/reconciler/ReactMultiChild.js @@ -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, }); @@ -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, }); @@ -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. * @@ -121,7 +141,7 @@ function enqueueTextContent(parentID, textContent) { parentNode: null, type: ReactMultiChildUpdateTypes.TEXT_CONTENT, markupIndex: null, - textContent: textContent, + content: textContent, fromIndex: null, toIndex: null, }); @@ -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. * @@ -355,7 +407,7 @@ var ReactMultiChild = { * @protected */ createChild: function(child, mountImage) { - enqueueMarkup(this._rootNodeID, mountImage, child._mountIndex); + enqueueInsertMarkup(this._rootNodeID, mountImage, child._mountIndex); }, /** @@ -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. * diff --git a/src/renderers/shared/reconciler/ReactMultiChildUpdateTypes.js b/src/renderers/shared/reconciler/ReactMultiChildUpdateTypes.js index d6ccf99071847..9cf1fb1f336bd 100644 --- a/src/renderers/shared/reconciler/ReactMultiChildUpdateTypes.js +++ b/src/renderers/shared/reconciler/ReactMultiChildUpdateTypes.js @@ -25,6 +25,7 @@ var ReactMultiChildUpdateTypes = keyMirror({ INSERT_MARKUP: null, MOVE_EXISTING: null, REMOVE_NODE: null, + SET_MARKUP: null, TEXT_CONTENT: null, }); diff --git a/src/test/ReactDefaultPerfAnalysis.js b/src/test/ReactDefaultPerfAnalysis.js index 869c2b0e39b5c..3dee0db821748 100644 --- a/src/test/ReactDefaultPerfAnalysis.js +++ b/src/test/ReactDefaultPerfAnalysis.js @@ -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', };