fix(ngEventDirs): check for $rootScope.$$phase in event handler and d… #14674

markgardner · 2016-05-25T19:45:30Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#14673

Digest cycle already in progress error can inadvertently be caused when triggering an
element's click event while within an active digest cycle. This is due to the ngEventsDirs
event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase.
Checking the phase and calling the function immediately if within an active digest cycle
will prevent the problem without reducing current functionality.

What is the new behavior (if this is a feature change)?
This will change ngClick and the like to check for $rootScope.$$phase to determine if it should call the callback function without entering a new $apply.

Does this PR introduce a breaking change?
Nope

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

googlebot · 2016-05-25T19:45:31Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-05-25T19:45:31Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-05-25T19:47:31Z

CLAs look good, thanks!

googlebot · 2016-05-25T19:47:31Z

CLAs look good, thanks!

gkalpak · 2016-05-25T22:23:07Z

src/ng/directive/ngEventDirs.js

@@ -65,6 +65,8 @@ forEach(
              };
              if (forceAsyncEvents[eventName] && $rootScope.$$phase) {
                scope.$evalAsync(callback);
+              } else if ($rootScope.$$phase) {


In order to avoid double checking $rootScope.$$phase and simplify, I might change the whole block to:

if (!$rootScope.$$phase) { scope.$apply(callback); } else if (forceAsyncEvents[eventName]) { scope.$evalAsync(callback); } else { callback(); }

Great point, I've updated the commit to reflect this.

gkalpak · 2016-05-25T22:23:14Z

Technically speaking, there are ways to avoid this (e.g. wrapping the call to click() in a $timeout or preventing event propagation) and having an ngClick handler on an element that calls .click() on a sibling element, while their parent element also has an ngClick handler isn't a very common usecase 😃

But then again, why not ? It has minimal overhead and is a reasonable change 👍

LGTM

markgardner · 2016-05-25T22:38:16Z

@gkalpak very true, I have had this happen a couple of times and normally worked around it.

jbedard · 2016-05-25T22:51:34Z

I think the reason this was never done is because it makes element.click() unpredictable. With this change the event handler might be async or might not. If it is async then things like event.preventDefault() or event.stopPropagation() won't work, event order might become unpredictable etc. Blur and focus were special cases because they are sometimes automatically triggered by the browser when manipulating the dom (at least blur is).

markgardner · 2016-05-25T23:38:00Z

@jbedard that makes sense that would be a concern. Though I don't think this change will effect that. The biggest difference is whether it will use $rootScope.$apply or a regular function call to call the event handler. Both should be synchronous if I understand the code correctly.

jbedard · 2016-05-26T00:05:19Z

My bad then! I guess it's only the forceAsyncEvents have that behavior and this doesn't change that. I wonder if it is worth moving the expect($rootScope.click).toHaveBeenCalledOnce() into the $apply to verify this?

gkalpak · 2016-05-26T09:27:12Z

Previously if an non-async ng-event was fired while a $digest was in progress, there would be an error. With this PR, such calls will execute the callback directly (without $apply()).

So, the only change in terms of behavior is that calls that would previously throw, now won't.

Moving the expectation inside the $apply block won't hurt, but it doesn't buy us anything either. It only verifies half of the story (events fired while $digest in progress) - it doesn't tell us anything about events fired outside of a $digest.

I would be fine adding adding another test that verifies the synchronous nature of both types of callback calls (events fired inside and outside of $digest).

markgardner · 2016-05-26T16:11:17Z

I've updated the PR to include another test to cover triggering an event outside of a digest cycle.

gkalpak · 2016-05-26T18:50:38Z

test/ng/directive/ngEventDirsSpec.js

+        element.triggerHandler('click');
+      });
+
+      expect($rootScope.click).toHaveBeenCalledOnce();


This doesn't actually check that it is called synchronously (e.g. it might be called outside of the $apply() block).
You could use the same approach as below (using a $watch).

I've updated the PR to use the same checks for both tests

…on't $apply if already in $digest Digest cycle already in progress error can inadvertently be caused when triggering an element's click event while within an active digest cycle. This is due to the ngEventsDirs event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase. Checking the phase and calling the function immediately if within an active digest cycle will prevent the problem without reducing current functionality. Closes #14673

…on't $apply if already in $digest Digest cycle already in progress error can inadvertently be caused when triggering an element's click event while within an active digest cycle. This is due to the ngEventsDirs event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase. Checking the phase and calling the function immediately if within an active digest cycle will prevent the problem without reducing current functionality. Closes angular#14673 Closes angular#14674

…n $digest Digest cycle already in progress error can inadvertently be caused when triggering an element's click event while within an active digest cycle. This is due to the ngEventsDirs event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase. Checking the phase and calling the function immediately if within an active digest cycle will prevent the problem without reducing current functionality. Closes angular#14673 Closes angular#14674

…n $digest Digest cycle already in progress error can inadvertently be caused when triggering an element's click event while within an active digest cycle. This is due to the ngEventsDirs event handler always calling $rootScope.$apply regardless of the status of $rootScope.$$phase. Checking the phase and calling the function immediately if within an active digest cycle will prevent the problem without reducing current functionality. Closes #14673 Closes #14674

googlebot added the cla: no label May 25, 2016

googlebot added cla: yes and removed cla: no labels May 25, 2016

gkalpak added type: feature frequency: low component: misc core severity: inconvenient labels May 25, 2016

gkalpak added this to the Backlog milestone May 25, 2016

gkalpak reviewed May 25, 2016
View reviewed changes

gkalpak reviewed May 26, 2016
View reviewed changes

Narretz self-assigned this Jul 12, 2018

Narretz closed this in 6b0193e Jul 25, 2018

dependencies bot mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngEventDirs): check for $rootScope.$$phase in event handler and d… #14674

fix(ngEventDirs): check for $rootScope.$$phase in event handler and d… #14674

markgardner commented May 25, 2016 •

edited

Loading

googlebot commented May 25, 2016

googlebot commented May 25, 2016

googlebot commented May 25, 2016

googlebot commented May 25, 2016

gkalpak May 25, 2016

markgardner May 25, 2016

gkalpak commented May 25, 2016

markgardner commented May 25, 2016

jbedard commented May 25, 2016

markgardner commented May 25, 2016

jbedard commented May 26, 2016

gkalpak commented May 26, 2016

markgardner commented May 26, 2016

gkalpak May 26, 2016

markgardner Jun 1, 2016

fix(ngEventDirs): check for $rootScope.$$phase in event handler and d… #14674

fix(ngEventDirs): check for $rootScope.$$phase in event handler and d… #14674

Conversation

markgardner commented May 25, 2016 • edited Loading

googlebot commented May 25, 2016

googlebot commented May 25, 2016

googlebot commented May 25, 2016

googlebot commented May 25, 2016

gkalpak May 25, 2016

Choose a reason for hiding this comment

markgardner May 25, 2016

Choose a reason for hiding this comment

gkalpak commented May 25, 2016

markgardner commented May 25, 2016

jbedard commented May 25, 2016

markgardner commented May 25, 2016

jbedard commented May 26, 2016

gkalpak commented May 26, 2016

markgardner commented May 26, 2016

gkalpak May 26, 2016

Choose a reason for hiding this comment

markgardner Jun 1, 2016

Choose a reason for hiding this comment

markgardner commented May 25, 2016 •

edited

Loading