Repository navigation
events: setting Event.cancelBubble calls stopPropagation #50401
Description
Version
v22
Platform
Linux DESKTOP-L4O1H93 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
events
What steps will reproduce the bug?
const ev = new Event('')
ev.stopPropagation = () => {
console.log('???')
}
ev.cancelBubble = trueHow often does it reproduce? Is there a required condition?
n/a
What is the expected behavior? Why is that the expected behavior?
nothing to be printed
What do you see instead?
???
Additional information
node/lib/internal/event_target.js
Lines 304 to 310 in 9c714d8
Activity
KhafraDev commentedon Oct 26, 2023
I have a couple of these but I don't know if they come off as spammy. I'm hoping a new contributor might find them easy to fix.
BenzeneAlcohol commentedon Oct 26, 2023
Anymore such good first issues? :)
KhafraDev commentedon Oct 26, 2023
@BenzeneAlcohol #50417
DylanTet commentedon Oct 27, 2023
Looking for some other good first issues as well!
KhafraDev commentedon Oct 27, 2023
@DylanTet #50432
DylanTet commentedon Oct 27, 2023
@KhafraDev thank you!
MattSilverio commentedon Nov 5, 2023
@KhafraDev , I'm newbie here and if I understood your issue, you are currently changing the behaviour of Event.stopPropagation to just a simple console.log(???)
MattSilverio commentedon Nov 5, 2023
Here is the interface produced by your code
And this one is produced without changing the stopPropagation function behaviour:
Calling both instances to execute StopPropagation, it looks that you call your function and the stopPropagation, since when I call stopPropagation in my instance it gives undefined since it's the same second output provided from your code
Scout2012 commentedon Dec 6, 2023
@KhafraDev could you please elaborate a bit on what makes this a bug exactly?
I see that the expectation was defined as "nothing to be printed", but that seems like it's just a byproduct of overwriting the stopPropagation method and should be allowed if the user sees fit.
If it's for the fact that cancelBubble calls stopPropagation, the spec that seems to have been used to guide the PR that introduced this behavior states that stopPropagation and cancelBubble have essentially the same responsibility of setting the stop propagation flag, cancelBubble will just only set the flag if the value provided to cancelBubble was true.
The spec doesn't say that they have to call each other, but it makes sense to me at least to only have one definition for what "setting the stop propagation flag" looks like for a given event, and have any other of the event's methods that needs to perform the same task of setting it rely on doing it in the same defined manner. If that user defined the process of setting the flag as including the execution of some logging statements, then that seems reasonable to allow.
KhafraDev commentedon Dec 6, 2023
The behavior is implied, WHATWG specs typically do it. If we look at addEventListener (as an example), the spec doesn't mention that the first argument must be coerced to a string, but the implied behavior is that it's converted to a DOMString because of its idl definition. Similarly, it's implied that setting cancelBubble won't have any side effects because it's not mentioned in the spec.
If we go deeper into spec, cancelBubble's steps are to "set this’s stop propagation flag if the given value is true; otherwise do nothing". The spec explicitly tells implementations to set the flag directly, rather than calling stopPropagation.
Scout2012 commentedon Dec 6, 2023
Good point, thank you for the elaboration!
So to make sure I understand, the bug here is the unspec'd/implied relationship between stopPropagation and cancelBubble, and the resolve would just be to specifically only set the stop propagation flag (when value is true) in cancelBubble's setter? If so, it definitely does sound like a good first issue indeed!
Thanks for logging it, I'll work on a PR 🙂.
events: unlink stopPropagation and cancelBubble methods
KhafraDev commentedon Dec 6, 2023
If you have any questions, I can answer them (they might be answered in the contributing guide already). Make sure to add in a test too! The change looks good to me.
Scout2012 commentedon Dec 6, 2023
I somehow missed that there was already a PR open that addresses this issue🤦. Still has been a good exercise. I'll keep my eye out for other issues!
sedaklnc commentedon Feb 28, 2024
hello #50401 can we close this issue because it has been merged
mertcanaltin commentedon Feb 28, 2024
as @sedaklnc said #50405 pr was also merge as said I am closing everyone's hands health ❤️