Skip to content
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 stop propagation of `jQuery.click()` on native event-handlers #3693

Closed
caillou opened this issue Jun 12, 2017 · 10 comments
Closed
Labels

Comments

@caillou
Copy link
Contributor

@caillou caillou commented Jun 12, 2017

Description

Event-handlers that are registered natively are called even if event.stopPropagation() is called when jQuery.click() is used.

Using the native HTMLelement.click() correctly stops propagation.

Link to test case

I the following test case, both, a HTMLelement.click() and a jQuery.click() are triggered when the page is loaded.

In both cases, the event bubbling is prevented. The jQuery.click() is still triggered on the document event-handler.

https://jsbin.com/qekuda/7/edit?html,js,output

Link to Stackoverflow discussion

https://stackoverflow.com/questions/44309897/jquery-does-not-stop-propagation-of-jquery-click-on-native-event-handlers

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jun 12, 2017

jQuery has never been able to guarantee full interoperability of native-vs-jQuery event handlers. It is best to think of jQuery's event subsystem as a layer on top of native events.

What's happening here is that you are triggering a "fake" jQuery click on the button. There is no associated native MouseEvent that we can use, so we track with a jQuery Event whether propagation is stopped or the default prevented. Although .stopPropagation() was stopped, .preventDefault() was not so at the end of our (truncated) bubbling we call the native DOM .click() event which calls the native handler.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jun 12, 2017

If we switched to using native events for triggering we could handle this scenario, but at the expense of several serious breaking changes in jQuery's event subsystem. Also, there is no simple lookup to know which events are magically understood and specially processed by the DOM ("click") versus custom ("myCustomEvent") and which constructor to use (MouseEvent, TouchEvent, etc.). Since we would be breaking the API anyway, the easiest thing would be to make the caller construct the event so we wouldn't need a lookup at all.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Jun 12, 2017

To agree with @dmethvin, this is a bit surprising but working as designed. See the .trigger documentation:

For both plain objects and DOM objects other than window, if a triggered event name matches the name of a property on the object, jQuery will attempt to invoke the property as a method if no event handler calls event.preventDefault(). If this behavior is not desired, use .triggerHandler() instead.

So .stopPropagation() correctly prevents jQuery from running handlers on ancestors, but by itself does not prevent the browser from creating and bubbling an event when we invoke elem.click() to execute the default action (just like we would invoke elem.submit() to submit a form even if propagation were stopped). Since it seems like you don't want the default action, you can use .triggerHandler("click") instead of .trigger("click") or .click(), or invoke .preventDefault() in addition to .stopPropagation(), or return false from your listener (which calls both on your behalf).

@gibson042 gibson042 closed this Jun 12, 2017
@gibson042 gibson042 added Event and removed Needs review labels Jun 12, 2017
@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jun 12, 2017

Yes, as @gibson042 says if you don't need to stop propagation beyond the event target and just want to run jQuery handlers without preventing the default action, you can use .triggerHandler(). A lot of tracking systems (Google Analytics, Facebook, etc) seem to attach events on the document.

@caillou
Copy link
Contributor Author

@caillou caillou commented Jun 13, 2017

@dmethvin @gibson042 My naive assumption tells me, that something could be done here.

If we look at the following lines from /src/event/trigger.js:

// Prevent re-triggering of the same event, since we already bubbled it above
jQuery.event.triggered = type;
elem[ type ]();
jQuery.event.triggered = undefined;

Couldn't we do something like that:

// Prevent re-triggering of the same event, since we already bubbled it above
jQuery.event.triggered = type;

if ( event.isPropagationStopped() ) {
  // Add native event handler that stopsPropagation where it should.
}

elem[ type ]();

if ( event.isPropagationStopped() ) {
  // Remove the event listener we just added.
}

jQuery.event.triggered = undefined;

If you think that makes sense, I could try to work on it. If yes, is there more to running the tests than grunt test? I can not find any tests relating to the .trigger() function.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jun 13, 2017

@caillou something like that might work. How do you determine "stopsPropagation where it should"? We would need to remember the element where it stopped in jQuery.event.trigger()?

@caillou
Copy link
Contributor Author

@caillou caillou commented Jun 13, 2017

@dmethvin You have a while () loot that ends when event.isPropagationStopped() on line 95 of /src/event/trigger.js:

while ( ( cur = eventPath[ i++ ] ) && !event.isPropagationStopped() ) {
}

All we need to do, is to keep track of the last node that has been executed upon. Something like that:

var lastNode;
while ( ( cur = eventPath[ i++ ] ) && !event.isPropagationStopped() ) {
  lastNode = cur;
}

Or we can just use the i counter to access the last node that has been iterated on eventPath[i-1].

@caillou
Copy link
Contributor Author

@caillou caillou commented Jun 13, 2017

@dmethvin One problem I have, is that I don't seem to find any tests relating to this code. Are there additional test cases outside of this repository, that test this code?

Update: I figured out how to run the integration tests in addition to the unit tests.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Jun 13, 2017

The tests are in https://github.com/jquery/jquery/blob/master/test/unit/event.js, you can try them locally and see if they work.

caillou added a commit to caillou/jquery that referenced this issue Jun 13, 2017
This test reproduces the following bug reported in jquery#3693:

Calling `.click()` triggers event-handler that was natively
registered event if `e.stopPropagation()` is called on the parent.
caillou added a commit to caillou/jquery that referenced this issue Jun 13, 2017
@caillou
Copy link
Contributor Author

@caillou caillou commented Jun 13, 2017

@dmethvin @gibson042 I created a branch with a test to reproduce the bug and a proposal for a bug-fix: https://github.com/caillou/jquery/commits/stop-propagation-on-native-events

How would you proceed from here? Should I just create a PR?

@caillou caillou mentioned this issue Jun 13, 2017
4 of 4 tasks complete
timmywil added a commit that referenced this issue Jul 10, 2017
Fixes gh-3693
Close gh-3694
@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.