Bug Tracker

Opened 8 years ago

Closed 7 years ago

#10699 closed bug (wontfix)

Calling stopPropagation() before trigger() doesn't call handlers on element

Reported by: gyoshev Owned by: dmethvin
Priority: low Milestone: 1.next
Component: event Version: git
Keywords: Cc:
Blocked by: Blocking:

Description

Calling stopPropagation() on an event object before triggering it doesn't call any immediate event handlers. This works for any jQuery version before 1.7. Test case: http://jsfiddle.net/UG7bF/

Change History (17)

comment:1 Changed 8 years ago by dmethvin

Component: unfiledevent
Milestone: None1.7.1
Owner: set to dmethvin
Priority: undecidedhigh
Status: newassigned

Yep, it was caused by adding support for .stopPropagation on delegated events. Relatively easy to fix though.

comment:2 Changed 8 years ago by dmethvin

Hmmm, looking at this some more it isn't as easy to fix as I had thought. Can you describe the situation where you have an event with propagation stopped that you want to trigger?

comment:3 in reply to:  2 Changed 8 years ago by gyoshev

Replying to dmethvin:

Hmmm, looking at this some more it isn't as easy to fix as I had thought. Can you describe the situation where you have an event with propagation stopped that you want to trigger?

We're using that for custom events on nested components. For example, if you have a tabstrip within a tabstrip and want to trigger a custom event (i.e. "select" for selecting a tab) only on the nested component. If the event propagates, it would trigger handlers on the outer component. On the other hand, if the event doesn't trigger at all, none of its immediate event handlers will be executed.

comment:4 Changed 8 years ago by dmethvin

Can't you use .triggerHandler() for that, since there is no default action for your custom event? That doesn't bubble.

comment:5 Changed 8 years ago by anonymous

Just tried it out -- unfortunately, triggerHandler internally calls preventDefault(), which in turn reports that every triggered custom event has a prevented default action. That is not desired, as no user code has done this. Consider the following:

var e = $.extend({ url: "/foo" }, new $.Event("dataRequest"));
$(element).triggerHandler(e);
// now e.isDefaultPrevented() == true, yet none of the handlers
// has prevented the dataRequest

comment:6 Changed 8 years ago by dmethvin

Yeah, I agree with that and it's a legitimate use case. I wonder, however, if that should be considered a bug with triggerHandler, which doesn't document that it has a side-effect like that. It's pretty easy to fix that in .trigger() by not calling event.preventDefault() and checking the onlyHandlers param before trying the default action.

What do you think?

comment:7 Changed 8 years ago by gyoshev

Sure, that should do the trick -- I just tried it out with our codebase and it works perfectly. Plus, triggerHandler() seems more appropriate in this use case. Great thing about the fix is that it actually results in a better jQuery size (one if less), so bytes are actually removed when fixing it. Should the side-effect be logged as a new bug so that it gets to the release notes for 1.7.1?

comment:8 Changed 8 years ago by dmethvin

Blocked by: 10794 added

comment:9 Changed 8 years ago by dmethvin

Resolution: wontfix
Status: assignedclosed

Yes, I agree, it seems logically right and keeping the code small doesn't hurt either. This is a behavior change, but I don't think it will be very common and we've provided a better solution by fixing .triggerHandler().

Thanks for your help and advice on this. I've opened #10794 and will get it patched for 1.7.1!

comment:10 Changed 7 years ago by maranomynet

So we need to change our code from using a simple .trigger(ev) to .each(function(){ $(this).triggerHandler(ev); });

This seems like a woefully inelegant workaround for this bug.

You should at the very least document this behavioral oddity that a early .stopPropagation() effectively *disables* the Event.

comment:11 Changed 7 years ago by dmethvin

This case isn't something that can ever happen naturally; an event is never delivered from the browser with propagation initially stopped. If you just want to run handlers because it's not a browser-related event, then .triggerHandler() does seem like the correct choice here, especially since .trigger() will attempt to run a method with the same name as the event.

comment:12 Changed 7 years ago by maranomynet

Actually it's worse than it appears.

Wontfixing this bug along with bug #10794 causes an unfortunate conflict where I can't fix my existing code to work with jQuery 1.7.1 without breaking backwards compatibility with older jQuery versions. (Which I can't guarantee will never be used in production.)

So now I have to fork my code based on jQuery versions.

Moreover, I have several cases where .trigger() is what I really really want, and I wanna do .stopPropagation() only when plugin's options.noBubble is set to true.

Again I need to fork the whole .triggering mechanism based on the value of .noBubble.

Pretty please consider reopening and fixing this bug.

jQuery.Event() is such a neat method and offers such a clean way for advanced triggering of events (custom or not). It a shame to see this bug ruin its elegance and make it all kludgy, all of a sudden.

comment:13 Changed 7 years ago by gyoshev

You can do a test of what the jQuery version supports the fixed triggerHandler or trigger+stopPropagation -- that's what we did in the end:

var useTriggerHandler = (function(){
        var e = new $.Event("triggerHandlerTest");
        $("<div />").triggerHandler(e);
        return !e.isDefaultPrevented();
    })();

function trigger(element, type, e) {
    e = $.extend(e || {}, new $.Event(type));

    if (useTriggerHandler) {
        $(element).triggerHandler(e);
    } else {
        e.stopPropagation();
        $(element).trigger(e);
    }

    return e.isDefaultPrevented();
}

Theoretically, you can modify this to make it depend on your plugin options.

It introduces a fork in your code if you decide wish to be backwards-compatible, which is usually the case with plug-ins.

comment:14 Changed 7 years ago by maranomynet

gyoshev, thanks for illustrating perfectly what I was pointing out... Working around this bug is a horrible mess.

Please fix the bug.

comment:15 Changed 7 years ago by maranomynet

Here's my actual use-case, and I hope you consider it a valid one:

"I'd like to create and trigger a *native* event, but I don't want it to bubble up the DOM, and I want the handlers to be able to determine that via .isPropagationStopped()."

As of jQuery 1.7.1 there is *no* workable way to accomplish this.

comment:16 Changed 7 years ago by dmethvin

Blocked by: 10794 removed
Milestone: 1.7.11.next
Priority: highlow
Resolution: wontfix
Status: closedreopened

@maranomynet, I closed this ticket because I thought we'd addressed the common use cases. If I am to interpret your snarky posts and tweets, we haven't addressed a use case that is important to you.

Can you describe your situation a bit more? Is there a reason why the handlers themselves cannot stop propagation?

comment:17 Changed 7 years ago by dmethvin

Resolution: wontfix
Status: reopenedclosed

The ticket's been open for a while with no discussion, so I will close it again.

The reason this is difficult to revert is that we need to take delegated events into account. If a synthesized event arrives at jQuery.event.dispatch with propagation stopped, there may be delegated events. Should we run those, even though propagation is supposedly stopped? If so, why are we then running the non-delegated events for this element and only then stopping? This kind of behavior is never caused by the browser since it won't ever call our handler with propagation stopped.

Note: See TracTickets for help on using tickets.