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

Commit

Permalink
fix(input): call $setTouched in blur asynchronously if necessary
Browse files Browse the repository at this point in the history 
If the model is blurred during an apply it should trigger
$setTouched asynchronously.

Fixes #8762
Fixes #9808
Closes #10014
  • Loading branch information
@alexanderchan @Narretz
alexanderchan authored and Narretz committed Nov 20, 2014
1 parent 637d3b4 commit eab2718
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 5 deletions.
12 changes: 7 additions & 5 deletions src/ng/directive/input.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2497,7 +2497,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
</file>
* </example>
*/
var ngModelDirective = function() {
var ngModelDirective = ['$rootScope', function($rootScope) {
return {
restrict: 'A',
require: ['ngModel', '^?form', '^?ngModelOptions'],
Expand Down Expand Up @@ -2541,15 +2541,17 @@ var ngModelDirective = function() {
element.on('blur', function(ev) {
if (modelCtrl.$touched) return;

scope.$apply(function() {
modelCtrl.$setTouched();
});
if ($rootScope.$$phase) {
scope.$evalAsync(modelCtrl.$setTouched);

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Nov 20, 2014

Member

@Narretz, @alexanderchan: I don't know why I didn't notice this earlier, but is there any particular reason for not calling $setTouched() rightaway ?

This comment has been minimized.

Sign in to view

Copy link
@alexanderchan

alexanderchan Nov 21, 2014

Author Contributor

@gkalpak: Good question, it looks like it's needed for custom animations triggered on the blur event.

I believe that this is used for the$animate.setClass which in turn calls runAnimationPostDigest which is queued up and only called at the end of $rootScope.$digest. I'm just guessing here, but If we called it directly I think that we would miss the ng-touched class on the element and also triggering any custom ng-touched animations (until the next digest cycle).

(edited some of the wording)

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Nov 21, 2014

Member

@alexanderchan: So, you are saying that in order for $animate to work, $setTouched() has to be called on another $digest ? Seems weird.
Could you elaborate on why you believe calling it right-away would break ng-touched related animations ?

This comment has been minimized.

Sign in to view

Copy link
@alexanderchan

alexanderchan Nov 21, 2014

Author Contributor

@gkalpak: Ahh, I understand now, I believe you are correct that we can call $setTouched() directly because we are already first checking that we are in a digest with $$phase.

I thought we needed to call the $apply for some other purpose so I wrapped it in the $evalAsync to ensure the same behaviour. I hope that I am reading the calls correctly and there is not some other reason for the original $apply. Even if there is, performing the same action within the digest should be correct, right?

It will definitely improve the performance. Thanks for your suggestion to look into why the apply was called in the first place and re-explaining the weirdness 😃 I'll try to generate a new PR with the updated test.

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Nov 21, 2014

Member

The reason for the original $apply is that the blur event (like any other event) is usually triggered outside of the Angular context (so, Angular has to be explicitly notified).

But, it can happen that when the blur event happens (i.e. when the user leaves the field) a $digest is already in progress (triggered by something totally unrelated). In that (rare but possible) case, calling $apply again leads to errors and this is why introducing the $$phase check makes sense.

So, I believe that replacing $evalAsync() with a direct call to $setTouched() is the right thing to do.

This comment has been minimized.

Sign in to view

Copy link
@alexanderchan

alexanderchan Nov 21, 2014

Author Contributor

Awesome, sounds good I'll try to get the update soon and I'll ping you when it's all set. Thanks again.

This comment has been minimized.

Sign in to view

Copy link
@petebacondarwin

petebacondarwin Nov 21, 2014

Contributor

I think the use of $evalAsync is more subtle here. Yes, we can ensure that we are in exactly one $apply block by checking $$phase and so the $evalAsync is not needed for that.
What it is trying to do here, is force blur to always be async. I.E. If we are already in a $apply then it must have been called programmatically so make it async; if it is not already in a $apply then it must have been triggered by a user event (such as a click or key press), so it is already async.

It is the same as in this commit 719c747

Does that make sense?

This comment has been minimized.

Sign in to view

Copy link
@alexanderchan

alexanderchan Nov 21, 2014

Author Contributor

I think it's starting to make sense. I modelled #9808 originally after 719c747. It seems like there's a lot of good discussion in the related fixed and closed on that commit, in particular #6910. I'll read through that to try to see more of the history behind the $evalAsync

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Nov 21, 2014

Member

I am not so sure :)

I mean all it does here is call $setTouched(), so I don't know if we really care that this is executed asynchronously.
It is not executing a user-defined callback or something (as is the case with ngBlur) and thus the state of the model shouldn't make any difference.
(But I could be wrong of course :))

This comment has been minimized.

Sign in to view

Copy link
@gkalpak

gkalpak Nov 21, 2014

Member

@alexanderchan: Little heads-up:

In #9808 #9891* we didn't need "asynchronicity", but a "safe $apply()". We used $evalAsync() as a form of "safe $apply()" (without having to resort to private $$phase property).

In 719c747, $evalAsync() is not used a "safe $apply()", but because of the need of asynchronicity.

So, those are two fundamentally different uses of $evalAsync().

*: Sorry, I thought you were talking about #9891 (but you were talking about #9808). What I said applies to #9891.
} else {
scope.$apply(modelCtrl.$setTouched);
}
});
}
};
}
};
};
}];


/**
Expand Down
26 changes: 26 additions & 0 deletions test/ng/directive/inputSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,32 @@ describe('ngModel', function() {
dealoc(element);
}));

it('should digest asynchronously on "blur" event if a apply is already in progress',
inject(function($compile, $rootScope) {

var element = $compile('<form name="myForm">' +
'<input name="myControl" ng-model="value" >' +
'</form>')($rootScope);
var inputElm = element.find('input');
var control = $rootScope.myForm.myControl;

$rootScope.$apply(function() {
expect(control.$touched).toBe(false);
expect(control.$untouched).toBe(true);

browserTrigger(inputElm, 'blur');

expect(control.$touched).toBe(false);
expect(control.$untouched).toBe(true);
});

expect(control.$touched).toBe(true);
expect(control.$untouched).toBe(false);

dealoc(element);
}));


it('should register/deregister a nested ngModel with parent form when entering or leaving DOM',
inject(function($compile, $rootScope) {

Expand Down

0 comments on commit eab2718

Please sign in to comment.