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

fix($rootScope): avoid unstable reference in $broadcast event #7445

Closed
wants to merge 3 commits into from
Closed

fix($rootScope): avoid unstable reference in $broadcast event #7445

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b138db
fix($rootScope): avoid unstable reference in $broadcast event
ncuillery May 13, 2014
814f0e0
style: add missing semi-colon
ncuillery May 13, 2014
bd131bb
fix($rootScope): avoid unstable reference in $emit event
ncuillery May 15, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,23 +1096,25 @@ function $RootScopeProvider(){
* @return {Object} Event object, see {@link ng.$rootScope.Scope#$on}
*/
$broadcast: function(name, args) {
function Event(currentScope) {
this.name = name;
this.currentScope = currentScope;
this.targetScope = target;
this.defaultPrevented = false;
}
Event.prototype.preventDefault = function() {
this.defaultPrevented = true;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It defines a javascript class which is instantiated for each listener among the tree of scopes.

By this way, each listener received a dedicated event with a stable currentScope property.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why does this class need a preventDefault? There is not action being taken there other then altering this.defaultPrevented. This seems misleading to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I retained the existing preventDefault.

I asked myself for the purpose of this property. Perhaps to be consistent with DOM events.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it should be removed, this may have been a mistake in the angular api. Events without a native default should not have a preventDefault method. @lgalfaso thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they should have preventDefault even when in the case of a $broadcast it does nothing

Copy link
Contributor

Choose a reason for hiding this comment

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

@lgalfaso why? I don't believe it does anything for $emit either. Its just basically a misleading noop.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fresheyeball the event used to have a function called preventDefault and that should not be removed, user may call it (even if it does nothing).
In the case of $emit, calling preventDefault does prevent the event from going up

Copy link
Contributor

Choose a reason for hiding this comment

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

@lgalfaso thats even worse. Why would preventDefault be stopPropagation? There is no browser default, so there should be no prevent default. That is exactly why this is bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fresheyeball There are two things involved there

This PR is to fix an existing issue with events current parameter (that affects $broadcast and $emit). As part of this fix, it is required that this does not break what people expect from the API, this includes that there is a function called preventDefault. How this function works is not part of this PR.

Now, preventDefault is part of the DOM standard http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-preventDefault. This patch is not about adding/removing or being consistent on the fact that there is a preventDefault function and not stopPropagation nor on the fact that preventDefault works in one way when calling $broadcast and in another way when calling $emit. If you feel that this is a limitation you are welcome to put a PR in place


var target = this,
current = target,
next = target,
event = {
name: name,
targetScope: target,
preventDefault: function() {
event.defaultPrevented = true;
},
defaultPrevented: false
},
listenerArgs = concat([event], arguments, 1),
listenerArgs,
event = new Event(current),
listeners, i, length;

//down while you can, then up and next sibling or up and next sibling until back at root
while ((current = next)) {
event.currentScope = current;
listeners = current.$$listeners[name] || [];
for (i=0, length = listeners.length; i<length; i++) {
// if listeners were deregistered, defragment the array
Expand All @@ -1124,6 +1126,8 @@ function $RootScopeProvider(){
}

try {
event = new Event(current);
listenerArgs = concat([event], arguments, 1);
listeners[i].apply(null, listenerArgs);
} catch(e) {
$exceptionHandler(e);
Expand Down
1 change: 1 addition & 0 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1700,6 +1700,7 @@ describe('Scope', function() {
it('should receive event object', inject(function($rootScope) {
var scope = $rootScope,
child = scope.$new(),
grandChild = child.$new(),
event;

child.$on('fooEvent', function(e) {
Expand Down