Skip to content

Conversation

@syranide
Copy link
Contributor

What the title says, if it appends the \uFEFF to fix whitespace it can end up leaving an empty TextNode behind. This is very bad because it offsets the index of children. Also, renderComponent still uses firstChild which causes it to see the TextNode and replace the content rather than update it as it should.

@zpao We definitely want to merge this sooner rather than later.

@syranide
Copy link
Contributor Author

@zpao I've added a simple test-case to prevent future regressions (but like spicyj said, we don't run these tests on IE8?).

@syranide syranide mentioned this pull request Jul 23, 2014
10 tasks
Copy link
Member

Choose a reason for hiding this comment

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

Is .removeNode universally supported? If not, node.removeChild(textNode) is definitely safe.
@benjamn

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's only IE (and I don't know what versions). This workaround is only IE though so that might be ok. any chance this will ever run for non-IE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zpao My intention here was that this only targets IE8, which does support it. As I mentioned somewhere else, we could change the feature test to just document.documentMode === 8, because if some other old browser flunks the test, I really doubt that this trickery would fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change pushed.

zpao added a commit that referenced this pull request Jul 24, 2014
Remove empty TextNode left behind by IE8 setInnerHTML workaround
@zpao zpao merged commit efdc5da into facebook:master Jul 24, 2014
@syranide syranide deleted the ie8nmlb branch July 25, 2014 21:41
@syranide
Copy link
Contributor Author

@zpao For posterity, this was actually a sneaky side-effect of #1495 (and not an issue with the initial fix). Before that, the TextNode could never be a single char (the prefix char) as there had to be white-space after it.

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.

2 participants