Fixed bug with updating TextNode in IE8 (#7824)

[mnpenner]

Fixes #7824

[facebook-github-bot]

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email [email protected] with your details so we can update your status.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[syranide]

That's wrong, it will render the HTML as text rather than HTML. EDIT: Oh, now I see #7824.

[syranide]

It's bothersome to reproduce this because you can't run jsfiddles on IE8 (IE8 emulation is not the same as IE8!), so I don't have the time right now. However, because TextNodes doesn't have innerHTML in any browser then this should apply equally to all browsers I would assume, however other browsers do allow you to set innerHTML (but it just won't show), so it's possible that this is a bug that isn't showing in other browsers.

So this behavior should definitely be investigated, but I'm quite sure that this PR isn't a correct fix.

[mnpenner]

Well, I do have a real IE8 VM, I just hate firing it up because it doesn't even have a proper debugger.

I managed to reproduce it properly now.

I've hosted a unadulterated copy of the demo here.

I had to run all the libs through webpack to get it to run in IE8. Here's what's included:

• core-js/es5
• core-js/es6
• react
• react-dom
• Hello.js

I'm not sure why this bug doesn't affect newer browsers -- is this handled further up the callstack for them? Maybe they don't even enter this setInnerHTML function.

[mnpenner]

I switched back to IE11 emulating IE8 so that I could get a callstack. It actually goes through here first:

var setTextContent = function(node, text) {
  if (text) {
    var firstChild = node.firstChild;

    if (firstChild && firstChild === node.lastChild && firstChild.nodeType === 3) {
      firstChild.nodeValue = text;
      return;
    }
  }
  node.textContent = text;
};

if (ExecutionEnvironment.canUseDOM) {
  if (!('textContent' in document.documentElement)) {
    setTextContent = function(node, text) {
      setInnerHTML(node, escapeTextContentForBrowser(text)); // <-- gets called
    };
  }
}

You'll note that it only falls back to setInnerHTML if textContent is not available, which is true of IE8 but not newer browsers.

So, new browsers should use node.textContent = text and my patch will only affect browsers without textContent.

[syranide]

@mnpenner Ah now that makes a lot more sense :) excellent digging. So setTextContent should have a special-case for TextNodes and for those set nodeValue directly.

PS. Also, you messed up the PR when you pulled/rebased.

[mnpenner]

@syranide Yeah...sorry. Totally different workflow at my company. Don't know how this Git + Github stuff works 😝 I was trying to revert my code to facebook/react master so I could try again.

Would we want to put that check in the fallback setTextContent function then, since the original is overwritten?

It seems like it should be in the original too, but it never gets hit AFAIK.

[syranide]

@mnpenner Absolutely no worries, just wanted to give the heads up :)

Would we want to put that check in the fallback setTextContent function then, since the original is overwritten?

Yeah, so the fallback setTextContent should check if node is a TextNode, if it is then it should set with nodeValue, else it should use setInnerHTML as it already does. setInnerHTML should not be changed because it's not meant to act on TextNodes.

[syranide]

As for the "original" setTextContent it is not needed there, TextNodes has textContent as well. So yeah, there should technically be a branch for nodeValue and one for textContent, but in practice it is not needed.

[syranide]

Nit, but this wasn't really necessary :)

[mnpenner]

PhpStorm did that 😢 I noticed after I pushed.

[syranide]

Looks good to me 👍

cc @jimfb

[syranide]

Oh sorry, the PR is still messed up.

[mnpenner]

Uhhh...hate to ask, but do you know the Git commands to set my repo back to a "clean" state so I can try again?