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

Fix for event.stopPropagation() and $emit method #3639

Closed
wants to merge 10 commits into from
Closed

Fix for event.stopPropagation() and $emit method #3639

wants to merge 10 commits into from

Conversation

stsvilik
Copy link

This fix addresses an issue where event.stopPropagation() prematurely ends execution of event listeners assigned to the same scope from which they are $emit-ted.

For example you assign 2 listeners to the same scope:
a)

scope.on('myEvent', function(evt) {
  evt.stopPropagation();
  //Do something
});

b) Somewhere else on the same scope, perhaps inside other directive or controller

scope.on('myEvent', function(evt) {
  //Do something else
});

Currently handler in 'a' will prevent handler 'b' from executing even though they are on the same exact scope, but this is not the intention of the stopPropagation() function. Intention is to prevent 'myEvent' from bubbling into $parent scope.

If we take jQuery or just plain DOM eventing model, we'll see that stopPropagation does NOT prevent multiple handlers that are attached to the same node from firing.

stsvilik and others added 7 commits February 20, 2013 19:27
Changing $document.find to angular.element permits more complex selectors when using jQuery. Otherwise you're limited to selecting only by ID's and element tags which is very restricting in the real life testing scenarios.
Hidden fields are still used in hybrid solutions such as MVC.net/etc. and it is valuable to be able to set value of this type of field through ng-model.
Extend function of Angular blindly overwrites destination values even if source value is undefined. This fix allows "default" values to remain intact if new value is undefined.
- re-applied the fix to the $emit method
- improved the the unit test
- fixed spacing issue
@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

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!

@stsvilik
Copy link
Author

CLA signed, I tried to clean-up my pull request history but not sure if it worked :/. Github blows when it comes to cherry-picking what you want to PULL. So for clarification sake, my pull should only be for c0c5d8e.

@btford
Copy link
Contributor

btford commented Aug 21, 2013

This PR is a mess, but the final diff looks ok.

@stsvilik
Copy link
Author

Agreed (re:mess) this was due to some old PR's that were done w/o following formal commit instructions (sorry for that), so I had to revert them. It also took me a while to get into the PR procedure flow. I promise it will not happen again.

@btford
Copy link
Contributor

btford commented Aug 22, 2013

It's ok. Just rebase and squash them (see git rebase -i) please. :)

@stsvilik
Copy link
Author

When now? :) or you talking about the future PR's?

@btford
Copy link
Contributor

btford commented Aug 28, 2013

For this one, as well as in the future. :)

@btford btford closed this Sep 30, 2013
@ghost ghost assigned btford Sep 30, 2013
@btford btford reopened this Sep 30, 2013
@btford
Copy link
Contributor

btford commented Sep 30, 2013

whoops, wrong thread. ignore me :)

@btford
Copy link
Contributor

btford commented Sep 30, 2013

I'm going to close this for now due to inactivity. If you can rebase against master and clean up the commit history, I'd be happy to re-open this.

@btford btford closed this Sep 30, 2013
@stsvilik
Copy link
Author

stsvilik commented Oct 1, 2013

Brian I had to start from the very beginning. Please refer to the new pull request I created: #4204

Thank You

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants