OnChange drops user input with React 15 in IE11

[sfnelson]

https://github.com/facebook/react/issues/7027

React 15 uses a polyfill for change events in IE11 that relies on onpropertychange events that unlike most input-related events are asynchronous. They've decided that this was a bad idea and use a different approach in master, but this issue is a big deal for om users.

If change event consumers (like om) asynchronously set component state then it's possible to race the property change events and drop user input. For example:

1. user types 'a' into a text field, which
   • cause browser to sets field value to 'a', and
   • schedules onpropertychange event
2. onpropertychange event fires, which
   • calls react on change, which
   • calls om on change, which
   • calls (om/set-state owner {:value target.value}) which
   • sets pending state and schedules a re-render to update the state to {:value "a"}
3. user types 'b' into the text field, which
   • causes the browser to set field value to 'ab', and
   • schedules onpropertychange event
4. render fires, which
   • sets field value to 'a' (new-state)
5. onpropertychange event fires, which
   • calls react on change, which
   • calls om on change, which
   • calls (om/set-state owner {:value target.value}) which
   • sets pending state and schedules a re-render to update the state to {:value "a"}

Note that because step 4 fired before step 5 the user's second character (b) is lost.

This issue is not really om's issue, but the combination of react's bug and om's asynchronous state updates means that om users cannot support IE 11 as no workaround is possible within these constraints. I'd strongly recommend keeping this issue open until om depends on a version of react that addresses this issue, and possibly even reverting om's dependency to react 14.

See https://github.com/facebook/react/pull/5746 for the upstream fix. Note that this still affects the 15 branch that om depends on.

[swannodette]

@sfnelson so it sounds like something that will be fixed upstream eventually?

[sfnelson]

It depends what you mean by fixed upstream. It's already been addressed in the development branch that will become React 16, but as it's a pretty fundamental change to the way events are generated in IE browsers I think it's unlikely that the fix will be back-ported to the React 15 branch.

In the mean time, an application that uses om to implement, e.g. a login form, will see input dropped even if they type at a moderate speed. In our testing this issue was so reliably reproducible that typing "abababab" resulted in "aaaa". We've pinned React to React 14 as we could not find a workaround that allows us to use React 15 and om and support IE 11.

Note that React users will seldom encounter this issue as it only shows up when state updates are asynchronous, which is not idiomatic for react.

Happy to discuss this with you further if you have more questions.

[swannodette]

@sfnelson makes sense but it also sounds surmountable by just creating your own custom input field no and avoiding the Om provided ones in this case (i.e. hard coding a lot of special logic for IE11)? Using Om's stuff here was never a requirement really - I only provided them so that controlled input behavior could be simulated just like all the other various things we try to simulate but don't actually replicate.

[sfnelson]

That's true, if React 15 was a strict requirement then it would certainly be an option but it would require reimplementing all of the polyfill stuff that React already does in order to paper over browser differences -- there are a lot of non-trivial edge cases like autofill and copy/paste that behave quite differently between browsers, and one of the big advantages of using React managed components is supposed to be that they handle that stuff for you.

Unless you had a big team of testers available I think you'd be better off going the more traditional un-managed route with value polling, which is a pretty poor user experience compared to react managed components.

[sfnelson]

I guess my point here is that om's dependency on React 15 plus it's asynchronous state update behaviour is setting users up for failure. IE 11 has significant market share so this is quite likely to affect a lot of om projects, and it could be hard to work out what causes it. I see several options for om:

• Don't do anything, it's an upstream bug (frustrating for users)
• Downgrade the react dependency to 14 until 16 is released (maybe easy?)
• Change om's state updates to be synchronous (hard)
• Implement an om-specific polyfill for IE 11 managed inputs (hard)

It's a hard call to make, I don't know what the right answer is.

[sfnelson]

If you'd like to go the polyfill route, I would be open to helping.

[swannodette]

You're thinking about something far more complicated than I am. The existing inputs are hacks over existing React inputs not rewrites. I'm suggesting writing a different set of hacks for 15 inputs only for IE 11. The IE event model is quite clear you may need to just register something guaranteed to go after that only runs for input components (and they skip the normal async render so we have a guaranteed order). It'll be ugly but whatever.

[swannodette]

Sorry on phone didn't mean to close.

[sfnelson]

If by 'hacks over existing React inputs' you're talking about wrap-form-elements, then that's only incidental -- the problem is fundamental to the om render pipeline. This issue is triggered by the asynchronous state update implementation provided by om.core/specify-state-methods! used in conjunction with js/React.DOM.input.

If my understanding is correct, this problem would occur with any om widget that relies on the om render loop to propagate state changes, so I think a work-around would require forcing a synchronous render on state change, which is inherently far more complicated than swapping out an input component for IE 11.

[sfnelson]

For my sanity I'm going to spell out the problem again as I understand it:

1. js/React.DOM.input uses an asynchronous browser event to trigger onChange when running on IE 11
2. om's set-state! relies on the asynchronous render loop and merge-props-state to push component state back into the dom.

The issue is the potential for data races between these two asynchronous steps required to perform an update (1 reads from the dom without writing, 2 writes to the dom without reading). Fixing this issue requires eliminating the potential for data races.

• React 16 will do this by making 1 synchronous
• A React 15 workaround must make 2 synchronous, or break the race by reading and writing in 2.

Is that consistent with your understanding? Is there an obvious extension point for om widgets to allow input state change handling in place of merge-props-state in the render loop?

[swannodette]

All I'm suggesting is forcing a deterministic ordering. I don't see how the async rendering poses any problems. Always render after the IE event if there's an IE 11 input. The render loop is not a hard thing anyway and JS is single threaded. Seems plenty doable to me.

[sfnelson]

I don't see how the async rendering poses any problems.

I'm not convinced that you understand the problem. The javascript event doesn't include a value, the event hander must read target.value from the dom when it fires. There is no way to guarantee that the javascript event fires before a queued render writes value and stomps on the user's input (assuming React 15).

Either the javascript event must be fired synchronously so no other updates occur between the user typing the input and the event reading the input into state (React 14/16), or the render must make sure it doesn't stomp on input that hasn't yet been captured as state.

Perhaps I don't understand what you mean, could you run through the sequence of events required to process a user input with the model you have in mind?

[sfnelson]

I'm worried I've missed some crucial point that you've seen and I haven't that results in a trivial fix to this issue. Please enlighten me, I'd love to be able to use React 15 :-)

[seltzer1717]

I'd suggest the simplest possible application that demonstrates the problem. Sometimes code conveys more than words.

[sfnelson]

@seltzer1717 I don't think a code example will help you understanding this bug because the problematic behaviour is platform-specific, timing sensitive, and inherent to the library. Any use of managed inputs in IE11 will exhibit the behaviour, e.g. https://github.com/omcljs/om/tree/master/examples/input

[sfnelson]

@swannodette I've reread through your comments, I think the bit I missed was:

they skip the normal async render so we have a guaranteed order

So that makes sense, a sequence of events would be:

1. user types input "a"
   • browser update input value
   • browser schedules a change event
2. some time later onChange fires, Om widget
   • reads target.value => a
   • calls set-state (or user-provided onChange handler)
   • calls componentWillUpdate
   • calls render (etc)

By joining together the event hander and the render we're breaking the race by ensuring that read/write happen in the same contiguous execution. If another event happens in the mean time you'll capture that change as well in step 2, followed by another onChange which will be a no-op.

How would you treat a 'normal' render? Would you roll the target.value reading into render so every render has the potential to fire an 'onChange' state update in order to process state before rendering?

Is this something you'd like to add to Om or something you're suggesting users write for themselves?

[sfnelson]

I've spend some more time testing and debugging this issue and I'm not sure that I understand it sufficiently to offer any insights into how to solve it with React 15.

I thought it would be possible to identify and capture pending changes in react state on render so that when the eventual onChange event fires they can be resurrected.

In practice, react doesn't even fire the change event in the problematic cases, so my explanation of the bug is wrong. If you interleave asynchronous renders with input events then the change events simply don't fire. I think it relates to React's use of getters and setters internally to suppress duplicate events, but I don't understand their approach well enough to present a coherent explanation.

It's possible that your approach of adding a render to the change handler will work. I think it would still be vulnerable to data races with incoming props, but I also think that's true of React's IE event handling in general, so it might be good enough.

[ducky427]

I don't know if its relevant to this issue, but a similar bug was found in Reagent as well reagent-project/reagent#253

Also exists in tonsky/rum#86.

Sorry for the noise.

[deseman]

+1

[MrBemd]

+1

[wstaat]

+1

[swannodette]

Locking this conversation. The information has been communicated. If someone wants to work on this get in touch in the Clojurians #om Slack channel.