Stop propagation on native events #3694
Conversation
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.
This commit fixes jquery#3693.
|
|
||
| var rfocusMorph = /^(?:focusinfocus|focusoutblur)$/; | ||
| var rfocusMorph = /^(?:focusinfocus|focusoutblur)$/, | ||
| stopPropagationCallback = function( e ) { |
There was a problem hiding this comment.
I wonder if we should have this inside trigger() since it is only used there. Opinions welcome.
There was a problem hiding this comment.
The difference is the following:
- If we define the function within the
triggermethod, it is defined every time trigger is called. - If we define the function within the AMD callback function as I did here, the function will be defined only once, but even if the
triggermethod is never used. Additionally, there is an additional scope lookup.
All in all, these considerations are micro optimisations, that are not really worth considering. The questions is more one of culture within a project. We could also define it at the end of the file, just before the return jQuery statement.
I will look at the other files and see how it is usually handled within the jQuery project.
| jQuery.event.triggered = type; | ||
|
|
||
| if ( event.isPropagationStopped() ) { | ||
| lastElement.addEventListener( type, stopPropagationCallback ); |
There was a problem hiding this comment.
I think it would be possible to get here with lastElement === undefined if the incoming event already had propagation stopped? Maybe initialize with lastElement = elem?
There was a problem hiding this comment.
Hmmm, this does bring up the problem of .stopImmediatePropagation() which isn't fixed. Since that method is sensitive to the order that handlers are attached and ours is always going to be last, there isn't much we can do for that 🤷♂️ .
There was a problem hiding this comment.
@dmethvin One option with the .stopImmediatePropagation() would be to register an additional event handler on the second to last element (the great ancestor) if there is one present, that stops the propagation.
But this seems rather fiddly.
There was a problem hiding this comment.
@dmethvin I will try to add a test-case for the lastElement === undefined case. But I don't yes see how this could happen. How can the propagation of the incoming even already be stopped?
test/unit/event.js
Outdated
| $anchor2[ 0 ].removeEventListener( "click", neverCallMe ); | ||
| } ); | ||
|
|
||
| QUnit.test( "triggerd events stopPropagation() for natively-bound events", function( assert ) { |
gibson042
left a comment
There was a problem hiding this comment.
I had my doubts, but this looks good. 👍
| i = 0; | ||
| while ( ( cur = eventPath[ i++ ] ) && !event.isPropagationStopped() ) { | ||
|
|
||
| lastElement = cur; |
There was a problem hiding this comment.
You can reduce the size increase of jquery.min.gz by using eventPath[ i - 2 ] below rather than introducing a new variable.
There was a problem hiding this comment.
@gibson042 in my opinion, it would reduce the size at a high cost of code readability. What is the code guideline in the jQuery project with regard to readability v.s. size?
It was my initial thought too. But I felt that the eventPath.pop() on line 126 makes it fairly difficult to follow the code.
|
@dmethvin @gibson042: I have added a test with a perviously stopped event and fixed it. Could you review this again? Sorry for the trouble, I should have thought about this. This yields the question: Do we really want to go forward with this PR? While it helps with interoperability between native and jQuery events, I wonder if it is worth the trouble. WDYT? |
|
@caillou I think you have a pretty small patch here and it could be useful. What are your concerns? |
|
@dmethvin Good. No real concerns. What is the workflow? Do I need to do anything, or will it get merged at some point? |
|
@caillou Yes, we'll merge it shortly. Thanks! |
Summary
This PR ensures that
event.stopPropagation()is respected for event handlers that were registered using the nativeelement.addEventlistener()method when the event was triggered by jQuery's.trigger('click')method.This PR fixes #3693.
The ticket was closed at first with the following argument:
Nonetheless, the discussion lead me to believe, that this incompatibility between native and jQuery event handling could be fixed and that such a fix is worthwhile.
Checklist