From 3ca507d73ffc07087ed4e95e010a101298f78146 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Thu, 26 Sep 2013 23:52:58 -0700 Subject: [PATCH] Fix reconciling when switching to/from innerHTML There's no way that this can work if _updateDOMChildren doesn't know about dangerouslySetInnerHTML, so tell it. Fixes #377. --- src/core/ReactDOMIDOperations.js | 8 ++- src/core/ReactNativeComponent.js | 68 +++++++++---------- .../__tests__/ReactDOMIDOperations-test.js | 2 +- .../__tests__/ReactNativeComponent-test.js | 25 ++++++- 4 files changed, 64 insertions(+), 39 deletions(-) diff --git a/src/core/ReactDOMIDOperations.js b/src/core/ReactDOMIDOperations.js index 3b7d21748c1..f07841a2fbd 100644 --- a/src/core/ReactDOMIDOperations.js +++ b/src/core/ReactDOMIDOperations.js @@ -49,6 +49,8 @@ var INVALID_PROPERTY_ERRORS = { */ var textContentAccessor = getTextContentAccessor() || 'NA'; +var LEADING_SPACE = /^ /; + /** * Operations used to process updates to DOM nodes. This is made injectable via * `ReactComponent.DOMIDOperations`. @@ -133,17 +135,17 @@ var ReactDOMIDOperations = { }, /** - * Updates a DOM node's innerHTML set by `props.dangerouslySetInnerHTML`. + * Updates a DOM node's innerHTML. * * @param {string} id ID of the node to update. - * @param {object} html An HTML object with the `__html` property. + * @param {string} html An HTML string. * @internal */ updateInnerHTMLByID: function(id, html) { var node = ReactMount.getNode(id); // HACK: IE8- normalize whitespace in innerHTML, removing leading spaces. // @see quirksmode.org/bugreports/archives/2004/11/innerhtml_and_t.html - node.innerHTML = (html && html.__html || '').replace(/^ /g, ' '); + node.innerHTML = html.replace(LEADING_SPACE, ' '); }, /** diff --git a/src/core/ReactNativeComponent.js b/src/core/ReactNativeComponent.js index 2ccc69b44f3..dff72fa0618 100644 --- a/src/core/ReactNativeComponent.js +++ b/src/core/ReactNativeComponent.js @@ -42,7 +42,6 @@ var registrationNames = ReactEventEmitter.registrationNames; // For quickly matching children type, to test if can be treated as content. var CONTENT_TYPES = {'string': true, 'number': true}; -var DANGEROUSLY_SET_INNER_HTML = keyOf({dangerouslySetInnerHTML: null}); var STYLE = keyOf({style: null}); /** @@ -231,15 +230,11 @@ ReactNativeComponent.Mixin = { styleUpdates[styleName] = ''; } } - } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { - // http://jsperf.com/emptying-speed - ReactComponent.DOMIDOperations.updateTextContentByID( - this._rootNodeID, - '' - ); } else if (registrationNames[propKey]) { deleteListener(this._rootNodeID, propKey); - } else { + } else if ( + DOMProperty.isStandardName[propKey] || + DOMProperty.isCustomAttribute(propKey)) { ReactComponent.DOMIDOperations.deletePropertyByID( this._rootNodeID, propKey @@ -277,15 +272,6 @@ ReactNativeComponent.Mixin = { // Relies on `updateStylesByID` not mutating `styleUpdates`. styleUpdates = nextProp; } - } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { - var lastHtml = lastProp && lastProp.__html; - var nextHtml = nextProp && nextProp.__html; - if (lastHtml !== nextHtml) { - ReactComponent.DOMIDOperations.updateInnerHTMLByID( - this._rootNodeID, - nextProp - ); - } } else if (registrationNames[propKey]) { putListener(this._rootNodeID, propKey, nextProp); } else if ( @@ -316,31 +302,45 @@ ReactNativeComponent.Mixin = { _updateDOMChildren: function(lastProps, transaction) { var nextProps = this.props; - var lastUsedContent = + var lastContent = CONTENT_TYPES[typeof lastProps.children] ? lastProps.children : null; - var contentToUse = + var nextContent = CONTENT_TYPES[typeof nextProps.children] ? nextProps.children : null; + var lastHtml = + lastProps.dangerouslySetInnerHTML && + lastProps.dangerouslySetInnerHTML.__html; + var nextHtml = + nextProps.dangerouslySetInnerHTML && + nextProps.dangerouslySetInnerHTML.__html; + // Note the use of `!=` which checks for null or undefined. + var lastChildren = lastContent != null ? null : lastProps.children; + var nextChildren = nextContent != null ? null : nextProps.children; - var lastUsedChildren = - lastUsedContent != null ? null : lastProps.children; - var childrenToUse = contentToUse != null ? null : nextProps.children; + // If we're switching from children to content/html or vice versa, remove + // the old content + var lastHasContentOrHtml = lastContent != null || lastHtml != null; + var nextHasContentOrHtml = nextContent != null || nextHtml != null; + if (lastChildren != null && nextChildren == null) { + this.updateChildren(null, transaction); + } else if (lastHasContentOrHtml && !nextHasContentOrHtml) { + this.updateTextContent(''); + } - if (contentToUse != null) { - var childrenRemoved = lastUsedChildren != null && childrenToUse == null; - if (childrenRemoved) { - this.updateChildren(null, transaction); - } - if (lastUsedContent !== contentToUse) { - this.updateTextContent('' + contentToUse); + if (nextContent != null) { + if (lastContent !== nextContent) { + this.updateTextContent('' + nextContent); } - } else { - var contentRemoved = lastUsedContent != null && contentToUse == null; - if (contentRemoved) { - this.updateTextContent(''); + } else if (nextHtml != null) { + if (lastHtml !== nextHtml) { + ReactComponent.DOMIDOperations.updateInnerHTMLByID( + this._rootNodeID, + nextHtml + ); } - this.updateChildren(flattenChildren(nextProps.children), transaction); + } else if (nextChildren != null) { + this.updateChildren(flattenChildren(nextChildren), transaction); } }, diff --git a/src/core/__tests__/ReactDOMIDOperations-test.js b/src/core/__tests__/ReactDOMIDOperations-test.js index 0dfd2bfd470..6aa5fd26126 100644 --- a/src/core/__tests__/ReactDOMIDOperations-test.js +++ b/src/core/__tests__/ReactDOMIDOperations-test.js @@ -53,7 +53,7 @@ describe('ReactDOMIDOperations', function() { ReactDOMIDOperations.updateInnerHTMLByID( 'testID', - {__html: ' testContent'} + ' testContent' ); expect( diff --git a/src/core/__tests__/ReactNativeComponent-test.js b/src/core/__tests__/ReactNativeComponent-test.js index 29a9498a86d..96847425d09 100644 --- a/src/core/__tests__/ReactNativeComponent-test.js +++ b/src/core/__tests__/ReactNativeComponent-test.js @@ -152,7 +152,7 @@ describe('ReactNativeComponent', function() { it("should empty element when removing innerHTML", function() { var stub = ReactTestUtils.renderIntoDocument( -
+
); expect(stub.getDOMNode().innerHTML).toEqual(':)'); @@ -160,6 +160,29 @@ describe('ReactNativeComponent', function() { expect(stub.getDOMNode().innerHTML).toEqual(''); }); + it("should transition from string content to innerHTML", function() { + var stub = ReactTestUtils.renderIntoDocument( +
hello
+ ); + + expect(stub.getDOMNode().innerHTML).toEqual('hello'); + stub.receiveProps( + {dangerouslySetInnerHTML: {__html: 'goodbye'}}, + transaction + ); + expect(stub.getDOMNode().innerHTML).toEqual('goodbye'); + }); + + it("should transition from innerHTML to string content", function() { + var stub = ReactTestUtils.renderIntoDocument( +
+ ); + + expect(stub.getDOMNode().innerHTML).toEqual('bonjour'); + stub.receiveProps({children: 'adieu'}, transaction); + expect(stub.getDOMNode().innerHTML).toEqual('adieu'); + }); + it("should not incur unnecessary DOM mutations", function() { var stub = ReactTestUtils.renderIntoDocument(
);