-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
events: setting Event.cancelBubble calls stopPropagation #50401
Comments
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. |
Anymore such good first issues? :) |
Looking for some other good first issues as well! |
@KhafraDev thank you! |
@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(???) |
@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. |
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. |
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 🙂. |
WHATWG spec does not define any explicit relationship between stopPropagation and cancelBubble, the methods' responsibilities are simply to "set this’s stop propagation flag" under varying circumstances. Fixes: nodejs#50401
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. |
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! |
hello #50401 can we close this issue because it has been merged |
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?
How 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
The text was updated successfully, but these errors were encountered: