Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Open
StrangelyTyped wants to merge 1 commit into angular:master
base: master
Choose a base branch
Loading
from StrangelyTyped:feature/scope-broadcast-stoppropagation
Open

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

StrangelyTyped wants to merge 1 commit into angular:master from StrangelyTyped:feature/scope-broadcast-stoppropagation

Conversation

@StrangelyTyped
Copy link

@StrangelyTyped StrangelyTyped commented Apr 1, 2017

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.
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
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.

Copy link
Author

I signed it!

Copy link

CLAs look good, thanks!

1 similar comment
Copy link

CLAs look good, thanks!

Copy link
Author

StrangelyTyped commented Apr 1, 2017
edited
Loading

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.

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

@StrangelyTyped StrangelyTyped Apr 19, 2017

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

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.

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

Reviewers

2 more reviewers

@Narretz Narretz Narretz left review comments

@3imed-jaberi 3imed-jaberi 3imed-jaberi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /