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

Newlines handled incorrectly by innerText in IE8 #1864

Merged
merged 1 commit into from
Feb 2, 2015
Merged

Newlines handled incorrectly by innerText in IE8 #1864

merged 1 commit into from
Feb 2, 2015

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