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

dangerouslySetInnerHTML doesn't properly reconcile #377

Closed
vjeux opened this issue Sep 27, 2013 · 20 comments · Fixed by #378
Closed

dangerouslySetInnerHTML doesn't properly reconcile #377

vjeux opened this issue Sep 27, 2013 · 20 comments · Fixed by #378

Comments

@vjeux
Copy link
Contributor

vjeux commented Sep 27, 2013

@sbezboro reported the following issue on IRC:

http://jsfiddle.net/vjeux/8vZ57/1/ (Both on master and 4.0)

The repro case is:

  1. Render <div dangerouslySetInnerHTML={{__html: '<b>HTML</b>'}} />
  2. Render <div>Text</div>
  3. Render the same as 1)

The div has its innerHTML empty instead of <b>HTML</b>.

I don't really know what's going on. @jordwalke, @petehunt can you guys look at it?

@brianr
Copy link
Contributor

brianr commented Sep 27, 2013

One other tidbit: this only seems to happen when the __html value is exactly the same in step 3 as it was in step 1. If it's different (i.e. '<b>html 2</b>'), things work as expected.

@petehunt
Copy link
Contributor

What happens if you change step 2 to a span instead of a div? It works, right?

@vjeux
Copy link
Contributor Author

vjeux commented Sep 27, 2013

I just reduced the test case to 19 lines.

@vjeux
Copy link
Contributor Author

vjeux commented Sep 27, 2013

@petehunt: yeah changing 2) to use a span works as expected

@petehunt
Copy link
Contributor

If anyone wants to look into this, the bug is probably here https://github.com/facebook/react/blob/master/src/core/ReactNativeComponent.js#L280

@vjeux
Copy link
Contributor Author

vjeux commented Sep 27, 2013

@brianr: I cannot reproduce what you are seeing: http://jsfiddle.net/vjeux/FwvMt/ It also bugs if I use a different string

@brianr
Copy link
Contributor

brianr commented Sep 27, 2013

@vjeux I'm not able to reproduce that particular behavior given this small repro case. The original code had several more layers... will keep trying.

sophiebits added a commit to sophiebits/react that referenced this issue Sep 27, 2013
There's no way that this can work if _updateDOMChildren doesn't know about dangerouslySetInnerHTML, so tell it.

Fixes facebook#377.
@zpao zpao closed this as completed in #378 Oct 1, 2013
@adewes
Copy link

adewes commented May 10, 2015

I know this is a very old issue but I had a similar problem and came across this.

I found that this problem occurs when you do not add a key property to the React component containing the dangerouslySetInnerHTML property. So,

<div dangerouslySetInnerHTML={{__html : markdownText}} />

does not seem to work reproducibly, whereas

<div key="some_unique_key" dangerouslySetInnerHTML={{__html : markdownText}} />

seems to. I think this could be related to React's diffing algorithm that might not properly treat a component with a dangerouslySetInnerHTML statement if that component gets rerendered multiple times with different inner content. Just posting this here in case other people have the same issue and stumble upon this.

@cdujeu
Copy link

cdujeu commented May 15, 2015

thx @adewes you saved my day :-)

@LeZuse
Copy link

LeZuse commented May 17, 2015

Seems like the issue is back again. @adewes Thanks, solves the problem for me too.

@sophiebits
Copy link
Collaborator

@adewes @cdujeu @LeZuse If any of you can post a simple repro case showing the broken behavior here, I'd be happy to take a look at fixing it.

@LeZuse
Copy link

LeZuse commented May 17, 2015

Took me a while to repro, but here you go: http://jsfiddle.net/LeZuse/uoff476z/2/ (ugly, but demonstrates the issue well)

@sophiebits
Copy link
Collaborator

@LeZuse Thanks. This is the same issue as #1232. No fix currently, but using the key as a workaround is fine and I'll see if we can fix this sometime.

@sreahard
Copy link

Thanks @adewes that was giving me fits!

@sophiebits
Copy link
Collaborator

If anyone is still seeing issues on 0.14, please make a new issue with a simple repro case.

@ksavenkov
Copy link

I have this issue, but can't reproduce in a simple example - my original code is layered and I think that's the reason the real DOM doesn't reconcile. In the react debugger is looks as dangerouslySetInnerHTML has proper __html value, but it doesn't equeal to the innerHTML of the actual element.

I used an extremely simple (and equally nasty) workaround. Writing it here just in case it will help someone:

componentDidUpdate: function() {
    // force html update
    if (this.props.html !== ReactDOM.findDOMNode(this).innerHTML) {
      ReactDOM.findDOMNode(this).innerHTML = this.props.html;
   } 
} 

@sophiebits
Copy link
Collaborator

Reading .innerHTML out will rarely give you what you put in because the browser does various kinds of normalization on it. If you can repro with a standalone example though I'm happy to take a look.

@ksavenkov
Copy link

well, that's more like a control shot :-)

@revolunet
Copy link

Looks like the issue still exist in 16.4.1 when using dangerouslySetInnerHTML on a <p> tag, you cannot nest other <p> or <ul>. issue is fixed by replacing the container with a <div> 🤔

@gaearon
Copy link
Collaborator

gaearon commented Aug 23, 2018

If you experience something similar three years later, and the original issue is closed, you can be sure it's a different unrelated issue. :-) Please file a new one. We don't keep track of closed issues so your feedback is unfortunately going into the void. When filing a new issue, please provide a reproducing example.

when using dangerouslySetInnerHTML on a <p> tag, you cannot nest other <p> or <ul>

This sounds like browsers work. You can't nest <p> into <p>. This is why React warns when you do it (except dangerouslyInnerHTML in which case React can't validate it).

@facebook facebook locked as resolved and limited conversation to collaborators Aug 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.