Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OnChange randomly misses keys on IE if typing very quickly #7027

Closed
pasieronen opened this issue Jun 13, 2016 · 54 comments
Closed

OnChange randomly misses keys on IE if typing very quickly #7027

pasieronen opened this issue Jun 13, 2016 · 54 comments

Comments

@pasieronen
Copy link

I have a simple search bar (controlled text input) with autocomplete.

When I type text very quickly, I do get onKeyDown events, but every now and then, the corresponding onChange event goes missing. For example, if I type "foobar", the input field may end up showing "foobr".

This happens only on IE (tested with IE11 on Windows 7), and I have not seen this on any other browser.

Here's a fiddle showing the issue: https://jsfiddle.net/zjfbow3w/3/ and a log:

keyDown:  f
keyDown:  o
change:   f
render:   f
change:   fo
render:   fo
render:   fo
keyDown:  o
keyDown:  b
change:   foo
render:   foo
change:   foob
render:   foob
keyDown:  a
render:   foob
keyDown:  r
change:   foobr
render:   foobr
render:   foobr

Note that onKeyDown handler sees that "a", but not the onChange handler.

The fiddle includes a timer (which in real code triggers an AJAX call), which is somehow important for this issue -- if I remove the timer, I cannot reproduce this issue any more.

The React version in the fiddle is 15.1.0 (but this happens on 0.14.3, too).

@syranide
Copy link
Contributor

It's really weird, I don't see anything wrong with your code... this should not happen. So this should indeed be a bug. I can't really wrap my head around how or why this behavior is happening, will be interesting to see what's causing it.

Really interesting.

@syranide
Copy link
Contributor

cc @jquense I'm sure you'll love this ;)

@jquense
Copy link
Contributor

jquense commented Jun 13, 2016

interesting... @pasieronen could you possibly try using React master and see if it's still a problem? i also wonder if it happens on older ie..hmm

@syranide
Copy link
Contributor

IE11 affected.
IE9, IE10 and Edge unaffected.

@jimfb
Copy link
Contributor

jimfb commented Jun 13, 2016

It looks to me like IE11 is setting the value as soon as the user types a keystroke, and enqueuing the event handler. As a result, the timeout callback calls setState, which causes a reconciliation, which resets the input value, which causes the onChange handler to read the wrong value from the DOM node.

Notice that if you replace the onTimeout logic with this.setState({unused: 1, value: this.refs.input.value}), the input starts working as expected again.

To me, this looks like a bug in IE (setting the value should happen immediately prior to calling onChange) rather than a bug in React.

@syranide
Copy link
Contributor

syranide commented Jun 14, 2016

To me, this looks like a bug in IE (setting the value should happen immediately prior to calling onChange) rather than a bug in React.

@jimfb onChange is polyfilled so that seems like an odd statement? Or am I missing something?

Anyway, after some debugging I don't really feel a lot smarter. However, it seems the root of the problem may be related to the keyup/keydown/etc handling of ChangeEventPlugin in a sense. To accurately reproduce the problem, hold down a key and then press a different one without letting go of the first and you have reproduced the problem, which may further hint at the underlying problem. It's also worth noting that in that instance, logging onInput will actually output the correct value (before being overwritten by React) and if you update the state with it will turn out correct. On another note, anyone know why we aren't using onInput for IE11?

@jimfb
Copy link
Contributor

jimfb commented Jun 14, 2016

@jimfb onChange is polyfilled so that seems like an odd statement? Or am I missing something?

Sure, I should have more correctly said: The native event that causes our onChange handler function to fire.

On another note, anyone know why we aren't using onInput for IE11?

cc @jquense

@jquense
Copy link
Contributor

jquense commented Jun 14, 2016

the most recent reason is that it fires too often and inconsistently. changing a placeholder for instance fires onInput, re-using the polyfill was a quick way to fix it though it seems to probably have cause more bugs than fixed (browsers!). master listens for both onChange and onInput and dedupe em, which is (hopefully) a more correct solution

what I've learned is that the polyfill doesn't scale super well to ie11 which being the first "good" IE seems to break a bunch of MS-isms that make it work in ie8-9. Also there are probably a bunch of these bugs in old IE too but didn't notice them because of lower market share?

@smashercosmo
Copy link

Is there any temporal workaround for that, that could be applied in the app?

@jimfb
Copy link
Contributor

jimfb commented Jun 15, 2016

@smashercosmo

I think you could update the value of your controlled component during your timeout/setState call, as I mentioned earlier...

Notice that if you replace the onTimeout logic with this.setState({unused: 1, value: this.refs.input.value}), the input starts working as expected again.

Alternatively, you could restructure your code to avoid calling setState in an asynchronous/timeout callback. I think if you call setState() only in the event handler, things will work without an issue, since the problem is a race condition between the characters typed and the setState calls.

I realize neither of those are particularly good workarounds.

I would also recommend filing a bug with the microsoft/IE11 team, since this looks like an IE bug to me. Setting the node's value and firing the event handlers should be synchronous. Other timers/events (like setTimeout) should not be injected between those two operations. This assumes that my understanding of the issue is correct. If you can reproduce the bug without React (should be possible, just do whatever React is doing, probably just setting the node's value directly in the setTimeout call would be sufficient), that will likely help the IE team narrow down the issue.

On our end, I'm not sure there is much we could do about this problem. At first glance it appears to be a browser bug, though I'd love to be proven wrong (it is conceivable this is a React bug). Assuming it is a browser bug, we might be able to mitigate the problem, but we may not be able to eliminate it entirely. In years of React, you're first person to run into this, so it is probably not the highest item on our todo list, but I do understand that it is annoying/frustrating to run into things like this. Unfortunately, browser bugs are often difficult for us to fix on our end.

@syranide
Copy link
Contributor

syranide commented Jun 16, 2016

On our end, I'm not sure there is much we could do about this problem.

@jimfb Considering that onInput does report the new value correctly intuitively this should be entirely fixable unless there's something else blocking.

Alternatively, you could restructure your code to avoid calling setState in an asynchronous/timeout callback. I think if you call setState() only in the event handler, things will work without an issue, since the problem is a race condition between the characters typed and the setState calls.

While true, that doesn't take into consideration any unrelated asynchronous updates that may happen elsewhere in the application, which would cause it to update and rerender. Outside of your control.

I would also recommend filing a bug with the microsoft/IE11 team, since this looks like an IE bug to me. Setting the node's value and firing the event handlers should be synchronous. Other timers/events (like setTimeout) should not be injected between those two operations.

I'm not convinced IE11 is really at fault here, as far as I can tell we have a hacky workaround for reimplementing oninput which may rely on very specific browser behavior of keydown/keyup/etc (keydown fires before value is updated). Determining the exact fault or adding the native oninput (which seems to work) into the mix may solve the problem. I don't have any faith in IE11 shipping a fix for this any time soon (or ever, and everyone also updating to it) and we're bound to support it for a very long time.

@smashercosmo
Copy link

smashercosmo commented Jun 16, 2016

@jimfb @syranide The thing is that react 0.14.8 handles this correctly. I have two demos with the exact code and different versions of react. One is buggy in IE11 and one is not.
buggy demo, non-buggy demo

@jimfb
Copy link
Contributor

jimfb commented Jun 16, 2016

@smashercosmo

The React version in the fiddle is 15.1.0 (but this happens on 0.14.3, too).

@smashercosmo

The thing is that react 0.14.8 handles this correctly. I have two demos with the exact code and different versions of react. One is buggy in IE11 and one is not.

Oh, weird, ok, so this was broken in 0.14.3, fixed in 0.14.8, and broken again in 15.1.0, is that correct?

I just tried on my master fiddle (http://jsfiddle.net/s2ot6w4z/) and it seemed to be fine. @smashercosmo, can you verify that it is fixed in the fiddle? http://jsfiddle.net/s2ot6w4z/

@smashercosmo
Copy link

@jimfb I confirm. Latest react doesn't have this speedtyping issue. Tested both demos (yours and mine).

@jquense
Copy link
Contributor

jquense commented Jun 16, 2016

master does listen for onInput; if IE is correctly reporting there this should be fixed. like I said above it just turns out that the polyfill we have for onInput actually has a lot of edge cases no one noticed :P (and it's assumptions don't quite work on ie11)

@jimfb
Copy link
Contributor

jimfb commented Jun 16, 2016

Ok, we close out bugs when they're fixed in master. This fix will be part of a future release.

@jimfb jimfb closed this as completed Jun 16, 2016
@sfnelson
Copy link

I know this bug has been closed by changes to master, but it's still affecting the 15 branch.

I don't think that @syranide is correct about this being an IE11 bug, he correctly points out it seems to be caused by making asynchronous updates to state, but I think the cause of the bug is that react uses onpropertychange to generate change events in IE11, which is asynchronous!

Consequently any state changes triggered in response to a change event in IE11 are racy, and may cause input to be lost.

@HankMcCoy
Copy link

I would assume that since the fix was on master in mid-June that it went out with 15.2.0 or 15.2.1? I ask because I'm still seeing this problem on 15.2.1, on a large redux-form form on IE11 machines of sufficient slowness. If it didn't go out yet, is there any estimate on when it will go out?

@jquense
Copy link
Contributor

jquense commented Jul 27, 2016

@HankMcCoy master is v16.0.0 so any v15 wouldn't include them

@HankMcCoy
Copy link

Our company works in the construction industry and therefore has a lot of clients on old machines with IE11 filling out massive forms, so this is a pretty substantial problem for us. Do you know where in the codebase the fix was applied? We're seriously considering temporarily forking in order to cherry-pick the fix for this, if that is even remotely feasible. And thanks for the ludicrously fast reply, @jquense!

@jquense
Copy link
Contributor

jquense commented Jul 28, 2016

I forget the PR number, but you can npm install react@next that might do it for you.

@HankMcCoy
Copy link

Sadly react@next is 15.3, so still no fix. :/

Is this the PR in question: #5746? I cherry picked that change onto the 15-stable branch locally and rebuilt my app with the forked version but am still seeing the problematic behavior. Are there perhaps other changes on master that contribute to fixing this that I missed?

@jquense
Copy link
Contributor

jquense commented Jul 28, 2016

I believe that's the only one. I do think that there is a releases v16 alpha perhaps.

@cstoffer
Copy link

We encounter the same issue here; just wondering when v16.0.0 is due to be released?

@PenguinOfThunder
Copy link

We are seeing the same issue. It's a pretty big deal for us since our users are on IE11 for enterprise policy reasons. This bug renders our app almost unusable to them.
Any chance of getting a fix back-ported to v15 before v16 comes out? Is there a workaround we can apply to our own code in the meantime?

@HankMcCoy
Copy link

Would just like to back up @PenguinOfThunder. Back-porting this fix to the 15 release would be immensely helpful. As it is this is screwing us over pretty royally with customers stuck on IE11 (of which there are, unfortunately, many).

@gaearon
Copy link
Collaborator

gaearon commented Aug 17, 2016

For now, the best I can suggest is to build from master yourself, or grab a nightly master build here: http://react.zpao.com/builds/master/latest/. Obviously they are not guaranteed to be stable.

(Please make your own copy, don’t link to @zpao’s website 😉 )

@JohanBengtsson
Copy link

JohanBengtsson commented Jan 18, 2017

I've tested react 16.0.0-alpha today and I can still reproduce this issue on IE11. Isn't 16.0.0-alpha based on the master branch thus having code that fixes this issue? Anyone else experiencing this?

On another note, we tried replacing onChange with onInput (which worked great for IE11) as @felixkurniawan did, but that made typing in IE10 stop completely for us.

@MacKentoch
Copy link

MacKentoch commented Jan 18, 2017

@JohanBengtsson I did not test my tempory fix on IE10 (yes like everyone I try to step away IE as much as I can 😄) but did you set defaultValue too?

Here is my TextInput custom component (= my tempory fix until React 16) working smoothly on IE11 (even with a temporised callback to manage value in redux store without performance cost):

import React, {
  Component,
  PropTypes
}                     from 'react';
import shallowCompare from 'react-addons-shallow-compare';

class TextInput extends Component {
  constructor(props) {
    super(props);
    this.state = { stateValue: '' };
    this.handlesOnChange = this.handlesOnChange.bind(this);

    this.timer = null;
  }

  componentWillReceiveProps(nextProps) {
    const { stateValue } = this.state;
    const { value } = nextProps;

    if ((value !== stateValue) && stateValue.length === 0) {
      this.setState({stateValue: value});
    }
  }

  shouldComponentUpdate(nextProps, nextState) {
    return shallowCompare(this, nextProps, nextState);
  }

  componentWillUnmount() {
    if (this.timer) {
      clearTimeout(this.timer);
      this.timer = null;
    }
  }

  render() {
    const {label, id} = this.props;
    const { stateValue } = this.state;

    return (
      <div className="form-group">
        <label
          className="control-label"
          htmlFor={id}>
          {label}
        </label>
        <div>
          <input
            className="form-control"
            id={id}
            type="text"
            // value={value}
            defaultValue={stateValue}
            onInput={this.handlesOnChange}
          />
        </div>
      </div>
    );
  }

  handlesOnChange(event) {
    event.preventDefault();
    this.setState({stateValue: event.target.value});
    this.setTimerBeforeCallback(event.target.value);
  }

  setTimerBeforeCallback(value) {
    const { onChange, delay } = this.props;

    if (this.timer) {
      clearTimeout(this.timer);
      this.timer = null;
    }

    this.timer = setTimeout(
      () => onChange(value),
      delay
    );
  }
}

TextInput.propTypes = {
  label:    PropTypes.string.isRequired,
  id:       PropTypes.string.isRequired,
  value:    PropTypes.string.isRequired,
  onChange: PropTypes.func.isRequired,
  delay:    PropTypes.number
};

TextInput.defaultProps = {
  delay: 200
};

export default TextInput;

@JohanBengtsson
Copy link

@MacKentoch Thanks for the tip! Yeah we've got it working well for IE11 now with a short term work around of changing to onInput.. but still have no good solution for IE10 and we don't want to have spend too much time on it. Unfortunately we have a lot of customer at universities/hospitals (etc.) where individuals aren't always allowed to upgrade from IE10. But either way... unwillingly we have decided to temporarily drop IE10 support 'til a proper solution in React 16 is out.

@jaibeales
Copy link

jaibeales commented Mar 23, 2017

I switched to using onInput, this then broke IE10 & IE9. The following disgusting hack is what I have ended up doing until it's fixed, YUCK.

import { IE_VERSION } from '../../util/browserDetection';

export default function getInputOnChangeProps(handler) {
    return IE_VERSION === 11 ?
        { onInput: handler } :
        { onChange: handler };
}

<input type='text' placeholder='Title' {...getInputOnChangeProps(this.handleTitleChange)} />

@frontendphil
Copy link

I'd also want to follow up to my post from August last year. It's now been 7 months of waiting :( Last year I hoped that 16.0 is not too far away. However that assumptions seems to be wrong. If this wouldn't be such a super yucky thing I'd be ok with it but I constantly have to answer the question, why we can't fix that.

@jquense
Copy link
Contributor

jquense commented Mar 23, 2017

@frontendphil maybe note that in #8575 :)

@adamsdotter
Copy link

adamsdotter commented Apr 19, 2017

I had the same problem. I solved it by using a similar solution as @MacKentoch suggested (using onInput combined with setTimeout). However, in the end all I needed to do was to remove e.preventDefault() from the onChange method to make it work in IE 11.

mthx pushed a commit to CoreFiling/react-autosuggest that referenced this issue Apr 19, 2017
There are various better sources for this fix than this repo.  See
discussion on facebook/react#7027 and various
react-autosuggest forks.

We might want to detect IE11/Edge rather than doing this across the
board but this is a start (what about IE < 11?).
jslatts added a commit to ZeroarcSoftware/carbondream that referenced this issue May 11, 2017
@bbarnell
Copy link

@pasieronen It's fixed in React version 15.6.1

@programmist
Copy link

@bbarnell Upgrading to 15.6.1 does not fix the issue for me.

@bbarnell
Copy link

@programmist My site is now working without changing any code other than the version:
http://www.greenwoodsc.gov/permits/

@tcraftdan
Copy link

tcraftdan commented Sep 15, 2017 via email

@Jarch09
Copy link

Jarch09 commented Jul 11, 2018

This seems to be broken in React 16.4.1 - I am experiencing this same bug in IE11

@svsool

This comment has been minimized.

@gaearon
Copy link
Collaborator

gaearon commented Aug 29, 2018

File a new issue with details please. Thanks. We don't track closed issues.

@facebook facebook locked as resolved and limited conversation to collaborators Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests