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 637035fcd8a1..bec8d3e6192a 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 be744c7a5fd8..18eb7378a2ee 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 cd739d875dba..ad6bea4057ba 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 9f3af5cfc047..4ed845c5b8b0 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 d658a862370c..445fd2f04b19 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 af5e42f503ba..3ae6ddd790b1 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 d6ccf9907184..9cf1fb1f336b 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 869c2b0e39b5..3dee0db82174 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', };