fix(jqLite): fix event.stopImmediatePropagation() so it works as expected

[mbenford]

jqLite doesn't override the default implementation of event.stopImmediatePropagation and so it doesn't work as expected, i.e, it doesn't prevent the rest of the event handlers from being executed. I've fixed that for events handled with on and triggered by triggerHandler.

For the sake of consistency with jQuery, I've also added an isImmediatePropagationStopped function to the event object so one can tell whether the immediate propagation was stopped or not.

Lastly, this implementation won't immediate stop propagation to event handlers created with element.addEventListener. I could have easily solved that by making stopImmediatePropagation call its native counterpart, but that would render it incompatible with jQuery's implementation. In order to comply with jQuery, I would have to add an originalEvent property to the event object so one could call originalEvent.stopImmediatePropagation(), but I've found it hard to correctly implement without making the code overly complex.

[petebacondarwin]

This seems on the surface to be a duplicate of #4337, but the implementation is somewhat different. So not closing but adding to post-1.2 milestone. @mbenford perhaps you could work with @stsvilik to provide a definitive PR?

[mbenford]

Hi @petebacondarwin.

That PR and mine are just similar in concept, but not in scope. #4337 is about adding a way to immediate stop propagation of $rootScope events in the same way Javascript events do. Mine, on the other hand, is about making jqLite's event.stopImmediatePropagation work as it was supposed to, since the current implementation doesn't prevent event handlers from being executed after stopImmediatePropagation is called (actually it works for event handlers created with addEventListener, which is the native behavior, but not for the ones created with jqLite's on function, as one would expect).

Kind regards.

[stsvilik]

Seem like two different issues to me. Mine adds the stopImmediatePropagation feature to scopes where the other attempts to correct similar functionality on the DOM element level. Both are relevant.

[mbenford]

Rebased to latest code from master.

[IgorMinar]

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[mbenford]

Hi Igor.

That's odd. I had signed the CLA before I first sent the PR, a month ago, using the very same email address.

Anyway, I've just signed it again. My real name is Michael Benford.

Kind regards.

[mbenford]

Rebased to latest master.

[mbenford]

I see the feature label has been applied to this issue, but isn't it supposed to be a bug in jqLite implementation?

[alexraginskiy]

Are there any updates on this or is there a way to monkey patch this into jqLite in the meantime?

[mbenford]

Is there any update on this?

[Narretz]

+1
or add docs for the difference?
This is really useful if you want to stop ng-click handlers from custom directives, but don't want to use jquery.

[IgorMinar]

this expectation is inside of a callback - such expectations are not guaranteed to be executed so it's better to refactor this by exposing the spy outside of the closure and moving the expectation after the browserTrigger call on line 926

[mbenford]

@IgorMinar That expectation can't be moved elsewhere because any overridden property/method (stopPropagation, preventDefault etc) are deleted after the on method returns. So the only safe place I can access the event is inside that callback. I believe that's why there's another spec using that pattern as well.

I could use a flag spy in order to guarantee that the test will either pass or fail. What do you think about it?

[IgorMinar]

I'm inclined to just document the differences between jqLite and jQuery and not merge this PR because don't know of any use-case that would absolutely need it.

Angular core certainly doesn't need it and that's the primary customer for jqLite.

jqLite has no aspiration to ever be full replacement for jQuery. The goal is to have the best of jQuery in a compact format that can be bundled with the core.

On several occasions we had discussions that made us think that exposing jqLite publicly was a mistake, but I recognize that for small apps it's all they really need and big apps will almost always use jQuery anyway.

So unless you can convince me that this is something that many or most apps need, I we should just go ahead and document the difference.

[alexraginskiy]

@IgorMinar the problem as I see it, is that this is not a jquery feature, but a native DOM feature. It's exclusion breaks expected behavior. In our app, we use this to prevent directives that listen to keyboard events from triggering prematurely if another directive is also listening to the same key event.

[mbenford]

Hi Igor.

@alexraginskiy has pretty much summed it up. That's why I've named this PR "fix" instead of "feature". In addition to that, jqLite currently exposes stopImmediatePropagation() from the underlying Javascript event, even though it does nothing useful. I think that's worse than not having the method at all since people will try to use it and be confused when it doesn't work as expected. I myself had spent a significant amount of time trying to understand what was going on before sending this PR.

In case you guys decide not to fix the method, I think it should be either deleted or overridden to throw an exception so other people would be aware right away that they can't use it.

[mbenford]

Any updates on this issue? I see the PR is invalid right now and I could rebase it again on top of master, but I'd rather do it (and address Igor's comments on the code) if I know there's a chance for the PR to be eventually used.

[IgorMinar]

@caitp how do you feel about this one?

[caitp]

Lastly, this implementation won't immediate stop propagation to event handlers created with element.addEventListener. I could have easily solved that by making stopImmediatePropagation call its native counterpart, but that would render it incompatible with jQuery's implementation.

jQuery's implementation does call the native event's stopImmediatePropagation() if it's available. https://github.com/jquery/jquery/blob/d837f119c3729565103005d5d7fa89e1dd8110cb/src/event.js#L686-L692

I see that the 1.x tags are not calling native stopImmediatePropagation(), but 1.x's days are numbered in the 1.3 branch

I think @mzgol might be a better one to ask about this though.

[mbenford]

@caitp Back when this PR was sent, jQuery had a bug and the native stopImmediatePropagation method wasn't called. Now that it's fixed, this PR can indeed be updated.

[IgorMinar]

Let's get this in guys.

@mbenford can you please update this PR?

[IgorMinar]

@mzgol can you own this please?

[mbenford]

@IgorMinar Will do.

[mgol]

@IgorMinar

@mzgol can you own this please?

Sure.

[mgol]

@caitp

I see that the 1.x tags are not calling native stopImmediatePropagation(), but 1.x's days are numbered in the 1.3 branch

jQuery 1.x did get this patch as well; it's just that currently Angular is tested on 1.10.x which doesn't have it.

We'll soon be on jQuery 2.1.1, though, so it doesn't make much sense to update testing jQuery to 1.11.1 now.

[mbenford]

@mzgol Since this is going into v1.3, should I rebase the PR on a branch other than master? Or is master v1.3 already?

[caitp]

you should rebase the PR against master, it's already bitrotten against master! don't worry about the rest

[mbenford]

@caitp Thanks.

[mbenford]

I've rebased my branch on top of master and addressed Igor's concerns on the code. I've also added support for the native stopImmediatePropagation method since this is supposed to go into v1.3 and jQuery 2.x will be available there. I've updated the specs to take that into consideration, but had to comment out one expectation since it won't work against jQuery 1.10.

[mgol]

@mbenford I've just landed 9e7cb3c which switches Angular to jQuery 2.1. Could you rebase over current master so that we're sure your patch works fine after the switch? Thanks!

[mgol]

@mbenford Ping?

[mbenford]

@mzgol Pong. :) I'm sorry for not having rebased this PR yet. I've been very busy lately, but I'll try to do it tonight.

[mbenford]

@mzgol Done.

[mbenford]

@mzgol Rebased again on top of master.

[mgol]

Landed. Sorry for the delay!