fix(ngEventDirs): blur and focus use $evalAsync to prevent inprog errors

[Narretz]

Request Type: bug

How to reproduce:

• ng-click triggers focus() on an element that has ng-focus
• ng-key* triggers blur() on an element that has ng-blur
• moving an element in an ng-repeat that has ng-blur before the preceding item

Component(s):

Impact: medium

Complexity: medium

This issue is related to:

Detailed Description:

Other Comments:

This fixes cases where ngFocus and ngBlur will throw $rootScope.inprog errors, because they have been called during an active digest. There are (at least) three scenarios where this can happen, see above

ng-click triggers focus() on an element that has ng-focus
ng-key* triggers blur() on an element that has ng-blur
moving an element with ng-blur in an ng-repeat before the preceding item

There's some discussion about this in #5402

I'd say we give it a shot and see if it causes any problems. Using $evalAsync was suggested by @caitp in #4979 (comment) .

Fixes #5945
Closes #5402

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#6910)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[Narretz]

Other options include using $timeout() and if (!scope.$$phase) {...}

[IgorMinar]

@Narretz can you please refactor this PR to use if (!scope.$$phase) {...}.

We can't use evalAsync because that desynchronizes the DOM event from our event handler, which means that the event will bubble up by the time we call the the user code to respond to it.

I'm definitely not crazy about this solution, but it's better than the alternatives.

oh and also, instead of creating duplicate forEach loop, just check the phase and then either call apply or don't.

[Narretz]

@IgorMinar okay, will do

[quantizor]

I'm not sure $evalAsync is the right choice for event handlers. The response time should really be instant, such that preventDefault() and stopPropagation() can work correctly inside the handler function.

I'd say it's more on the developer's side to implement one of these solutions inside his/her own function, rather than have it be automatic.

[Narretz]

yeah, the travis build failed, so you might be right. ;)

[caitp]

That's a good point @3lux, we might need a "safe-$apply" pattern after all :(

[jbedard]

How about doing the $$phase check already done in many other places? This would also allow manually triggered/simulated events to be done without having to be async or check $$phase...

[Narretz]

@jbedard Can you provide an example (or test) where this error will happen when using triggered events? I could include a test then.

[jbedard]

http://plnkr.co/edit/qsfLFZsxSaAaQsLSDmnV?p=preview

Whenever programmatically invoking an event such as dom.focus() or using the jQuery .trigger(event) or .event() to manually trigger events which also have ng-event directives on them. In this case I generally put a setTimeout(..., 0) around the element.event() to avoid it.

The other case is harder to avoid, but I can't reproduce atm. But if an element gets removed by angular (during a digest) and it has focus while being removed then it may fire a blur event (synchronously as part of the DOM removal) which will try to start another digest (if ng-blur is being used). This might have only been in IE? I tried doing it in the plunkr but it's not reproducing it.

[Narretz]

@IgorMinar The commit is updated. Test failures are unrelated.
@jbedard Thanks for the example plunker. I have a test that checks if ngFocus get called when an ngClick handlers triggers focus (using browserTrigger), so we're good there. Regarding the other error, we can always add it later.

[jbedard]

@Narretz Will the exception handling be the same in both cases? And don't you have to check $rootScope.$$phase? I seem to recall scope.$$phase not always being set (if it is isolated?)?

Here's what I've been using...

    try {
        fn(scope, {$event:event});
    }
    catch (e) {
        $exceptionHandler(e);
    }

    if (!$rootScope.$$phase) {
        $rootScope.$apply();
    }

[Narretz]

@jbedard This is the way I know, and have used before. Actually, input uses the same pattern (see last result in link), low level apis use $rootScope ... None have special exception handling.
https://github.com/angular/angular.js/search?q=scope.%24%24phase&ref=cmdform&type=Code

[jbedard]

I guess if we're in a digest already then the exception handling will be done elsewhere? But if the event is triggered programmatically exceptions will now be different depending if it is within a digest or not, would that be wanted?

I swear I had to use $rootScope for some reason, I guess I'll let you know if I find it or reproduce the blur on remove case.

[IgorMinar]

This looks good.

@petebacondarwin you wrote some docs about digest in progress error. will those docs need to be updated?

[petebacondarwin]

@IgorMinar - this is the thing we spent ages discussing a while back. I am pretty sure that we decided that is was not right to try to fix this in the way that it is being fixed here. This is why I wrote this extended explanation of exactly this scenario: https://docs.angularjs.org/error/$rootScope/inprog#diagnosing-this-error

[Narretz]

@petebacondarwin Is it possible to have a test for this that fails even with this fix? I have even used this fix myself and recommended it, and it has always worked in the specific cases that have tests now. Please note that the fix doesn't use $evalAsyncas the title says, but checks the $$phase

[petebacondarwin]

@Narretz - the explanation for why this is not ideal is set out here by @IgorMinar.

We looked at this again last night and while I could rewrite your two failing unit tests in a way that does not fail even without the $$phase checking approach, there is a problem with the ngRepeat issue.

In that case, it appears that when the browser moves a DOM element that has focus, it briefly blurs and then refocuses it, thus triggering both blur and focus events. In ngRepeat, we reorder DOM elements when the underlying repeated items are reordered. If one of the items that moves has focus then we get these unwanted blur then focus events occurring, and because the moves are run inside a $apply block then you get this inprog exception.

Although we could get rid of the exception using your PR, it doesn't actually stop an underlying issue in that when we move ngRepeat items, blur and focus events occur when they really shouldn't, or at least are not expected. We need to look at this a bit further.

[IgorMinar]

I reviewed this one again with @tbosch and he got an idea that we could just delay the evaluation of the focus/blur expression via postDigest or evalAsync and thus avoid the whole digest in progress issue. I think that's how this PR was originally submitted. I'll explore that more next week and get this resolved.

[jbedard]

Then the focus/blur expression can't .preventDefault() or .stopPropagation() though...?

[tbosch]

blur and focus don't bubble, and cannot be defaultPrevented (see https://developer.mozilla.org/en-US/docs/Web/Events/focus and https://developer.mozilla.org/en-US/docs/Web/Events/blur), so that should not be a problem.

[jbedard]

True, I guess I was thinking of focusin/out (which don't have ngEvents right now). It will still change the event order though, so the order ngEvent expressions get executed vs order of DOM events will be different. For example it might change the ngBlur vs ngChange order?

[tbosch]

True, but I don't think it is a good practice to rely on this order...

On Mon, Aug 25, 2014 at 12:26 PM, Jason Bedard [email protected]
wrote:

True, I guess I was thinking of focusin/out (which don't have ngEvents
right now). It will still change the event order though, so the order
ngEvent expressions get executed vs order of DOM events will be different.
For example it might change the ngBlur vs ngChange order?

—
Reply to this email directly or view it on GitHub
#6910 (comment).

[tbosch]

Btw, here is the spec that shows which events are sync and async: https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#h3_event-types-list

[Narretz]

Yes, with $evalAsnyc this would now come full circle. I've used it like this and never had any problems.