fix(inputs): improve "input" event emulation for legacy browsers

[cvn]

Hello,

In IE versions that use the keydown handler to check for changes to an input, there is a bug when pressing the tab key that will update the input after the change and blur events.

This PR fixes this behavior by cancelling the keydown handler, and updating immediately on change/blur.

See a demo of the bug here (only affects IE8/IE9): http://codepen.io/cvn/full/lvjJr/

Here is a fixed demo with the changes from this PR applied to angular.js: http://codepen.io/cvn/full/cgLns/

Currently, this bug only impacts IE8 & 9, but if PR #9265 lands, it will affect all versions of IE.

Issue #1206 may be related, but I'm not sure about this.

I'm submitting this PR to master, but I think it makes sense to include in both 1.2 and 1.3 branches.

Please let me know what you think.

[caitp]

This will affect IE10 and 11 after #9265 lands --- we'll need to look at this and see if it's a problem + the right approach.

@jbedard want to own this? (or maybe when you get back from traveling :p)

[mary-poppins]

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[cvn]

@mary-poppins I completed the CLA form today before I submitted this PR. As far as I can tell, there are two email addresses on the agreement. [redacted]. The latter address is the one I use for github. Please let me know if you need anything else.

[caitp]

@cvn mary-poppins is a robot that periodically updates these things. If you signed with the second email listed, you should be good here

[cvn]

Thanks, @caitp. I figured it was a bot, but wanted to be polite, just in case.

[mary-poppins]

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

[jbedard]

@cvn do you have an easier demo of the issue (without the placeholder stuff etc.), maybe on http://plnkr.co or http://jsfiddle.net/?

[cvn]

@jbedard Thanks for taking a look. This issue is directly related to #1206, so I'm going to post the fiddle from there instead of simplifying mine: http://jsfiddle.net/symblify/XL6RX/1/

In that fiddle, running in IE8/9, pressing tab while focused on a field will set it $dirty unnecessarily. This bug and my bug have the same cause: the tab key triggers a call to $setViewValue(element.val()). More generally stated, special keys that do not trigger the real "input" event are triggering angular's emulated input event. My bug—tab triggering a model update after the blur event—is another side effect.

The first commit I submitted only addressed the symptoms of my bug, but I've pushed changes that address the key handling instead. This solves it closer to the source. The code is based on work by @jbdutton, @symblify, and @montgomery1944 in the #1206 thread. Let me know what you think.

[jbedard]

Was there a case where this change was required?

[cvn]

@jbedard This allows Opera versions with the Presto engine to use the real "input" event instead of the emulation. Opera Desktop and Mobile up through versions 12 and all versions of Opera Mini only have the input event on form elements (inputs, textareas, and selects). I learned about this from a tip by @montgomery1944 in the #1206 thread.

Here's a test result from Opera 12.16 mac that confirms this: http://i.imgur.com/Carhv3n.png. Opera's support for the input event is pretty good, in my testing. The only flaws I found are that it will fire an input event when pressing the "enter" key in a single-line field.

EDIT: I didn't consider divs with the contenteditable attribute until now. These are supported in Presto, but without the input event. See table here. In other words, Opera's support for the input event is incomplete and we shouldn't rely on it. Forget about this change.

[jbedard]

I think old Presto versions of opera are too old and unsupported anyway...

[jbedard]

I think I'd like to do this once we can write proper integration tests in #9697.

But overall I'd prefer not adding more listeners and keycode combinations. I think I have a way of doing that but I'd really like a better way to test it...

[caitp]

Landed #9265, which is supposed to fix this problem too.

[caitp]

If it involves listening for a zillion different event types, it's a non-starter. We aren't merging that.

[jbedard]

#9265 fixed a few issues I found based on this discussion but I think there's at least one still broken.

If you tab out of an input without making a change, but some other code then modifies the value, then it will think the modification was caused by the tab key. Is that the issue still remaining @cvn? But that one requires more event listeners...

[cvn]

@jbedard That issue with the tab key you've described is still present after #9265.

That PR also adds the issue I discussed above where highlighting an m and typing m does not result in a listener() call. I consider this significant because the same action in a browser with the input event will cause a listener() call.

I've pushed a commit that I believe will improve the emulation and reduce edge cases. It fixes both of the problems I just mentioned. cvn@b43c14d

It does add a keypress event, but I think it's the best way, since it helps us filter down the keys that trigger listener() without having to add a ton of keys (tab, caps lock, etc) to the keydown check. #9265's technique is another way to solve this, but it introduced the "m" issue, which my new commit resolves.

@jbedard @caitp Let me know what you think. I've done a lot of testing, and can offer some reasons for my decisions if you're curious about anything.

And sorry about the deleted comment a few hours ago, I spoke too soon. :)

[jbedard]

@cvn so that's where your comment went :P (EDIT: and now it moved!)

For the highlighting m and typing m case... I thought that was ok since the ctrl.$viewValue !== value check will ignore that event anyway (https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L994)? It doesn't take quite the same code path (as the input event case), but is the result not the same?

[cvn]

@jbedard The listener() function currently uses value = trim(value) before the ctrl.$viewValue !== value check, so there's already one opportunity for different behavior in IE vs everything else.

A specific edge case I can think of is that if your input's model is set to "m ", and the user highlights m and types m, in modern browsers the input will update to "m" and in IE, with the current fix in 9265, it will stay at "m ". You can verify that modern browsers will trim the space here http://jsbin.com/pokod/2/edit.

And it's plausible that more logic could be added to the listener() in the future. I think the more accurate the emulation, the less edge cases are possible, and the cost of the additional listener here is worth it.

[jbedard]

The trim case is a good example.

I still think the situation is much better then before (and IE is more consistent :). If we can fix more issues without adding more listeners that would be great, but imo these remaining two issues might not be worth it...

[cvn]

If you don't think the additional listener is worthwhile, then I think at least we should add tab to the list of filtered keys in the keydown handler. This will resolve the tab key issue and leave the "m" issue as the only one that I'm aware of. I've added a commit with just this change: https://github.com/cvn/angular.js/commit/117c92e03c973ea090dddb2996464b6349015fbb

[jbedard]

I think https://github.com/cvn/angular.js/commit/117c92e03c973ea090dddb2996464b6349015fbb would be good actually. Unless I'm forgetting something. If the tab key ever causes a change it should also cause a blur + change event and the change event will capture it, right? Maybe just add a unit test that demonstrates that as well?

[jbedard]

Regarding the "m" vs "m " case, I think all browsers will do that actually? The model and input has "m " but the $viewValue === "m" because it was trimmed. Then highlighting and replacing one character with itself will still have $viewValue === "m" for non-IE and input.value === origValue for IE. It is different code paths though, which is lame and has the potential for bugs, but that specific case seems like it would have the same behaviour.

[cvn]

You are correct, that if tab key causes input, like with autofill, it will also cause change + blur events. I just tested this in IE11.

I wouldn't mind adding a unit test, but there's no way to trigger a specific key in unit tests as far as I'm aware. The closest approximation to IE's autofill behavior is something like browserTrigger(inputElm, 'keydown'); inputElem.val('new val'); browserTrigger(inputElm, 'blur'); Would that be sufficient? I'm not familiar with writing e2e tests and I feel like that's the only way to write a real test.

As for the "m" vs "m " case, $viewValue is only trimmed as part of parsing input from the browser, it does not trim the value when it comes from a model. You can see here: http://jsbin.com/pokod/4/edit?html,output This is evidence that the edge case I mentioned will behave differently in IE vs other browsers. I apologize for the janky setup, but I couldn't figure out a better way to get access to $viewValue for testing.

[jbedard]

I think you're right about the tests. I don't see any way to trigger a keydown event with the tabKey. Maybe you can just add support for a keyCode option to the browserTrigger eventData param? Might also need to support a KeyboardEvent event type. Maybe @caitp has an opinion on that though...

Regarding the "m " - I guess that only happens when going from model to the view and ngTrim doesn't execute. I was thinking of the $viewValue would always be trimmed. That case seems more weird in non-IE though - the model updates while the view looks the exact same and didn't change from the users perspective...