fix(inputs): ignoring input events in IE caused by placeholder changes o...

[jbedard]

...r focus/blur on inputs with placeholders

This is my attempt at fixing #8242 as well as the extra placeholder issues with focus/blur. I'm assuming I'm missing a weird test case that still breaks somewhere, but I still think this is an improvement (it definitely is for my use cases).

[caitp]

I don't think this is quite right.

If we're going to get an input event regardless, we probably want to skip the input event no matter what the value is, to avoid doing extra validating

[jbedard]

But there is no input event if there is a value, that is the issue this is trying to fix.

EDIT: i originally said it backwards

[caitp]

I wrote a plunker on the MS feedback site which illustrates a variety of cases where we do get an input event when it's not expected (click, which is basically focus --- I don't think we really need to care about blur but why not), and when the attribute changes (regardless of whether there is a value or not).

So we're going to get the input event anyways here, whether there is a value or not

[jbedard]

The plunker in #8242 shows otherwise...?

As I understand it the extra input event occurs when the placeholder visually changes. If it changes while not visible (because the input has focus or a value) it doesn't seem to occur. All my tests have been in IE11, it is possible IE10 is different...

[caitp]

http://plnkr.co/edit/HEjnp1ElIgBTwTPSGuok?p=preview --- okay I've edited the plunker which shows this issue, and you're right, it doesn't seem to happen with an empty value.

It's all good then

[caitp]

(Haven't tested this with validity.badInput, not sure if IE11 ships with ValidityState)

[jbedard]

I tested this with type=number and entering different values (f3 vs f vs 3f all seem different). It seems like validity.badInput doesn't exist in IE11 though and I couldn't find any issues.

[caitp]

so I think we can unit test this if we detect IE11 just by sending fake events in the prescribed order

[jbedard]

I thought of that, it's just hard to figure out all the different scenarios, but I guess once we do it would be great to unit test them all. If you think this solution might fly then I'll try adding tests. It seems like this is currently breaking a lot of unit tests in IE though :(

[caitp]

the test failures seem to indicate that in IE, we are skipping input events we shouldnt be skipping

[caitp]

I'll take another look tomorrow

[jbedard]

Looks like it's the initial call to $observe when there is no placeholder, so it's basically breaking inputs without placeholders, which is pretty bad :P

Adding (newPH || oldPH) && seems to fix it...

[jbedard]

Updated, slightly different then my previous comment: (newPH !== oldPH) && ignores the initialization call if there is no placeholder, but if there is a placeholder, even if empty, it will set the flag to ignore the next input.

[caitp]

So the change mostly works, except it fails a test due to the placeholder being set differently (not via interpolation).

We could call that test invalid and just go with this, but it kinda sucks to do that

[jbedard]

I think that test is incorrect, it assumes an input event will fire even though the input has a value. I have a bunch of tests just about ready to commit which try to simulate all the different scenarios. I'll try to add those tonight.

However, would it be better to just disable use of the input event in IE? The same thing is already done in IE9 due to how broken the input event is in IE9. I'd say the input event (+ placeholder) is pretty broken in IE10+ as well...

[caitp]

If the test is wrong, I would say fix the test =) Disabling the input event may be the simplest solution with the fewest lines of code --- however this may interact poorly with some input types

[caitp]

I would like to make this fix better in master pretty much ASAP, this would be good for RC4

[caitp]

(so: please fix le test or provide a different alternative if you prefer that)

[jbedard]

I'll add all the new tests tonight (they are stashed at home...). Did you have any other ideas or feedback for the fix itself?

One issue is that the fix now depends on $attr.$set. If someone changes a placeholder without $attr then we won't catch it and the input will be marked as dirty.

One bug I noticed, which may or may not be worth fixing: if the placeholder changes while the input has focus it does NOT emit the extra input event, but the current fix will ignore the next input event. Fixing this would require keeping a hasFocus state (or using document.activeElement) and just adding a hasFocus && to the $observe condition...

[jbedard]

I'd also like to fire up a VM to confirm that IE10 is the same as IE11.

[caitp]

One issue is that the fix now depends on $attr.$set. If someone changes a placeholder without $attr then we won't catch it and the input will be marked as dirty.

This is what I was talking about above, actually. I'm not sure if we actually care though

[caitp]

One bug I noticed, which may or may not be worth fixing: if the placeholder changes while the input has focus it does NOT emit the extra input event, but the current fix will ignore the next input event. Fixing this would require keeping a hasFocus state (or using document.activeElement) and just adding a hasFocus && to the $observe condition...

Dealing with this is getting a bit crazy, I prefer the alternative solution of just not using input events for IE at all.

[jbedard]

I think that would be better as well, and then we don't even have to think about all these edge cases. Would that be acceptable?

We'd just delete all the IE/placeholder stuff and change the $sniffer.hasEvent to detect all IE versions? Or are there some input types (file? radio/checkbox? etc.) where the input event should still be used?

[caitp]

I don't feel good about adding extra event listeners like #9623, but I guess if it's only done for legacy browsers it might help discourage people from using em? I dunno

[jbedard]

After this change all IE versions will be listening to change+keydown+cut+paste instead of just change+input, not only legacy browsers. But the second commit in this PR is quite different then the solution in #9623 and doesn't add another listener (#9623 adds keypress) but instead tries to ignore keydown events that did not actually cause a change to the input.value.

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[googlebot]

CLAs look good, thanks!

[googlebot]

CLAs look good, thanks!

[jbedard]

Changed the tests in the first commit to do msie && browserTrigger(inputElm, 'input') instead of putting the full test in if (msie).

Fixed the failing test in the second commit, it just required an extra $apply() due to one of the $animate changes a while back. I also moved these tests along with the other copy/paste tests.

And fixed some new jscs issues...

[googlebot]

CLAs look good, thanks!

[caitp]

Thanks for updating --- @petebacondarwin does this look good to you? I'm a bit confused about the timeout behaviour, but I guess it was there before this. (I really want to get this in --- we can add E2E tests later, so if the timeout behaviour doesn't break anyone then lets merge it imho)

[jbedard]

The timeouts delay the deferListener because the keydown event happens before the input.value actually changes. This also allows recording input.value on keydown and then checking if it actually changed after the delay (the second commit in this PR). I guess it might also avoid extra calls to listener if multiple cut/paste/keydown/change events occur before deferListener gets invoked...

[googlebot]

CLAs look good, thanks!

[caitp]

alright, well I'm just going to land it because this has been waiting for long enough

[caitp]

If we break things we can always revert it, and probably won't have to

[petebacondarwin]

+1 @caitp

[paquettea]

Not listening on "oninput" creates an issue on IE10 and IE11. The "clear field" button that is present in input type="text" and input type="search" is only firing oninput event (no onchange), so as it is right now, clearing the field with the button is not updating the ng-model value. I did a module to fix this because it seems there is quite a debate on what to do right now. I think that this tickets needs to be re-opened

https://github.com/paquettea/angular-ie-clearfield-databinding

[petebacondarwin]

OK let's take another look

[jbedard]

I think this is a method of modifying the input value that we missed. This has two issues (only in IE) that I've found so far:

• If you clear a search box angular doesn't see the change until blur (when a change event fires). This delays the ng-model update.
• The change event will not fire if the value is the same as when the input gained focus. This never updates the ng-model so the model value is wrong.

Should we open a new bug for this though?

[petebacondarwin]

@jbedard and @paquettea - if this is causing problems then let's open a new bug for it.