Abnormal workflow prevents onChange to be fired in IE 8

[blainekasten]

I've included a test link:

http://jsfiddle.net/53n8kgu3/10/

(an alert should happen when you change a select item)

Essentially, i'm letting my back end rendered DOM, drive what should be converted to React Components as I don't have the freedom to render on the back. But, when following this route, it works fine always except for in IE8, onChange events aren't fired.

I think it may have something to do with the fact that i'm calling React.createElement with a component instance instead of a string. So if that is the case, it either should reject the component instance, or the events should trigger properly.

[blainekasten]

Additional tests seem to allude to other events not being triggered also. I've experienced an onBlur event not being triggered in IE8.

[kendagriff]

I'm also seeing unfired onChange events under IE8. No real insights, either.

[krasimir]

@blainekasten @kendagriff did you manage to fix your problem?

I have the following code:

var MyComponent = React.createClass({
  changed: function () {
    console.log('changed');
  }, 
  render: function() {
    return (
      <div>
        <select onChange={this.changed}>
          <option value='a'>a</option>
          <option value='b'>b</option>
          <option value='c'>c</option>
        </select>
      </div>
    );
  }
});

React.render(<MyComponent />, document.querySelector('body'));

And I don't get the changed in the console under IE8. Testing with 0.13.3 version.

[kendagriff]

Thanks for checking in... no, no progress. Can't say I was using onChange in any special manner, and it works flawlessly in IE9+. React 0.13.3.

[krasimir]

Just for the record: in the beginning I thought that this may be a problem in React but later today we found that one of our polyfills causes the issue. It was an addEventListener polyfill for IE8. So, once we remove it everything gets back to normal.

[kendagriff]

That could definitely be our problem too. I'll investigate.

[krasimir]

Note: everything that touches prototypes may lead to similar issues.

[sophiebits]

Yeah, we use a feature test for addEventListener. Are there common polyfills that set it in old IE?

[krasimir]

I first tried with this one. However, it broke React's onChange event handling. I guess it is overwriting too much of the original js methods. I tried the official polyfill at MDN. That didn't work either. So in the end the code that was causing an error was transformed to:

if (window.addEventListener) {
  window.addEventListener(type, this.handleEvent);
} else {
  window.attachEvent('on' + type, this.handleEvent);
}

... and the polyfill removed completely.

[sophiebits]

@blainekasten Your fiddle works for me with IE11 in IE8 mode. You can still repro the problem with that?

[nosir]

Yes, this can still be reproduced on native IE8

[blainekasten]

Due to recent blog post, I'm going to just close this issue out for you guys :)

[mnpenner]

Thank you @krasimir. I just encountered this problem and it was due to ie8.js. I would have never figured that out if you didn't point it out!

[krasimir]

@mnpenner you are welcome ;)

[WebReflection]

and it was due ie8.js

no it wasn't.

If you replicate this simple logic in standard DOM ie8.js works like it should.

I don't know why React would fail here but it has not much to do with ie8.js and it won't be fixed since there won't be official support for IE8.

I'd rather let IE8 go and rest in peace OR I'd use standards insead of a framework that doesn't support IE8.

[sebmarkbage]

Going to reopen this to track. The OP closed the issue, so we didn't look into it. Wasn't aware of it. Nobody on our team chose to ignore it. I'll need to dig into this. I want to understand why this goes wrong so that we can explain this better in documentation etc.

Normally we recommend loading polyfills BEFORE the library but it seems like to fix this we have to load it after. We've also used shams for things like Object.create.

It's fair that maybe React should do more specific checks, but this isn't how we normally does things. It's new to me, so maybe it is a more systemic problem than just this bug. I.e. maybe we need to recommend/do things differently in more places.

[sebmarkbage]

We could fallback to using attachEvent like we normally do but actually most things just works with the IE8.js polyfill so it would be even nicer if we could use the polyfilled addEventListener.

If we used the polyfilled version we'd get consistent bubbling order with other things on the page since we'd go through the polyfill's event dispatching. That's a nice feature to have and why we normally recommend putting the polyfills first to guarantee such whole page consistency.

It turns out that this bug happens because we're expecting to be able to listen to focus (which is used to set up the change special case).

react/src/renderers/dom/client/ReactBrowserEventEmitter.js

Lines 292 to 302 in 5696ccf

	if (isEventSupported('focus', true)) {
	ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
	topLevelTypes.topFocus,
	'focus',
	mountAt
	);
	ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
	topLevelTypes.topBlur,
	'blur',
	mountAt
	);

We use the capture version of focus because it allows us to implement event delegation (it propagates unlike the bubble phase).

Since focus doesn't bubble, IE8.js doesn't pick it up. One solution would be to fix this in IE8.js to listen for focusin instead of focus when capture is used - as a special case to simulate this strange but standard behavior of focus. This would fix this bug and it would also make this trick work for anyone else using IE8.js.

However, we're also going to fix it in React by prioritizing the focusin event if it is available. That works around this limitation in the polyfill but also seems like a good move since focusin is on the standards track and is just as good.

[WebReflection]

I start with a big thank to @sebmarkbage

Normally we recommend loading polyfills BEFORE the library but it seems like to fix this we have to load it after. We've also used shams for things like Object.create.

I't not mutually exclusive, with my proposed guard for the runtime feature detection I could happily write this on the header and make everything work, letting React use the attachEvent logic.

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="content-type" content="text/html; charset=utf-8">
    <meta name="viewport" content="width=device-width,initial-scale=1.0,maximum-scale=1.0,user-scalable=0,minimal-ui">
    <!--[if lte IE 9]>
      <script>(function(f){window.setTimeout=f(window.setTimeout);window.setInterval=f(window.setInterval)})(function(f){return function(c,t){var a=[].slice.call(arguments,2);return f(function(){c.apply(this,a)},t)}});</script>
      <script src="//cdnjs.cloudflare.com/ajax/libs/es5-shim/4.5.7/es5-shim.js"></script>
      <script src="//cdnjs.cloudflare.com/ajax/libs/es5-shim/4.5.7/es5-sham.js"></script>
    <![endif]-->
    <script src="react-0.14.7.js"></script>
    <script src="//fb.me/react-dom-0.14.7.js"></script>
    <!--[if IE 8]><script src="//cdnjs.cloudflare.com/ajax/libs/ie8/0.3.2/ie8.js"></script><![endif]-->
  </head>
  <body></body>
</html>

Recommendations for polyfills always have a top down order. Writing that addEventListener patches should have been put after wouldn't have hurt much. Headers are always a one-off effort but yeah, I totally agree that it would be nicer if React could work with ie8.js and its capabilities.

Although ....

One solution would be to fix this in IE8.js to listen for focusin instead of focus when capture is used

That's not exactly how polyfill works. If the developer wants to use focus and focusin for whatever reason the developer has all rights to be able to set these two different events without intefeering.

Being IE8, it would be actually bad to put undesired logic in the midle ... after all, everything IE8 related is based on hacks specific for IE8 and my polyfill is no exception.

Of course I like the fact you managed to find a substitute for the capturing and if this wouldn't have been fixed in React I could have thought about a way to swap focusin with focus or try to force always focusin but again, that would be a mess in terms of polyfill responsibility.

As last thought: please bear in mind ie8.js capturing phase is an artifact so I hope no other special cases would be involved in the process because it's not 100% identical to native, it cannot, unfortunately, be.

To sum it up: thanks again for investigating and for finding probably an even better solution than the one I've proposed.

Best Regards

[aweary]

I'm going to close this since there's nothing actionable here and this issue is very stale.

[sebmarkbage]

@aweary I left a comment with something we were supposed to do. Did we already do it and therefore this issue is solved? Or should we still do that, if so, it seems actionable.

[aweary]

@sebmarkbage after #9285 we no longer listen for focusin at all, so it didn't seem actionable. I'm not sure it would make sense to go back in order to support a third-party polyfill for a browser that is far out of support.