Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

IE placeholder input event fix suppresses an extra event with interpolation #8242

Closed
jbedard opened this issue Jul 17, 2014 · 20 comments
Closed

Comments

@jbedard
Copy link
Contributor

jbedard commented Jul 17, 2014

109e5d1 introduced an issue which suppresses an extra input event when a placeholder changes due to interpolation and the input has a value. I believe the extra input event does not fire if the input has a value which causes the next real input event to be ignored. I haven't tested <IE11

See http://plnkr.co/edit/qZSjmzuQfzTbkI9eprcR?p=preview

The 2 inputs using interpolation in the placeholder suppress an extra input event on initialization and again each time the interpolation value changes.

Another issue not solved by 109e5d1 is shown in the last 2 inputs of the plunker. I'm not sure if that should be fixed within angular but it would be nice now that some of the placeholder issues are. Here is the fix I've been using, with 109e5d1 I just had to switch ignoreNextInput to false since angular handles that initial input event now. This doesn't handle the extra input event when the placeholder changes but that would be easy to add.

//input events order for inputs with placeholders in IE - the focus/blur input event only occurs if the input has no value
//  creation: input
//  focus:    focusin focus input
//  blur:     focusout input blur
var ignoreNextInput = !!$attrs.placeholder;

$element.on("focusin focusout", function(e) {
    ignoreNextInput = !!(!this.value && this.placeholder);
});

$element.on("input", function(e) {
    if (ignoreNextInput) {
        e.stopImmediatePropagation(); //(stopImmediate is used so the ng-model listener never receives it)
        ignoreNextInput = false;
    }
});
@jeffbcross jeffbcross self-assigned this Jul 23, 2014
@jeffbcross jeffbcross added this to the 1.3.0 milestone Jul 23, 2014
@jeffbcross jeffbcross removed their assignment Jul 23, 2014
@btford btford removed the gh: issue label Aug 20, 2014
@tbosch
Copy link
Contributor

tbosch commented Sep 11, 2014

@caitp Could you take on this one, as it is a regression?

@caitp
Copy link
Contributor

caitp commented Sep 11, 2014

There isn't much to do about it, you have to make a choice about whether you want IE to dirty models unnecessarily, or whether you want interpolation to work correctly --- there isn't really anything in between. I spent too much time on that problem, there is no reliable way to work around the browser bug other than just ignoring the input event in the case of the empty string.

So if this is causing a problem for people then just revert it (at which point the IE people will complain again) -- given that nobody has ever complained about this until now, for months and months, I think it's better to keep it as-is

@tbosch tbosch assigned caitp and btford and unassigned caitp Sep 15, 2014
@tbosch tbosch modified the milestones: 1.3.0-rc.3, 1.3.0 Sep 15, 2014
@btford
Copy link
Contributor

btford commented Sep 17, 2014

@caitp +1.

Closing, since there unfortunately isn't much we can do about this.

@jbedard, it might be worth filing a bug report against IE. I suspect other libs/frameworks might run into this issue.

@btford btford closed this as completed Sep 17, 2014
@caitp
Copy link
Contributor

caitp commented Sep 17, 2014

I have actually filed a bug against IE for this ages ago, and I think a few duplicates have popped up since: https://connect.microsoft.com/IE/feedbackdetail/view/856700

IIRC a microsoft rep actually commented on one of the angular bugs relating to it, but I think they did say it was pretty low-priority for IE11

@jbedard
Copy link
Contributor Author

jbedard commented Sep 17, 2014

Isn't the fix for this just adding && element[0].value? From my testing the original issue (an input event when the placeholder changes) only occurs if the input has a value, but the original fix doesn't take that into account.

@caitp
Copy link
Contributor

caitp commented Sep 17, 2014

@jbedard it's not, because we also can end up with input events when the value is the empty string. This IE bug doesn't play well with HTML5 validation.

Really it's more like if (value || (hasValidityState && validityState.badInput)) or something, but IIRC that's essentially what we were doing. Maybe we shrunk it down for simplicity

@jbedard
Copy link
Contributor Author

jbedard commented Sep 17, 2014

HTML5 validation didn't even cross my mind, I'll have to look into it more I guess.

@caitp
Copy link
Contributor

caitp commented Sep 17, 2014

Oh no, there were two cases. Changing to the empty string form a non-empty string, and changing to the empty string from the empty string (native validation case)

@jbedard
Copy link
Contributor Author

jbedard commented Sep 18, 2014

Is there an example somewhere to reproduce the issue caused by the validity state?

@caitp
Copy link
Contributor

caitp commented Sep 18, 2014

type "c" into a number input in a browser which supports ValidityState.badInput

@jbedard
Copy link
Contributor Author

jbedard commented Sep 18, 2014

But how can I get an input event when the value is empty due to badInput?

@caitp
Copy link
Contributor

caitp commented Sep 18, 2014

You get an input event when you type into the control. The value is reported as the empty string, but the input event still fires

@jbedard
Copy link
Contributor Author

jbedard commented Sep 24, 2014

I think that case can be covered quite easily, or at least I can't reproduce any issues like that. When a placeholder changes and is there no value then the next input event must be ignored, this is true if there is bad input or not. The same thing can be done on focusin/out to prevent the other issues I described (which makes testing this a million (or 2) times easier as well).

Here is my fix: jbedard@c85137d (this is for 1.2 since it was a regression).

If you don't see any issues I'll create a PR...

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

there are issues with it, we tried all this lol

@jbedard
Copy link
Contributor Author

jbedard commented Sep 24, 2014

Where can I see such attempts at this? And what issues exist? (sorry to keep nagging you about this, and thanks for all the info!)

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

In the PR where this original implementation landed, there was a lot of code review.

So the best solution that I had come up with, was to say "if we are IE, element.value looks the same as it did last time, and we aren't suffering from bad input, abort".

The problem is, element.value isn't guaranteed to be exactly the same as what it was in the last input event whether the bug happens or not, so the value check is kind of broken. Checking that the placeholder is identical is meaningless because placeholder changes aren't the only thing that causes this issue in IE (For example, if you so much as click on an element with a placeholder in IE, you get an input event --- the placeholder does not change).

We didn't end up going with the value check, which would have been broken in a number of cases anyways. So we've got the placeholder check instead, which doesn't prevent all of the IE bugs (only one of them), which isn't super useful. But even if we did go with a proper value check, it was still going to be broken, heh :(

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

Anyways, you are welcome to submit a pull request which does this better, but unfortunately I think this is probably never going to totally work right until IE stops emitting input events at nonsensical times

@jbedard
Copy link
Contributor Author

jbedard commented Sep 24, 2014

I still can't find any test case that breaks after my change, but I'm still assuming I'm missing something... I'll submit a PR and maybe someone can point out a use case where it breaks, and maybe that will be better or worse then the current issues.

@caitp
Copy link
Contributor

caitp commented Sep 24, 2014

I wouldn't trust the unit tests for this stuff, you really need to do a lot of manual testing.

Things like:

  1. don't do anything when interpolated placeholder changes (test IE)
  2. do things (validate) when control goes from empty string -> bad input (test Chrome)
  3. don't do anything when element with placeholder is clicked (test IE)

come up with new tests for this as needed, there are a lot more failure cases, these are just the most obvious ones

@jbedard
Copy link
Contributor Author

jbedard commented Sep 24, 2014

Ya I don't think you can unit test this very easily :(

Currently I have a test app with different combinations of:

  • input types (numeric, string)
  • placeholders (none, constant, interpolated which is never empty, interpolated which can be empty)
  • model values (none, empty, valid, invalid)
  • validity (as a result of type=numeric and value=a string)

Then in each case played with focus, interpolated placeholders changing (possibly changing to/from empty), modifying values with user input or model changes.

One I just thought of which I haven't tried is placeholders changing while the input is in focus. There would also be issues if someone programmatically changes the placeholder without using $attr since I'm using $observe.

I haven't tested chrome since this is all within an if (msie)...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants