fix(dropdown): Fix $digest:inprog on dropdown dismissal

[maxfierke]

I was working on a project leveraging Angular-UI with a directive that called $document.click() to dismiss dropdowns, and ran some callbacks and such, and I was running into a $digest:inprog exception in closeDropdown.

The prevailing opinion on $scope.$apply seems to be that it shouldn't be called directly, and that $timeout should be used instead.

Tested with Angular-UI 0.12 on Angular 1.2.27.

Signed-off-by: Max Fierke [email protected]

[wesleycho]

I'm a bit confused why we would get a $digest in progress error in that method. That method is only called on events on document, where an $apply is necessary. Can you explain this further?

[chrisirhc]

This is because the click event was triggered manually by a $document.click() call from another directive while in a digest cycle.
Instead of doing $timeout here, it might be worthwhile to check $rootScope.$$phase . As unpleasant as that sounds, we have no way of ensuring that users don't trigger click within a digest phase. So checking the undocumented $$phase attribute maybe needed here.

[wesleycho]

Hm, can't say I like the solution terribly much, but I think it is correct.

Can you make the modification to check if !$rootScope.$$phase before executing $rootScope.$digest?

[maxfierke]

For sure. I'll try to get something up tomorrow.

[maxfierke]

Hmmm... while checking $rootScope.$$phase does prevent the the $digest:inprog it also swallows the attempt to close the dropdown. Using $timeout effectively "schedules" it for the next digest, retaining the expected behavior of dismissing the dropdowns.

I can post the change for checking $rootScope.$$phase, but it's kind of a half-fix for the issue. The $timeout solution better addresses it. Are there any specific concerns you both have with regards to the use of $timeout?

[wesleycho]

Is there any particular reason why the dropdown would not be dismissed by this check? That seems strange to me.

[wesleycho]

Can you explain why checking for $rootScope.$$phase does not fully fix this in terms of code execution?

[maxfierke]

If we do the check like such

if (!openScope.$$phase) {
      openScope.$apply(function() {
        openScope.isOpen = false;
      });
}

then openScope.isOpen won't be set to false and the dropdown will stay open (closeDropdown is essentially a noop here).

We could instead do

if (!openScope.$$phase) {
      openScope.$apply(function() {
        openScope.isOpen = false;
      });
} else {
      openScope.isOpen = false;
}

but that doesn't feel like a cleaner solution compared to just using $timeout instead of.$scope.$apply.

[chrisirhc]

I was actually referring to doing $apply only when it's needed. You can do:

openScope.isOpen = false;
if (!$rootScope.$$phase) {
    openScope.$apply();
}

Note that you need to check $$phase of the $rootScope. Checking the $$phase of the openScope isn't adequate because there maybe a digest phase right now but it hasn't yet traversed to the openScope in the scope hierarchy.

Edit: Looking at the source code of Angular, looks like only the $rootScope's $$phase is meaningful. That attribute on any other scope isn't useful and is probably always null.

[maxfierke]

Doh! 😊 Yup, that makes sense. I'll rollback and condense it all into one patch. Forgot that you can call $scope.$apply without a callback.

[chrisirhc]

@maxfierke , mind adding a test for this? You can do the $document.click in your test 😄

[maxfierke]

Done 😄

[wesleycho]

Thanks, merged it in!