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

fix(ngModel): use $evalAsync for blur events to safely $apply #9808

Closed

Conversation

alexanderchan
Copy link
Contributor

Set the $touched asynchronously using $evalAsync if the element is blurred during an $apply, otherwise it keeps the old behaviour.

Fixes #8762

If the model is blurred during an apply it should trigger the $touched
asynchronously.

Fixes angular#8762
scope.$apply(function() {
modelCtrl.$setTouched();
});
if (scope.$$phase) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Narretz means $rootScope.$$phase (just clearing it up to avoid confusion 😈).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good I'll try to update that. I followed 719c747#diff-eefe7d65b6795cba34d2b08a27ebf2beR63 which only checked scope.$$phase.

Just to help my understanding is there a reason $rootScope.$$phase isn't used there? @tbosch thanks!

Ahh, sorry I see now, it came afterwards I should have checked the most recent version. Thanks for the tips guys.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification @gkalpak Why $rootScope is explained here: #8849 (comment)

@Narretz Narretz added this to the 1.3.x milestone Oct 29, 2014
@alexanderchan
Copy link
Contributor Author

@Narretz thanks for all your help with this one. Do you think it also makes sense to update the example in the docs at https://github.com/angular/angular.js/blob/master/src/ng/directive/input.js#L1645 with a check of the $rootScope.$$phase for the blur event? If so I can try to update this pr.

@alexanderchan
Copy link
Contributor Author

On second thought, if you think it is useful I might open a different PR as a doc fix as I might not have as much time in the upcoming evenings.

@Narretz
Copy link
Contributor

Narretz commented Oct 29, 2014

Please so! We can merge doc fixes faster anyway

----- Ursprüngliche Nachricht -----
Von: "Alex" notifications@github.com
Gesendet: ‎29.‎10.‎2014 22:37
An: "angular/angular.js" angular.js@noreply.github.com
Cc: "Narretz" mjstaffa@googlemail.com
Betreff: Re: [angular.js] fix(ngModel): use $evalAsync for blur events tosafely $apply (#9808)

On second thought, if you think it is useful I might open a different PR as a doc fix as I might not have as much time in the upcoming evenings.

Reply to this email directly or view it on GitHub.=

@gkalpak
Copy link
Member

gkalpak commented Oct 30, 2014

I don't think we should promote the use of private, undocumented APIs (such as $$phase) in the examples.
@Narretz, wdyt ?

@alexanderchan
Copy link
Contributor Author

That's a good point, I wonder instead if the solution would be to have a public safe apply function that internally checks the phase and does the right thing accordingly.

On Oct 30, 2014, at 07:15, Georgios Kalpakas notifications@github.com wrote:

I don't think we should promote using private, undocumented APIs (such as $$phase) in the examples.
@Narretz, wdyt ?


Reply to this email directly or view it on GitHub.

@Narretz
Copy link
Contributor

Narretz commented Oct 30, 2014

That's true, I thought the example already used $$phase, but on the scope. Let's not introduce private apis in examples then. safeApply is a different topic. You can open an issue about this, but it has been shot down a few times.

@gkalpak
Copy link
Member

gkalpak commented Oct 30, 2014

Why not just use $evalAsync() ? It's a form of safe $apply anyway (the only difference being that it executes the code "asyncronously", i.e. after the calling function has executed).

@Narretz
Copy link
Contributor

Narretz commented Oct 31, 2014

@gkalpak Good question. I actually don't know. In #6910 this was discussed, and it links to more discussion, but I can't tell with 100% certainty.

@Narretz
Copy link
Contributor

Narretz commented Oct 31, 2014

Anyway, this looks good to me now

@gkalpak
Copy link
Member

gkalpak commented Nov 1, 2014

@Narretz: Not sure if it was clear, but I meant why not use $evalAsync in the docs example.

@Narretz
Copy link
Contributor

Narretz commented Nov 2, 2014

Oh, okay. Yeah, I wanted to ask if you meant that, but then I didn't somehow :)

alexanderchan added a commit to alexanderchan/ui-date that referenced this pull request Nov 8, 2014
This refactors the date parsing to allow it to be used in the ui-date
directive.

Breaking Change the blur event isn't fired.  This can be added back when angular/angular.js#9808 is merged
@petebacondarwin petebacondarwin modified the milestones: 1.3.4, 1.3.x Nov 12, 2014
robmunro pushed a commit to robmunro/ui-date that referenced this pull request Nov 19, 2014
This refactors the date parsing to allow it to be used in the ui-date
directive.

Breaking Change the blur event isn't fired.  This can be added back when angular/angular.js#9808 is merged
var control = $rootScope.myForm.myControl;

$rootScope.$apply(function() {
expect(control.$touched).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specifc reason why the first two expects are inside the $apply block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Narretz there is no specific reason for the two expects to be in there. Do you think it would be better to move them just before the apply block? I was just trying to ensure that the touched and untouched state remained exactly the same before and after the blur event.

@Narretz Narretz closed this in eab2718 Nov 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion to use safe $apply in ngModelDirective
5 participants