bmcmahen / react-wysiwyg

retain some control over contenteditable input
169 stars 37 forks source link

Possible race condition on input event support detection? #16

Closed dtuite closed 9 years ago

dtuite commented 9 years ago

Support of the input event is detected simply by setting a flag the first time it's encountered in the #onInput event handler.

  onInput: function(e) {
    this.supportsInput = true
   // ...
  }

This flag is then used in the #onKeyUp event handler to determine whether or not the logic in that method should run.

  onKeyUp: function(e) {
    if (this.supportsInput) return
   /// ...
  }

However, if the first ever keyup event occurs before the first ever input event then the logic in #onKeyUp will run even though the browser supports input.

It would be better to detect support for the input event earlier in the execution process. Potentially like this.

dtuite commented 9 years ago

There's some housekeeping in those commits. The last commit is the important one.

There's a failing test caused by this change because #onKeyUp is now untestable since if (supportsInput) return is always executed when you run tests in the browser. The only way I can think to fix this is to support initialising the component with an override which forces #onKeyUp to execute fully. Thoughts? I'll implement that if you agree with the overall idea.

I'm not exactly sure what the isServer variable is for? How would this library function anywhere except in a browser?

bmcmahen commented 9 years ago

Good point about the potential for the first keyup to occur prior to the first input. Have you had a chance to test if oninput in window returns false in Internet Explorer? I was finding that a similar test would return true, even though oninput isn't supported on contenteditable elements in IE.

Server support is necessary if you want to do any server-side rendering using this module, which I do. Actually, that will be another problem with testing oninput in window since window won't exist on the server.

Thanks for the work though! -- I'll test out a few things before I merge.

dtuite commented 9 years ago

I haven't checked Internet Explorer. I don't have easy access to IE today unfortunately. If we find that this test is a false positive in IE then I'm sure we can find a more resilient way to sniff for input support.

While you might want to render the component on the server, the actual input event or keyup event can never be triggered on the server right? That means you don't need to be able to accurately detect event support on the server. Correct me if I'm wrong please, I've never used React for SS rendering.

bmcmahen commented 9 years ago

That's right, but you don't want it to throw on the server either if window isn't defined.

bmcmahen commented 9 years ago

I saw that you put it inside of the !isServer conditional. Awesome. Now to test it in IE...

bmcmahen commented 9 years ago

Unfortunately the oninput in window gives a false positive in IE. See here

For now I've reverted to the old (crappy) method of detecting input support.

Not sure how we can detect missing oninput support in IE.... I'm definitely open to suggestions.

dtuite commented 9 years ago

Here's how Modernizr do it. it uses this hasEventfunction. I haven't tested if that supports IE but I presume it does since that's Modernizr's remit.

Of course, whether or not it's worth including that code is another matter. I presume you could compress it's complexity by making it less general.

bmcmahen commented 9 years ago

Nice -- I'll give this a try.

dtuite commented 9 years ago

I just noticed I accidentally commented out the window.requestAnimationFrame line in your example code when I committed this. Hope I didn't break anything.

bmcmahen commented 9 years ago

Good catch. Looks like it should be reenabled for the demo to fully work.