Skip to content

Conversation

@syranide
Copy link
Contributor

Before this PR: http://dev.cetrez.com/jsx/2/indexie8.html
After this PR: http://dev.cetrez.com/jsx/2/indexie8fix.html

There's already fix to deal with this, but \r\n incorrectly become two newlines as you can see above. With this PR, the implementation builds on our "proven" consistent behavior of innerHTML for IE8, rather than TextNode, so it should also be favorable in that regard.

So if there are any other inconsistencies we are unaware of with innerHTML, they should at least be consistent between initial render and update now, which is a preferable behavior IMHO.

Pairs well with #1599 and especially #1863 to enjoy less overhead, #1863 could also be transplanted into setTextContent if we don't want to take it as-is.

PS. While we should probably follow #2273 and drop setInnerHTML, it would be too heavy-handed for setting text content. When that time comes, simply stripping setInnerHTML down to the essentials and inlining it into setTextContent should be the correct approach.

@syranide
Copy link
Contributor Author

I've submitted #2340, which should be fine to rely on, but it is probably not a good idea from a performance perspective and inlining the essential guts (replaceNode and prepend non-whitespace char) of the old setInnerHTML should be preferable.

syranide added a commit that referenced this pull request Feb 2, 2015
Newlines handled incorrectly by innerText in IE8
@syranide syranide merged commit 63c3461 into facebook:master Feb 2, 2015
@syranide syranide deleted the ie8text2 branch February 2, 2015 20:33
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c371709 on syranide:ie8text2 into * on facebook:master*.

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.

3 participants