New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jQuery does not reliably trigger() events added with addEventListener() #2476

Closed
rbu opened this Issue Jul 15, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@rbu

rbu commented Jul 15, 2015

jQuery 2.x still has code from 1.x around that attempts to trigger native/custom browser (not the events added with .on() ) events when you call .trigger(). For backwards compatibility, it uses the [on_event_] accessor. This does not (reliably) expose event handlers added with addEventListener though, even though some browsers expose the handler for some events.

As jQuery 2.x does not care about APIs from the stone age any more, it would be great if you instead triggered dispatchEvent so that jQuery APIs play nice with non-jQuery APIs.

References:

I should mention we encountered this issue when we tried to marry JSON Editor with jQuery Mobile on a select element. It turns out jQuery Mobile manipulates the select DOM node, then calls trigger('change') whereas JSON Editor listens for DOM changes using addEventListener().

@rbu

This comment has been minimized.

Show comment
Hide comment
@rbu

rbu Jul 15, 2015

Kudos go to @dwt for helping to debug this issue.

rbu commented Jul 15, 2015

Kudos go to @dwt for helping to debug this issue.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jul 15, 2015

Member

Well, we still support IE8 in the compat branch, it seems @dmethvin justification (in the original ticket) for leaving it as "patchwelcome" is still valid.

Although if you send us a PR with the code that works for all supported browsers, i think we can consider it.

Member

markelog commented Jul 15, 2015

Well, we still support IE8 in the compat branch, it seems @dmethvin justification (in the original ticket) for leaving it as "patchwelcome" is still valid.

Although if you send us a PR with the code that works for all supported browsers, i think we can consider it.

@markelog markelog added the Event label Jul 15, 2015

@rbu

This comment has been minimized.

Show comment
Hide comment
@rbu

rbu Jul 15, 2015

By "all supported", do you mean IE9+ and the rest from http://jquery.com/browser-support/ ?

rbu commented Jul 15, 2015

By "all supported", do you mean IE9+ and the rest from http://jquery.com/browser-support/ ?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 15, 2015

Member

It's been long enough that it's worth re-discussing here in the Github tracker, but I'm still of the opinion from trac-11047 that jQuery's event system is a layer on top of the DOM event system. For the exceptions where we end up triggering native events via DOM methods like .click(), there are special guards to prevent infinite recursion but those have been a constant source of bugs over the years.

Reaching down to the level below us is pretty complicated. When you call $(elem).trigger("randomeventname") jQuery does not know whether the browser understands that event or not. So today, in order to ensure consistent cross-browser DOM bubbling behavior it manually bubbles from the triggered target all the way up to window, executing any jQuery handlers along the way.

If the DOM dispatchEvent of a native event is also going to bubble, and it sometimes will, we cannot do it inside jQuery as well. jQuery doesn't inherently know whether a particular event name will bubble or not, that behavior would have to be determined through some size-or-time expensive process such as a lookup table (most likely impractical) or feature detect. And that list may differ even in modern browsers because of various quirks and not-yet-supported events.

jQuery doesn't know whether there are native DOM handlers attached to an element because there is no DOM getEventListeners method. That means it isn't possible to shortcut the process, we must fire these events via dispatchEvent and see what happens. On every page where jQuery is in use today, this new version would be doing an expensive bunch of work to support native listeners where none can possibly exist.

We'd still want to support triggering an event handler with additional data, as the jQuery API currently documents that. Perhaps you could say that didn't happen for native events, but if the native event caused the bubbling you still need that data to be passed to jQuery handlers attached to that element.

Normally the response would be to make this a plugin, but given how tricky this is I'm not sure a generalized solution can be crafted inside or outside jQuery. I'm going to assume that the anonymous person there tried some experiments and came to the same conclusion, but if someone wants to give it another try we could look at it.

Member

dmethvin commented Jul 15, 2015

It's been long enough that it's worth re-discussing here in the Github tracker, but I'm still of the opinion from trac-11047 that jQuery's event system is a layer on top of the DOM event system. For the exceptions where we end up triggering native events via DOM methods like .click(), there are special guards to prevent infinite recursion but those have been a constant source of bugs over the years.

Reaching down to the level below us is pretty complicated. When you call $(elem).trigger("randomeventname") jQuery does not know whether the browser understands that event or not. So today, in order to ensure consistent cross-browser DOM bubbling behavior it manually bubbles from the triggered target all the way up to window, executing any jQuery handlers along the way.

If the DOM dispatchEvent of a native event is also going to bubble, and it sometimes will, we cannot do it inside jQuery as well. jQuery doesn't inherently know whether a particular event name will bubble or not, that behavior would have to be determined through some size-or-time expensive process such as a lookup table (most likely impractical) or feature detect. And that list may differ even in modern browsers because of various quirks and not-yet-supported events.

jQuery doesn't know whether there are native DOM handlers attached to an element because there is no DOM getEventListeners method. That means it isn't possible to shortcut the process, we must fire these events via dispatchEvent and see what happens. On every page where jQuery is in use today, this new version would be doing an expensive bunch of work to support native listeners where none can possibly exist.

We'd still want to support triggering an event handler with additional data, as the jQuery API currently documents that. Perhaps you could say that didn't happen for native events, but if the native event caused the bubbling you still need that data to be passed to jQuery handlers attached to that element.

Normally the response would be to make this a plugin, but given how tricky this is I'm not sure a generalized solution can be crafted inside or outside jQuery. I'm going to assume that the anonymous person there tried some experiments and came to the same conclusion, but if someone wants to give it another try we could look at it.

@dwt

This comment has been minimized.

Show comment
Hide comment
@dwt

dwt Jul 16, 2015

Out of interest, wouldn't it be viable to switch the whole event propagation mechanism to use the browsers native facilities to make this kind of interoperability just work? I.e. all events are registered via .addEventListener() and all .trigger() does is dispatch a real event? Sure this wouldn't work for old browsers, but thats what the compatibility branch is for. No?

As for jQuery specific functionality, that should be possible quite easily by wrapping the handlers registered via jQuery in a closure that extracts the extra information from the triggered custom event and hands it in as a parameter?

dwt commented Jul 16, 2015

Out of interest, wouldn't it be viable to switch the whole event propagation mechanism to use the browsers native facilities to make this kind of interoperability just work? I.e. all events are registered via .addEventListener() and all .trigger() does is dispatch a real event? Sure this wouldn't work for old browsers, but thats what the compatibility branch is for. No?

As for jQuery specific functionality, that should be possible quite easily by wrapping the handlers registered via jQuery in a closure that extracts the extra information from the triggered custom event and hands it in as a parameter?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 16, 2015

Member

It's pretty easy to test a theory like that. We have pretty thorough unit tests for events. Write whatever changes you think would work and see how well they do on the unit tests.

Member

dmethvin commented Jul 16, 2015

It's pretty easy to test a theory like that. We have pretty thorough unit tests for events. Write whatever changes you think would work and see how well they do on the unit tests.

@dwt

This comment has been minimized.

Show comment
Hide comment
@dwt

dwt Jul 16, 2015

@dmethvin Well, does that mean you really don't know and are interested to do that change? Or do you know it doesn't work?

dwt commented Jul 16, 2015

@dmethvin Well, does that mean you really don't know and are interested to do that change? Or do you know it doesn't work?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 16, 2015

Member

@dwt I can't know that it doesn't work, I haven't tried to write the code. There are a lot of potential issues but perhaps you can work them out. At least if you give it a try it will provide a double-check on my suspicion that it is not possible without significant breakage of the current APIs.

The lack of a DOM getEventListeners is likely to cause trouble for example, you will still need to maintain most of the bookkeeping we have today plus more. If a user attaches an event listener outside jQuery your plan can trigger it, but I don't think there is a non-triggering way to know it's there or clone it to another element.

Edit: So in short, an ideal patch for this would not be significantly bigger than the current code and not introduce any breakage to the current API contracts.

Member

dmethvin commented Jul 16, 2015

@dwt I can't know that it doesn't work, I haven't tried to write the code. There are a lot of potential issues but perhaps you can work them out. At least if you give it a try it will provide a double-check on my suspicion that it is not possible without significant breakage of the current APIs.

The lack of a DOM getEventListeners is likely to cause trouble for example, you will still need to maintain most of the bookkeeping we have today plus more. If a user attaches an event listener outside jQuery your plan can trigger it, but I don't think there is a non-triggering way to know it's there or clone it to another element.

Edit: So in short, an ideal patch for this would not be significantly bigger than the current code and not introduce any breakage to the current API contracts.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Sep 9, 2015

Member

I'm closing this, since I suspect it isn't possible to do without breaking existing code or causing performance issues. If someone can post a solution proving me wrong we can reopen.

Member

dmethvin commented Sep 9, 2015

I'm closing this, since I suspect it isn't possible to do without breaking existing code or causing performance issues. If someone can post a solution proving me wrong we can reopen.

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