Only fire input value change events when the value changes

[jquense]

More aggressive alternative to #5687, This PR tracks the value for all inputs and only fires a change when the value changed. This is similar in theory to how the onInput polyfill works except it doesn't track the value in activeElement but on the input DOM node. It certainly starts to edge into "not to spec" territory, but onChange is already pretty unique in React so perhaps that's not important.

The extra state tracking isn't ideal, but it has benefits over the track-when-focused approach. For one testing is easier. detached inputs will still fire onChange events, which will break a lot less existing tests. I could also track the value on the node instance, instead of the DOM node, but I wasn't sure if how ya'll feel about mutating the component instance that way.

The downside to this approach is that manually triggered events won't fire if the value hasn't changed. As far as I can tell there isn't a way to distinguish between naturally triggered events and manually triggered ones. Not sure if it's worth it, since most are triggered in testing by the ReactTestUtils, it might make sense add a flag that way?

fixes #554, fixes #1471, fixes #2185 (that last one is probably caused by something else entirely but it was fixed in my testing)

cc: @jimfb @spicyj @syranide

[jquense]

reverts #4051 since this addresses that issue as well. Lets the polyfill be more focused since it has some downsides.

[jimfb]

@spicyj I know you had some thoughts on this topic, just wanted to raise this PR for your attention/feedback. It's nearly NYE, so maybe we won't get to this reviewed before the new year, but I do think we should fast track this PR to the extent possible :).

[syranide]

We can't really do this, updating the value of inputs via script is allowed for uncontrolled inputs and these updates would not update [LAST_VALUE_KEY]. Also, as far as I can tell, this implementation does not update [LAST_VALUE_KEY] when the value an input is updated for controlled inputs either, but that is fixable (although slightly hacky perhaps).

[jquense]

changing a value on on uncontrolled input should trigger an onChange which will trigger an update here (in text inputs anyway) since the code listens for both onChange and onInput. Missing an update should be fine since the next raised event will update the cached value

That being said, there is an inconsistency here but I think it might be the opposite one (or maybe the same). native onInput doesn't trigger when you set node.value; the IE8 pollyfill even wraps the value descriptor in order to detect and silence value changes triggered from javascript. If we need that consistency (which makes sense to me) then wrapping the value descriptor in the same way would allow for that, but I'm not sure if that causes any significant perf regressions, getting and setting all value descriptors on listened to inputs.

[jquense]

actually...it seems likes node.value = 'foo' doesn't trigger a change...which I could have sworn was not the case. Maybe there isn't any issue (from my perspective)

[syranide]

Missing an update should be fine since the next raised event will update the cached value

But it's not fine if something reverts what the user typed and he types it again, it will then think the value hasn't changed, while uncommon that would be a severe bug.

I'm also skeptical of something like this if there are edge-cases where we are still triggering when the value hasn't actually changed (which can happen if the cache is wrong). If it's not 100% then the end benefit of this seems little more than a performance optimization, IMHO a good thing but perhaps not in relation to there being a fragile element to the implementation.

Also consider that we still don't know when auto-filling occurs in some browsers, this is a troublesome area.

[jquense]

sorry slow this morning: yes failing test case

it.only('should not fire change when setting the value programmatically', function() {
    var called = 0;

    function cb(e) {
      called += 1;
      expect(e.type).toBe('change');
    }

    var input = ReactTestUtils.renderIntoDocument(
      <input type="text" onChange={cb} defaultValue="foo"/>
    );

    ReactTestUtils.SimulateNative.change(input);
    input.value = 'bar'
    input.value = 'foo'
    ReactTestUtils.SimulateNative.change(input);

    expect(called).toBe(2);
  });

[jquense]

I agree that failing to report a change is not acceptable, I also think that over reporting in some cases is probably "ok" but most of the point of making this a sweeping change is to fix those sort of minor inconsistencies so not fixing them is sort of meh.

That being said I think this approach is still salvageable. I can't update the PR till later but I have some ideas that shouldn't be fragile...

[facebook-github-bot]

@jquense updated the pull request.

[jquense]

I am not sure these hooks are appropriate for this sort of thing, but they seem like they are well suited for it, baring the below annoyance

[facebook-github-bot]

@jquense updated the pull request.

[jquense]

When unmounting the node is already gone by the time this hook is called so the getNodeFromInstance call throws.

This seems like an unfortunate break in encapsulation, the alternative was to use a try/catch but I'm mostly conditioned at this point to be afraid of try/catch VM deopts.

[jquense]

I'm not quite sure why some tests sporadically fail, this isn't happening "On My Machine"(tm).

I would have thought setting configurable to true would stop that one. poking around some more

[jimfb]

@spicyj @syranide Can someone with expertise in this area of the core take a look at this, as per #5746 (comment)? Thanks!

[syranide]

#5746 (comment) #5746 (comment) are my thoughts on this.

I'd like to see this fixed, but unless you can 100% reliably track last value (which I don't think you can) then this seems dead in the water. We could kind of do it if we drop event bubbling (IMHO yes please) but we'd still have issues with external influences and AFAIK there is no event that reliably detects changes to the value across the board, even in modern browsers.

Please correct me if I'm wrong, but that's my understanding of the situation.

[jquense]

@syranide is there any reason to think that spying on the value descriptor (along with the change/onInput events) will miss value updates since this is already the strategy we take for the polyfill? What strategy would you suggest to fix the noted issues?

I am happy to code whatever anyone wants to get these bugs fixed but it seems like the approaches I take are passed on, for the concern of edge cases. However the current code is already full of these edge cases. At a certain point we are sticking with bugs for the fear of not covering every single edge case with a fix. It seems the options are:

• try and address each edge case directly
• solve the problem at a more holistic level

I've already tried the individual approaches to fixing each bug with an a fix unique to that bug, and those approaches were rejected for want of a more robust solution that dedupes the events.

I completely understand the concern that a change like this entails, and the can of worms it's potentially opening. I am happy to adjust the scope of the PR; If this seems like too much, then what about limiting it in scope as in: #5687 (needs to be updated to essentially this code but for ranges only) which at least solves one bad bug.

I am not attached to any of these solutions, I am attached to getting these bugs fixed, so if these approaches aren't helpful or acceptable, please recommend a different approach and I will be happy to do the leg work of getting it coded. Thanks again everyone

[syranide]

What strategy would you suggest to fix the noted issues?

@jquense Based on my (admittedly limited) understanding there is no 100% and reliable strategy for this problem, that's all I'm saying. AFAIK there are cases where we simply won't know the actual last value, that seems bad. I'm all for fixing this but personally if I have to choose between:

• Don't fix; inputs sometimes fire for noop.
• Fix; inputs will fire for noop in edge-cases, inputs will fail to fire for actual changes in edge-cases, the fix involves a lot of code.

That's my understanding of the situation right now and based on that "don't fix" seems like the best candidate. Again, please correct me if I'm wrong. But I'm happy for your contributions and ultimately it's up to the FB devs to decide, I'm not intimately familiar with the React event system.

[jquense]

Fix; inputs will fire for noop in edge-cases, inputs will fail to fire for actual changes in edge-cases, the fix involves a lot of code.

I can appreciate that, and I do want to make sure we are not missing any updates. However I want to be clear the above situation is already the case for input ranges (#554), which is where the PR originally came from. Input events just do not fire on IE and versions of chrome (and i'm sure others) when the value changes in various situations.

This PR doesn't just try to address noop onInput calls (though that is a potentially nice side effect) it allows for listening for more events, in order to not miss changes.

Of course we could just not dedupe the extra events and live with that, I find that more acceptable then missing changes, which is to say I think we are on the same page in terms of trade offs. I am just saying we are already in the worst case option for some inputs.

[syranide]

@jquense The other things you mention I have no issues with 👍. I defer to @spicyj or someone "official", but personally I'm just against the fragile deduping logic.

I am just saying we are already in the worst case option for some inputs.

True, but I feel like there is difference between such issues introduced by neglect and those introduced knowingly through added complexity.

[jimfb]

As per conversation with Sebastian, going to tentatively accept. We will sync with a few people internally to let them know that this change is coming down the pipeline, and push to www early in the week to maximize internal testing and make sure we're not breaking things.

@jquense can you fix the minor unit test failure?

@spicyj can you take a glance, and flag anything that you think would be a show stopper here?

Note to self: verify that we've cut v15 branch before merging.

[sophiebits]

It is hard for me to verify correctness here without a lot of thought, but I think this general strategy is okay. It would be better if we could refactor things to a point where ReactDOMInput could be responsible for tracking a lot of this data and logic instead of tacking on expandos in ChangeEventPlugin but that is probably significantly more difficult.

[jquense]

@spicyj I actual don't think i'd be too tough to put the value tracking stuff in ReactDOMInput, it'd certainly remove some awkward tracking in the Event Plugin, I wasn't sure if it would be weird for the event plugin to reach out for that though. Perhaps ReactDOMInput could just expose utils for interacting with the tracking info.

Does that make more sense, should I also update the PR to handle that?

Also I'd like to subsume the old-IE tracking logic so that tehre isn't too much confusing to follow duplication

[facebook-github-bot]

@jquense updated the pull request.

[jimfb]

@jquense We're getting "descriptor.get.call is not a thing" errors on Safari that looks like it blames to this diff. Can you take a look?

[jquense]

as the recent recipient of a new mac book pro I can, for the first time say: "yes" to that :P I'll take a look over the weekend

[jimfb]

Ping @jquense

[jquense]

sorry new job, not a lot of free time, hope to try and debug today. we'll see!

[jquense]

Quick test, seems to indicate that Safari returns { set: undefined, get: undefined, ...} for the value descriptor of the input...The issue is we need to wrap that to catch all value updates.

I am not very familiar with Safari, and can't find anything online about this, I don't know if its safe to assume that they are actually empty, or if Safari is hiding them. If they aren't there I think we can just use our local copy of the value?

nope misses value changes, the best I can think of here is to bail out of the optimization in this case and just over report changes for Safari...I have no idea if that is acceptable, it still seems better then a full revert to me.

I'd also love any thoughts from ppl more familiar with Safari, in case I'm missing any trick here.

[arendjr]

FYI: 15.0.x currently has a regression compared to 0.14.x with regards to auto-fill handling in IE11: #6614
This is a pretty big deal to us because our login form is currently pretty screwed up for people using auto-fill.

Now this PR should fix that, but I'm guessing you still won't merge this for 15.0.3... So does anyone know when 16.0.0 is scheduled?

[jquense]

FYI folks, this could go into 15. and fix a bunch of regressions in IE related to me naively trying to fix cross browser event bugs, (and perhaps cause a bunch more, who can say? browsers are the worst)

The only "breaking" bit of this is the use native.simulate which could be made non-breaking fairly straight forwardly by marshalling fake events via some flag. It's ugly but I think its the most ergonomic choice anyway, those bits could always be removed in 16 if its too ugly.

@jimfb @gaearon @zpao yall have master in production right? This does seem perhaps robust enough to handle the cases that've popped up.

[sophiebits]

@jquense Sorry if I'm missing something obvious, but can you elaborate on what the breaking bit is here?

[jquense]

@spicyj simulateNative no longer works for onChange in the way it's generally used.

Events only fire if the value in the event hasn't been seen before. so manually setting the value on an input and then calling simulateNative won't trigger a change.

The simple enough fix is to flag events created by simulateNative, so that they pass through the event plugin;

see: https://github.com/facebook/react/pull/5746/files#diff-2bead3a73555ca476b3dedf1f31fbb93R108

[ljharb]

Posted a comment here asking why this breaking change went into a minor version.

[mathiasbynens]

(For reference, this was backported to v15.6.0 here: #8575)

The Speedometer benchmark used to trigger React’s onChange programmatically as follows:

element.dispatchEvent(new Event('change', {
  bubbles: true,
  cancelable: true
}));

This patch intentionally broke such use cases. Is there a reasonable workaround?

Ref. https://bugs.webkit.org/show_bug.cgi?id=175715#c13

[sebmarkbage]

The core of that change is that the "internal" value must actually change for the event to trigger.

That's why these tests use internals to modify it: https://github.com/facebook/react/pull/5746/files#diff-2bead3a73555ca476b3dedf1f31fbb93

Before issuing the event.

You can also use a helper in ReactTestUtils to simulate an event happening at a higher level. ReactTestUtils.Simulate.change(node) but I'm not sure that works neither.

If you don't want any dependencies I think you can use the native setter to change the value before calling dispatchEvent.

Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value').set.call(input, 'new value')

To ensure that the value has changed in the internals without us observing it.