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

feat($rootScope): Implement stopPropatagion for $broadcast events #15877

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat($rootScope): Implement stopPropatagion for $broadcast events #15877

wants to merge 1 commit into from

Conversation

StrangelyTyped
Copy link

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

What is the current behavior? (You can also link to an open issue here)
$scope.$broadcast events cannot be cancelled and do not have a stopPropagation() function/method

What is the new behavior (if this is a feature change)?
This change allows any scope to prevent it's children from receiving that
event by calling stopPropagation on the event object. Other listeners on the scope which called
stopPropagation will continue to receive the event, as will siblings of that scope and any
children of those siblings.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

$scope.$broadcast dispatches an event to all child scopes by traversing the scope tree in a
depth-first manner. This change allows any scope to prevent it's children from receiving that
event by calling stopPropagation on the event object. Other listeners on the scope which called
stopPropagation will continue to receive the event, as will siblings of that scope and any
children of those siblings.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@StrangelyTyped
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Apr 1, 2017
@StrangelyTyped
Copy link
Author

StrangelyTyped commented Apr 1, 2017

I'm aware (though didn't spot it until after I'd already done mine) that there has been a previous PR for this functionality here.

I don't care which of these is preferred - mine or just-boris' - but I would like to see one of them accepted for the following reasons:

  • This change brings $broadcast and it's dispatched event object into line with $emit and it's event.
  • $broadcast-ed events and $emit-ed events are both dispatched through $on, but at present only one has a stopPropagation method, meaning $on needs to be aware of whether or not the event came from above via $broadcast or below via $emit.
  • Any arguments for needing stopPropagation on $emit-ed events also applies to $broadcast.
  • Given the angular movement towards component-heirarchy-based applications it is likely to become more common to need to intercept broadcast events to handle them at a higher level and either suppress it or broadcast a modified event instead.
  • The established 'workaround' for lack of stopPropagation is to use preventDefault, but this only works if the author has control over both event handlers to check preventDefault which is not always the case. It also affects all subsequent receivers not just the ones below the one that called preventDefault.

// (though it differs due to having the extra check for $$listenerCount)
if (!(next = ((current.$$listenerCount[name] && current.$$childHead) ||
// (though it differs due to having the extra check for $$listenerCount and stopPropagation)
if (!(next = ((current.$$listenerCount[name] && !stopPropagation && current.$$childHead) ||
(current !== target && current.$$nextSibling)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

PR #12672 has an additional check here, why not this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Actually it doesn't, it has an assignment here, since it's not part of the condition and sin e the condition is already confusing enough i put mine on line 1310

@Narretz
Copy link
Contributor

Narretz commented Apr 19, 2017

Personally, I'm kind of indifferent about this change, with the following caveat:

The scope event system in AngularJS is kind of bolted on the scope system and should not be a cornerstone of application design. Componentized app architecture should imo rely on the bindings as clearer interfaces - events rely on scopes, component bindings abstract them. So I don't think component archtitecture creates a big need for this - otherwise it would have been requested before and more loudly.

@Narretz Narretz modified the milestones: 1.6.x, 1.7.x Apr 12, 2018
@petebacondarwin petebacondarwin modified the milestones: 1.7.x, 1.7.x - won't fix May 16, 2018
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.

5 participants