Skip to content
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

Fix reconciling when switching to/from innerHTML #378

Merged
merged 1 commit into from
Oct 1, 2013

Conversation

sophiebits
Copy link
Collaborator

There's no way that this can work if _updateDOMChildren doesn't know about dangerouslySetInnerHTML, so tell it.

Fixes #377.

@brianr
Copy link
Contributor

brianr commented Sep 27, 2013

Ah, beat me to it -- I was just about to suggest the following as a fix:

--- a/src/core/ReactNativeComponent.js
+++ b/src/core/ReactNativeComponent.js
@@ -320,6 +320,12 @@ ReactNativeComponent.Mixin = {
       CONTENT_TYPES[typeof lastProps.children] ? lastProps.children : null;
     var contentToUse =
       CONTENT_TYPES[typeof nextProps.children] ? nextProps.children : null;
+    
+    if (nextProps.dangerouslySetInnerHTML && nextProps.dangerouslySetInnerHTML.__html) {
+      // DOM node will have already been modified in _updateDomProperties
+      // return so we avoid treating it as removed.
+      return;
+    }

     // Note the use of `!=` which checks for null or undefined.

Tests pass, but seems like a hack. I don't feel in any way qualified to compare this vs @spicyj's fix.

@sophiebits
Copy link
Collaborator Author

In addition to feeling hacky, that won't properly clean up the old children when switching from children to HTML.

@brianr
Copy link
Contributor

brianr commented Sep 27, 2013

@spicyj Makes sense, thanks for the explanation.

@zpao
Copy link
Member

zpao commented Sep 27, 2013

It may be a bit hacky @brianr but you managed to find your way around core pretty quickly. Let me know if you're interested in some other tasks :)

@@ -143,7 +143,7 @@ var ReactDOMIDOperations = {
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, ' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment for this method is no longer correct, can you fix it?

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.

@petehunt
Copy link
Contributor

@spicyj it looks like you're emptying content and then may put new innerhtml or text into it. This would be 2 dom mutations. Possible to make it one?

@sophiebits
Copy link
Collaborator Author

I'm only doing this.updateTextContent(''); if !nextHasContentOrHtml. I am running this.updateChildren(null, transaction); when replacing with text content or HTML, but that's necessary to properly unmount all of the child components.

* @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(/^ /g, ' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is html nullable? If so, can you change the docblock's type to {?string}? If it is not nullable, you can just do html.replace(...).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old docstring was similarly discongruous; it said "An HTML object with the __html property." but had the fallback. Happy to remove it if {__html: null} is unsupported (which seems reasonable to me).

By the way, should that regex be /^ +/g and should it be extracted out of this function so it can be compiled once?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... yeah, let's require it to be a string. And yes, let's pull it out of the function so it can be precompiled.

I think /^ /g is correct. We want to replace the leading space with   so that the remaining whitespace characters are preserved. For example:

<pre>  2-space indentation</pre>

Replacing the first whitespace with &nbsp; should give us similar behavior in IE8- as what we'd expect. If we replace all of the leading whitespace characters, we would lose the second character.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I totally misread that and imagined that it was stripping the spaces. Never mind on the +. :)

There's no way that this can work if _updateDOMChildren doesn't know about dangerouslySetInnerHTML, so tell it.

Fixes facebook#377.
zpao added a commit that referenced this pull request Oct 1, 2013
Fix reconciling when switching to/from innerHTML
@zpao zpao merged commit f20626f into facebook:master Oct 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dangerouslySetInnerHTML doesn't properly reconcile
5 participants