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
ncuillery 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 all 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
60 changes: 38 additions & 22 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,25 +1029,33 @@ function $RootScopeProvider(){
* @return {Object} Event object (see {@link ng.$rootScope.Scope#$on}).
*/
$emit: function(name, args) {
function Event(currentScope) {
this.name = name;
this.currentScope = currentScope;
this.targetScope = target;
this.defaultPrevented = globalDefaultPrevented;
}
Event.prototype.preventDefault = function() {
// Deals with the current instance (this) and also the next ones with globalDefaultPrevented.
this.defaultPrevented = true;
globalDefaultPrevented = true;
};
Event.prototype.stopPropagation = function() {
stopPropagation = true;
};

var empty = [],
namedListeners,
target = this,
scope = this,
stopPropagation = false,
event = {
name: name,
targetScope: scope,
stopPropagation: function() {stopPropagation = true;},
preventDefault: function() {
event.defaultPrevented = true;
},
defaultPrevented: false
},
listenerArgs = concat([event], arguments, 1),
globalDefaultPrevented = false,
event = new Event(scope),
listenerArgs,
i, length;

do {
namedListeners = scope.$$listeners[name] || empty;
event.currentScope = scope;
for (i=0, length=namedListeners.length; i<length; i++) {

// if listeners were deregistered, defragment the array
Expand All @@ -1059,6 +1067,8 @@ function $RootScopeProvider(){
}
try {
//allow all listeners attached to the current scope to run
event = new Event(scope);
listenerArgs = concat([event], arguments, 1);
namedListeners[i].apply(null, listenerArgs);
} catch (e) {
$exceptionHandler(e);
Expand All @@ -1069,7 +1079,6 @@ function $RootScopeProvider(){
//traverse upwards
scope = scope.$parent;
} while (scope);

return event;
},

Expand All @@ -1096,23 +1105,28 @@ 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 = globalDefaultPrevented;
}
Event.prototype.preventDefault = function() {
// Deals with the current instance (this) and also the next ones with globalDefaultPrevented.
this.defaultPrevented = true;
globalDefaultPrevented = true;
};
Copy link
Contributor

@Fresheyeball Fresheyeball May 14, 2014

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

@ncuillery ncuillery May 14, 2014

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

@Fresheyeball Fresheyeball May 14, 2014

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

@ncuillery ncuillery May 14, 2014

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

@Fresheyeball Fresheyeball May 14, 2014

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

@lgalfaso lgalfaso May 14, 2014

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

@Fresheyeball Fresheyeball May 15, 2014

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

@lgalfaso lgalfaso May 19, 2014

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

@Fresheyeball Fresheyeball May 19, 2014

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

@lgalfaso lgalfaso May 20, 2014

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,
globalDefaultPrevented = false,
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 +1138,8 @@ function $RootScopeProvider(){
}

try {
event = new Event(current);
listenerArgs = concat([event], arguments, 1);
listeners[i].apply(null, listenerArgs);
} catch(e) {
$exceptionHandler(e);
Expand Down
23 changes: 21 additions & 2 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,10 @@ describe('Scope', function() {
});
grandChild.$emit('myEvent');
expect(event).toBeDefined();

// Asynchronous expectation : current & target should be the same as above
expect(event.targetScope).toBe(grandChild);
expect(event.currentScope).toBe(child);
});


Expand Down Expand Up @@ -1693,26 +1697,41 @@ describe('Scope', function() {
expect(result.name).toBe('some');
expect(result.targetScope).toBe(child1);
});

it('should have preventDefault method and defaultPrevented property', function() {
var event = child2.$broadcast('myEvent');
expect(event.defaultPrevented).toBe(false);

grandChild21.$on('myEvent', function(event) {
event.preventDefault();
});
event = child2.$broadcast('myEvent');
expect(event.defaultPrevented).toBe(true);
});

});


describe('listener', function() {
it('should receive event object', inject(function($rootScope) {
var scope = $rootScope,
child = scope.$new(),
grandChild = child.$new(),
event;

child.$on('fooEvent', function(e) {
expect(e.targetScope).toBe(scope);
expect(e.currentScope).toBe(child);
expect(e.name).toBe('fooEvent');
event = e;
});
scope.$broadcast('fooEvent');

expect(event.name).toBe('fooEvent');
// Asynchronous expectation : current & target should be the same as above
Copy link
Contributor

@caitp caitp May 20, 2014

Choose a reason for hiding this comment

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

wat

Copy link
Contributor Author

@ncuillery ncuillery May 20, 2014

Choose a reason for hiding this comment

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

I just moved this expectation few lines above (in child.$on listener function L1725).

I made this to be consistent with the existing similar test case for $emit L1570.

Copy link
Contributor

@caitp caitp May 20, 2014

Choose a reason for hiding this comment

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

it's not asynchronous, and you should not give the impression that it's asynchronous

I made this to be consistent with the existing similar test case for $emit

That's a test case you wrote yourself, it's not present in angular core! Just remove the "Asynchronous expectation..." bit

Copy link
Contributor Author

@ncuillery ncuillery May 20, 2014

Choose a reason for hiding this comment

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

https://github.com/angular/angular.js/blob/master/test/ng/rootScopeSpec.js#L1570

Copy link
Contributor

@caitp caitp May 20, 2014

Choose a reason for hiding this comment

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

You understand that I'm referring to the comment, right?

That scope event handler is not being called asynchronously, there is nothing asynchronous happening here, it's all within a single turn of the event loop

Copy link
Contributor Author

@ncuillery ncuillery May 20, 2014

Choose a reason for hiding this comment

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

Oops. No, I didn't. Sorry for that.

expect(event.targetScope).toBe(scope);
expect(event.currentScope).toBe(child);
}));


it('should support passing messages as varargs', inject(function($rootScope) {
var scope = $rootScope,
child = scope.$new(),
Expand Down