fix(input): don't dirty model when input event triggered due to placeholder change #5960

caitp · 2014-01-23T19:35:32Z

Certain versions of IE inexplicably trigger an input event in response to a placeholder being set.

It is not possible to sniff for this behaviour nicely as the event is not triggered if the element is not attached to the document, and the event triggers asynchronously so it is not possible to accomplish this without deferring DOM compilation and slowing down load times.

Closes #2614
Closes #5960

This that could theoretically go wrong:

If a value change and a placeholder change both occur and emit a single coalesced event (rather than two independent events), this may lead to a model occasionally not being updated. I imagine this would be pretty rare in practice.
If a test for lastValue === value is added to the exit condition, then the same issue above still potentially happens when an empty string becomes an invalid string and constraint validation gives us an empty string.

There are probably a few more holes in this, unfortunately, which is why it would be better if MS would hotfix the affected browsers. But it's not breaking any tests and if it doesn't break any apps which dogfood it then that's probably fine.

…holder change Certain versions of IE inexplicably trigger an input event in response to a placeholder being set. It is not possible to sniff for this behaviour nicely as the event is not triggered if the element is not attached to the document, and the event triggers asynchronously so it is not possible to accomplish this without deferring DOM compilation and slowing down load times. Closes angular#2614 Closes angular#5960

caitp · 2014-01-23T20:30:54Z

test/ng/directive/inputSpec.js

@@ -509,6 +509,43 @@ describe('input', function() {
    });
  }

+
+  it('should not dirty the model on an input event in response to a placeholder change', inject(function($document) {


This test doesn't really work without the async junk, sorry about that :(

Hmm. I wonder whether the benefit of actually testing against the real behaviour in IE10 is worth the async stuff. Perhaps we should just mock out the IE bits and isolate the code you wrote?

I'll see if it's mock-able

petebacondarwin · 2014-01-23T20:36:26Z

Do we know exactly what versions of IE exhibit this behaviour?

caitp · 2014-01-23T20:37:32Z

the original issue and some stackoverflow posts report IE10 and a few cases IE11, with unknown emulation modes (but it definitely affects IE10 standards mode)

https://travis-ci.org/angular/angular.js/jobs/17502175 this build doesn't have the fix, just the test, so you can see it fail a few times

petebacondarwin · 2014-01-23T20:47:48Z

This is quite nasty. I wonder if it is fixable with a polyfill for placeholder instead. Then we can offer that as a work around rather than hacking AngularJS?
Here is a list of such polyfills. https://github.com/Modernizr/Modernizr/wiki/HTML5-Cross-browser-Polyfills#web-forms--input-placeholder

What do you think?

caitp · 2014-01-23T20:51:29Z

I think it's worth a shot, but I don't think there's any guarantee it would work. I'll try it out on saucelabs real quick

Actually most of these shims look like they're just adding placeholder rendering to browsers which don't support it natively, so I'm not super convinced these would fix the issue

petebacondarwin · 2014-01-23T20:54:08Z

So these polyfills tend to remove the placeholder attribute altogether and then simulate it instead.

petebacondarwin · 2014-01-23T21:11:35Z

OK, so I guess that apart from fine tuning the conditions of the if I think that this approach looks fine.

caitp · 2014-01-23T21:15:54Z

Just tested all of those polyfills with stock angular 1.2.9 on saucelabs IE10 http://plnkr.co/edit/PWwP3OZTtcp7VUEDHLcq?p=preview, none of them solve the issue. So maybe the if conditions can be tuned up a bit.

On the other hand maybe it's not totally worth fixing it, I dunno. I'll see if I can clean it up a bit, though

…holder change Certain versions of IE inexplicably trigger an input event in response to a placeholder being set. It is not possible to sniff for this behaviour nicely as the event is not triggered if the element is not attached to the document, and the event triggers asynchronously so it is not possible to accomplish this without deferring DOM compilation and slowing down load times. Closes angular#2614 Closes angular#5960

caitp · 2014-01-23T22:23:35Z

test/ng/directive/inputSpec.js

@@ -509,6 +509,28 @@ describe('input', function() {
    });
  }

+
+  it('should not dirty the model on an input event in response to a placeholder change', inject(function($sniffer) {
+    if (msie && $sniffer.hasEvent('input')) {


lastly, could remove the msie check from both the test and the code, which is probably okay but I'm not positive

…holder change Certain versions of IE inexplicably trigger an input event in response to a placeholder being set. It is not possible to sniff for this behaviour nicely as the event is not triggered if the element is not attached to the document, and the event triggers asynchronously so it is not possible to accomplish this without deferring DOM compilation and slowing down load times. Closes angular#2614 Closes angular#5960

caitp · 2014-04-17T21:19:46Z

src/ng/directive/input.js

+    // Some versions of MSIE emit an 'input' event when the placeholder attribute/property
+    // change. This hack prevents an otherwise pristine field from being dirtied on IE
+    // browsers.
+    if (msie && (event || noevent).type === 'input' && element[0].placeholder !== placeholder) {


Make note of affected IE versions here

can you change this to msie < 11 and test it on IE11?

We're running Angular 1.0.8, and only had the placeholder problem in IE10 (IE v10.0.9200.16899 on Windows 8). We added the changes to the textInputType function, and it solved the problem.

IgorMinar · 2014-04-17T21:20:01Z

otherwise this looks good

…holder change Certain versions of IE inexplicably trigger an input event in response to a placeholder being set. It is not possible to sniff for this behaviour nicely as the event is not triggered if the element is not attached to the document, and the event triggers asynchronously so it is not possible to accomplish this without deferring DOM compilation and slowing down load times. Closes #2614 Closes #5960

caitp reviewed Jan 23, 2014
View reviewed changes

caitp mentioned this pull request Feb 18, 2014

IE10 fires input event when a placeholder is defined so that form element is in dirty instead of pristine state #2615

Closed

caitp reviewed Apr 17, 2014
View reviewed changes

caitp closed this in ff428e7 Apr 18, 2014

jamie-pate mentioned this pull request Jun 11, 2014

IE10 Expression in placeholder attr causes input to be dirty #4781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): don't dirty model when input event triggered due to placeholder change #5960

fix(input): don't dirty model when input event triggered due to placeholder change #5960

caitp commented Jan 23, 2014

caitp Jan 23, 2014

petebacondarwin Jan 23, 2014

caitp Jan 23, 2014

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

caitp Jan 23, 2014

caitp Apr 17, 2014

IgorMinar Apr 17, 2014

Benxamin May 21, 2014

IgorMinar commented Apr 17, 2014

fix(input): don't dirty model when input event triggered due to placeholder change #5960

fix(input): don't dirty model when input event triggered due to placeholder change #5960

Conversation

caitp commented Jan 23, 2014

caitp Jan 23, 2014

Choose a reason for hiding this comment

petebacondarwin Jan 23, 2014

Choose a reason for hiding this comment

caitp Jan 23, 2014

Choose a reason for hiding this comment

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

petebacondarwin commented Jan 23, 2014

caitp commented Jan 23, 2014

caitp Jan 23, 2014

Choose a reason for hiding this comment

caitp Apr 17, 2014

Choose a reason for hiding this comment

IgorMinar Apr 17, 2014

Choose a reason for hiding this comment

Benxamin May 21, 2014

Choose a reason for hiding this comment

IgorMinar commented Apr 17, 2014