Avoid "Member not found exception" in IE10

[nhunzaker]

In <= IE10, custom change events raise "Member not found" when accessing 'cancelBubble' . To circumvent this, the SyntheticEvent class now checks for "typeof event.cancelBubble !== 'unknown'". This eliminates this exception and maintains the expected bubbling functionality.

I'd never heard of typeof x === 'unknown' before, but it seems to be a goofy JScript thing. I discovered it in the jQuery issue tracker:

https://bugs.jquery.com/ticket/10004

I've confirmed that this preserves bubbling functionality from IE9-11, Safari, Chrome, and Firefox.

This technically is not a problem on master, but it fixes the issue for 15-stable and I'm sort of curious what other events have this same issue.

[nhunzaker]

Looks like this fails because eslint won't allow typeof x === 'unknown' :-/

~/react/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
  138:46  error  Invalid typeof comparison value  valid-typeof

Any suggestions?

[syranide]

typeof x === 'undefined' you mean?

[nhunzaker]

I do mean typeof x === 'unknown'. This seems to be a browser quirk with IE:

https://bytes.com/topic/javascript/answers/515951-typeof-operator-returns-unknown-instead-undefined

I tried to get around this by doing "cancelBubble" in event, but that explodes in IE9.

[syranide]

What the... perhaps eslint is ok with it if you assign "unknown" to a constant and compare with that instead? If the warning can't be disabled (make an issue for eslint perhaps?).

[syranide]

I'm seeing conflicting information on this from every source (what and which browsers are affected) and I haven't been able to find any code to actually reproduce it. Do you have it?

[nhunzaker]

It's definitely bizarre. This issue was filed in #7320. I realize that I never linked it up.

The following gist will reproduce the problem:

https://gist.github.com/nhunzaker/79d48674f543a3bf0977667c2513e9ac

IE9

Likewise, entering text in the "Stops propagation" here will raise the same exception:

IE10

Entering text in the "Stops propagation" input will raise a "Member not found" exception:

IE 11, Edge 13, Edge 14

Gets it right:

---

I haven't figured out a way to reproduce this outside of React. As you've mentioned, the documentation is fuzzy at best. I think there might be something going on in LinkedValueUtils.

[nhunzaker]

Okay. Here we go. For whatever reason in IE9-10, a ton of properties are not found in the event backing the SyntheticEvent dispatching change:

I wish I could provide a spec on how IE treats these events. When I create a custom event:

>> var evt = document.createEvent("Event");
evt.initEvent("change", true, false);
console.log(evt.cancelBubble); 
 false 

Seems fine. I totally get the aversion to committing a hack like typeof x === 'unknown' until the problem is better understood. I'm going to keep digging in

[syranide]

From some experimentation it seems its the MSEventObj base class that has this problematic behavior. So event.nativeEvent instanceof MSEventObj actually seems to identify these events correctly, if there are false positives or vice versa I don't really know (but I doubt it).

Also, FYI for anyone trying to understand this. This is because we're listening to the propertychange event, which I guess is special for some reason.

Seems fine. I totally get the aversion to committing a hack like typeof x === 'unknown' until the problem is better understood. I'm going to keep digging in

IMHO, I don't really see an issue, it works and it's precise. If eslint is the only issue then working around it seems fine to me at least, either by comparing to a constant (if that works) or putting in a separate file and disabling eslint in that file.

On another note (and I may be totally off here), but I'm curious why we even stop propagation of native events at all, they're all document listeners so unless I'm mistaken nothing good comes from cancelling them in the first place? We might still want to change that in the future, so it makes sense to fix it... it just struck me as odd.

[mxstbr]

or putting in a separate file and disabling eslint in that file.

Chiming in, no need for a separate file, this should work:

// eslint-disable-next-line valid-typeof
if (typeof x === 'unknown') {
  /* … */
}

(if valid-typeof is the correct rulename)

[nhunzaker]

@mxstbr Beat me to it! Done.

[alexzherdev]

For completeness:
if (typeof x === 'unknown') { // eslint-disable-line valid-typeof

[nhunzaker]

On another note (and I may be totally off here), but I'm curious why we even stop propagation of native events at all

Did a quick test, and my initial impression is that we don't need to invoke them directly at all. It seems like the only reason to do it would be so that the native event reference indicates that stopPropagation has been applied.

[syranide]

Perhaps clarify that this is because of "propertychange" and not because of actual change events.

[nhunzaker]

Got it:

      // The ChangeEventPlugin registers a "propertychange" event for
      // IE. This event does not support bubbling or cancelling, and
      // any references to cancelBubble throw "Member not found".  A
      // typeof check of "unknown" circumvents this issue (and is also
      // IE specific).

[nhunzaker]

Alright. I think I'm set!

[nhunzaker]

Anything else to follow up on here?

[syranide]

I'm assuming this fails the lint line length rule?

[nhunzaker]

Technically no, though it's against the React style guide all the same.

ESLint seems to struggle with else if. The only other valid option I can manage is:

if (event.stopPropagation) {
  event.stopPropagation();

  /* eslint-disable valid-typeof */
} else if (typeof event.cancelBubble !== 'unknown') {
  /* eslint-enable valid-typeof */
}

Or we could flip the if to place typeof event.cancelBubble before the event.stopPropagation check.

I know this is annoying and silly. Thank you for your patience on this.

[syranide]

I'm care-face on this (not my decision). If it's a problem just leave it as is and they'll complain if need be.

[syranide]

I fail to see how this can be harmful (EDIT: The PR that is). The only thing that strikes me as odd I guess is that this doesn't really solve the problem, it just prevents an error right? So technically, you can stop propagation of onchange with this, but in IE<=10 it will not actually do it?

It might make sense to throw a warning or something as well perhaps?

[nhunzaker]

It works as expected. The bubbling is handled by the synthetic event system. Best I can tell, the whole reason for the stopPropagation logic is for interaction with code that might be listening to to the DOM nodes themselves (from a lifecycle hook or something).

Given the following code:

function onChange (event) {
  event.stopPropagation()
}

ReactDOM.render((
  <div onChange={ () => alert('Noooo!') }>
    <input defaultValue="hey" onChange={ onChange } />
  </div>
),document.getElementById('container'))

It behaves as one would expect. The onChange callback on the parent is never fired. If you remove event.stopPropagation() in the onChange callback, you'll see the alert. This is consistent from IE9 to Edge, Chrome, Safari, and Firefox.

[syranide]

The bubbling is handled by the synthetic event system.

Oh right right, that still happens. Yeah, then I have nothing to object to :)

👍

[syranide]

cc @zpao @spicyj any objections?

[sophiebits]

Okay, thanks!

[nhunzaker]

Hizzah!

[gmp]

I believe this only fixes half the problem. This same logic will also need to be added to preventDefault as well, correct? I'm running into this problem now and adding similar logic to the preventDefault handler as well fixes the issue.

[nhunzaker]

@g-palmer Cool. I can send out a PR with this change, unless you would like to.

[gmp]

@nhunzaker If you don't mind, I'd like to go ahead and make the change real quick

[nhunzaker]

I don't mind. Go for it!