Migrate input value change fix from 16.0.0-alpha

[nhunzaker]

This PR backports work done by @jquense to fix several event issues for React 15.x. Additionally, addressing issue #3659. It is a blend of his work with newer updates from master.

Should fix #3659, and a couple of others.

[jquense]

hey there, So the reason this change is in 16 is because it has some breaking changes in it. My changes were actually done before 15 was released :P but deferred to 16 intentionally

[nhunzaker]

Ah bummer. Think the only resolution for issues like #3659 is to simply wait for React 16?

[aweary]

Closing since we're not going to backport any breaking changes 👍

[jquense]

I've mentioned this before in other places but this need not be a breaking change.

[nhunzaker]

@jquense I'm interested. What do you have in mind?

[aweary]

Sorry @jquense, I was basing this on your comment

So the reason this change is in 16 is because it has some breaking changes in it

Can you link to previous discussion on how to make this non-breaking?

[jquense]

sure thing here it is: #5746 (comment)

The more i've sat on this I don't actually think this is even a reasonable breaking change it makes simulateNative impossible to use with onChange

[nhunzaker]

@jquense Interesting. Actually I think I may have bumped into a similar thing in #7642 with mouse events. Simulated events don't pass through the event plugin hub.

Are you interested in revisiting this? Otherwise I'd be happy to dig in.

[sophiebits]

Simulated events don't pass through the event plugin hub.

Not having looked at #7642, this sounds intentional. We don't want to go through the code that normalizes events for different browsers – that's exactly the difference between Simulate and SimulateNative.

To fix the issue here, would it make sense to track the current value on _wrapperState for each ReactDOMInput? Then you can detect changes when receiving a change or input event even if it is not focused. Sorry for just dumping a rough idea here but I have been meaning to look into this for weeks and keep not finding time. :(

[jquense]

its definitely intentional that Simulate doesn't pass through the plugins, but SimulateNative should. Right now it passes through just fine, but the value tracking logic means that a simulated event will always be swallowed by the changePlugin, because the value isn't changing and so it considers it a duplicate event and silences it.

The PR (from me) that this is backporting, does solve the issue of needing focus on inputs for them to work, by doing something similar to moving the value state to _wrapperState. The consequence though of deduping any change tho is that simulated native events cannot work.

consider this example:

  let inputNode = renderInput(<input  onChange={changeSpy}/>);

  inputNode.value = 'foo'; // internal state tracking sets "current" value to 'foo';

  SimulateNative.change(inputNode); 

The changeSpy will never fire, becasue when the change plugin sees the incoming onChange event, it checks that target.value does not equal the internally tracked last value since the value hasn't changed from 'foo' when we set it the change event is swallowed

[jquense]

you can see the issue here: https://github.com/facebook/react/pull/5746/files#diff-2bead3a73555ca476b3dedf1f31fbb93R81

and the moved state tracking logic here: https://github.com/jquense/react/blob/669e3dff96234ff4ec5626b93f50877b8889444d/src/renderers/dom/client/inputValueTracking.js

hooks into DOMInput here: https://github.com/jquense/react/blob/669e3dff96234ff4ec5626b93f50877b8889444d/src/renderers/dom/shared/ReactDOMComponent.js#L486

The older ie8-9 logic was left alone, to not stir the pot anymore :P

[sophiebits]

Is suppressing events due to setting .value the only reason we need the .value setter? I wonder if we can get rid of it. Maybe we need it though since you could imagine setting .value from within an onChange. :|

[jquense]

Is suppressing events due to setting .value the only reason we need the .value setter

i wish :/ but no, if you don't track imperative value sets but also try and dedupe the events you run the risk of missing legitimate value changes. Consider:

• type 'foo' into an input
• set value: input.value = 'bar'
• paste 'foo' into the input

The last foo won't fire because the internal tracking never saw that the value changed. Granted that's an unlikely case, but the cost of missing any value change is high, as seen by the slew of IE issues about it

I thought maybe if react just said "no" to supporting imperative value sets we;d be ok removing that, but you still run afoul of autocomplete, password managers, etc.