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

events: setting Event.cancelBubble calls stopPropagation #50401

Closed
KhafraDev opened this issue Oct 26, 2023 · 15 comments
Closed

events: setting Event.cancelBubble calls stopPropagation #50401

KhafraDev opened this issue Oct 26, 2023 · 15 comments
Labels
eventtarget Issues and PRs related to the EventTarget implementation. good first issue Issues that are suitable for first-time contributors.

Comments

@KhafraDev
Copy link
Member

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 = true

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

set cancelBubble(value) {
if (!isEvent(this))
throw new ERR_INVALID_THIS('Event');
if (value) {
this.stopPropagation();
}
}

@KhafraDev KhafraDev added good first issue Issues that are suitable for first-time contributors. eventtarget Issues and PRs related to the EventTarget implementation. labels Oct 26, 2023
@KhafraDev
Copy link
Member Author

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
Copy link
Contributor

Anymore such good first issues? :)

@KhafraDev
Copy link
Member Author

@BenzeneAlcohol #50417

@DylanTet
Copy link
Contributor

Looking for some other good first issues as well!

@KhafraDev
Copy link
Member Author

@DylanTet #50432

@DylanTet
Copy link
Contributor

@KhafraDev thank you!

@MattSilverio
Copy link

MattSilverio commented 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
Copy link

Here is the interface produced by your code
image

And this one is produced without changing the stopPropagation function behaviour:
image

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

image

@Scout2012
Copy link

@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
Copy link
Member Author

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
Copy link

Scout2012 commented 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 🙂.

Scout2012 added a commit to Scout2012/node that referenced this issue Dec 6, 2023
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
@KhafraDev
Copy link
Member Author

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
Copy link

Scout2012 commented 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
Copy link

hello #50401 can we close this issue because it has been merged

@mertcanaltin
Copy link
Member

mertcanaltin commented Feb 28, 2024

as @sedaklnc said #50405 pr was also merge as said I am closing everyone's hands health ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eventtarget Issues and PRs related to the EventTarget implementation. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

7 participants