Normalize onChange behavior in IE for inputs with placeholders

[jquense]

Checks for actual changes to the input/textarea value, eliminating the case where an empty
value moves to empty value triggers a input event, which happens when IE 10+ sets/unsets a placeholder. I made the change in the actual components vs the ChangeEventPlugin, because comparing last/next values is struck me as the easiest fix and that's much harder from the event handler.

fixes #3377 and fixes #3484

[jquense]

aaaaand 3 tries later I figure out how to get eslint to run locally so I don't need to keep using travis as a check

[zpao]

2 things here.

1. isIEInputEvent
2. Put the && at the end of the previous lines and then let's wrap the whole return in parens

return (
  'documentMode' in document &&
  document.documentMode > 9 &&
  etc
)

And now actually a 3rd since I'm looking. We can cache the value of 'documentMode' in document && document.documentMode > 9 so that we don't need to look it up each time (any access of DOM properties has a cost).

[zpao]

cc @syranide & @salier

[syranide]

This seems fragile considering there's lots of code beneath this that may or may not need to be executed, when all this really wants to do is discard the call to onChange right?

[jquense]

yeah, honestly i returned returnValue just to appease eslint's consistent-return rule, originally it was just a lone return, which would be less prone to accidental refactoring breakage. Alternatively I could just wrap the whole thing in a big if/else or return undefined perhaps? Not sure how to best handle the lint rule :P

[syranide]

On a general note, I don't think we need to special-case IE here, any browser that fires an event without the value having changed should receive the same treatment I would think?

[syranide]

(@zpao) It's also worth noting that the reason why this really should be fixed in the event system is because change bubbles, you can put onChange anywhere among the ancestors (and some do). I personally wouldn't mind deprecating that behavior entirely, but that's not how it is right now.

[jquense]

On a general note, I don't think we need to special-case IE here, any browser that fires an event without the value having changed should receive the same treatment I would think?

That is true, the attempt was to be as narrow as needed to fix the bugs, but yeah I don't check for browsers in my workarounds in userland and it has not caused any issues. though I'm sure mobile safari has some obscure reason why that might not work :P

It's also worth noting that the reason why this really should be fixed in the event system is because change bubbles, you can put onChange anywhere among the ancestors (and some do). I personally wouldn't mind deprecating that behavior entirely, but that's now how it is right now.

I totally agree that event system is the more correct place to handle this, but as I was thinking about how to do it from ChangeEventPlugin, it involved a lot more complexity without a clever way to grab the input's last value, to say nothing about the controlled/uncontrolled value disagreement issue. It struck me that the reduced complexity was probably more helpful in this case. That being said the bubbling is something I didn't think about. Would it be enough to just stopPropagation and preventDefault along with returning early in the component?

[danibrear]

nitpicking: couldn't you just use: hasNoisyInputEvent = document.documentMode > 9; since undefined > 9 will be false?

[jquense]

true, though the current way is consistent with similar checks elsewhere in the code base

[jquense]

I've simplified the check to just use "last" value regardless of whether its from props or the native input. works well enough. I haven't found any cases that break so my initial concern about the controlled values seem unwarranted.

Also added a propagation stop to handle the bubbling issue.

[syranide]

I would change event.target.value === '' && lastValue === '' to event.target.value === lastValue because we really don't want any of those to emit a change event.

I'm not a fan of this as it does not take care of foreign nodes (but it is beneficial...), it being a fix in the input wrapper rather than the event system. It's treating the symptom, not the cause, so it does not apply equally. I still think it's in the best interest of React to not use bubbled events, but it's an entirely separate discussion and a huge breaking change which is unlikely to happen I would think.

@zpao How do you feel about this?

[jquense]

So alternatively, to solve this in the event system we could just opt IE out of any input events, pushing any IE into the ie8/9 input event polyfill. I have another branch testing that approach. It complicates it a bit since attachEvent doesn't exist in ie11, but propertychange still does.

Maybe that whole polyfill is overkill for younger IE and I am not sure what the performance concerns are (if any)...@spicyj can you speak to that at all?

[jquense]

resolved by #4051!

[arendjr]

Given #6614, I suspect it might be worth it to have another look at this PR, since #4051 seems to have unintended side-effects...