Skip to content

Use feature detection and more robust recovering of whitespace for innerHTML in IE8 #684

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

Merged
merged 1 commit into from
Dec 23, 2013
Merged

Use feature detection and more robust recovering of whitespace for innerHTML in IE8 #684

merged 1 commit into from
Dec 23, 2013

Conversation

syranide
Copy link
Contributor

dangerouslySetInnerHTML in IE8 collapses whitespace in root TextNodes and removes leading whitespace. Your current fix for this is to replace any first space with   which is a surprisingly good and simple fix, but it does this for all browsers and doesn't solve the collapsing whitespace (or any whitespace besides space).

I propose the following fix instead, which makes it work identically to all other browsers at minimal cost, and does so via feature detection so only IE8 is affected.

PS. Make sure to spot the seriously magic trick I discovered to making IE8 behave better!

   raw     gz Compared to master @ 09bdcefd4f246eb49b82aa5bfece8d40cb8a2fca
     =      = build/JSXTransformer.js
 +3155   +642 build/react-test.js
  +658   +217 build/react-with-addons.js
  +169    +74 build/react-with-addons.min.js
  +658   +214 build/react.js
  +169    +91 build/react.min.js

Disclaimer: if the root node is a TABLE/TBODY/OL/SELECT or anything else that (kind of) doesn't support TextNodes AND there is whitespace in the beginning of the HTML, this will create a faulty TextNode with whitespace (when it actually should be removed)... however, the code/size penalty for solving this is unreasonably large when the use-cases are virtually none, as far as I'm aware

@zpao
Copy link
Member

zpao commented Dec 21, 2013

cc @benjamn @yungsters

node.parentNode.replaceChild(node, node);
}

if (useWhitespaceWorkaround && html.match(/^[ \r\n\t]/)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this regular expression any different from /^\s/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

\s is supposedly sugar for [ \f\n\r\t\v​\u00a0\u1680​\u180e\u2000​\u2001\u2002​\u2003\u2004​\u2005\u2006​\u2007\u2008​\u2009\u200a​\u2028\u2029​​\u202f\u205f​\u3000] so it matches a lot of other whitespace as well which does IE8 not collapse. However, your comment prompted me to do a more thorough test and it apparently also treats \f as whitespace. So I should add that, but in practice, \s could be used, it's not invasive, the regex is simply used to prevent the more costly branch from being run unnecessarily (and the other ones are very much extremely unlikely special cases).

Prefer that I add \f or switch to \s?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think just add \f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fixed, will hold of on pushing to prevent outdated comments.

@syranide
Copy link
Contributor Author

@benjamn Pushed the current changes. I did modify the current test a bit to excessively test that whitespace is kept as-is. As for undefined, what do you want to do? Keep it as-is or should it be using a more robust test? (Despite it not being used everywhere in React)

benjamn added a commit that referenced this pull request Dec 23, 2013
Use feature detection and more robust recovering of whitespace for innerHTML in IE8.
@benjamn benjamn merged commit 82a26ad into facebook:master Dec 23, 2013
@syranide syranide deleted the ie8dangerousinnerhtml branch December 23, 2013 23:11
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