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.

Sign up for GitHub

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

Jump to bottom

events: setting event cancelBubble calls stopPropagation #50405

Closed
wants to merge 3 commits into from
Closed

events: setting event cancelBubble calls stopPropagation #50405

wants to merge 3 commits into from

Conversation

mertcanaltin
Copy link
Member

issue: #50401

In this issue, we addressed the problem where setting cancelBubble to true unintentionally called the stopPropagation method. The solution ensures that stopPropagation is only called when cancelBubble is explicitly set to false, as it should be. This change prevents the method from being called when cancelBubble is true or not explicitly set

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 26, 2023
@mertcanaltin mertcanaltin added events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. labels Oct 26, 2023
@mertcanaltin @deokjinkim
Update lib/internal/event_target.js
b1fc248 
Co-authored-by: Deokjin Kim <deokjin81.kim@gmail.com>
@anonrig
Copy link
Member

anonrig commented Oct 26, 2023

Can you add a test?

benjamingr
benjamingr requested changes Oct 26, 2023
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test, changes lgtm

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Nov 12, 2023 •
edited
Loading

ı added a test

jeanbern
jeanbern reviewed Nov 16, 2023
View reviewed changes
lib/internal/event_target.js
@@ -305,7 +305,7 @@ class Event {
if (!isEvent(this))
throw new ERR_INVALID_THIS('Event');
if (value) {
this.stopPropagation();
this.#propagationStopped = true;
Copy link

@jeanbern jeanbern Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is setting cancelBubble supposed to be one-way? The proposed change doesn't modify this.#propagationStopped when value is false

nvm: For anyone else wondering the same thing see here: whatwg/dom#211

@Scout2012 Scout2012 mentioned this pull request Dec 6, 2023
Closed
@deokjinkim deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56173/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56522/

@mertcanaltin
Copy link
Member Author

I wonder if I should do an update here?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56603/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56621/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56624/

@mertcanaltin mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2024
@mertcanaltin mertcanaltin removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2024
@mertcanaltin
Copy link
Member Author

I guess flakky was the test 🤔 💭

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56696/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56723/

@mertcanaltin mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56821/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56862/

@mertcanaltin
Copy link
Member Author

passed 🎉🚀

@mertcanaltin
Copy link
Member Author

@benjamingr I would be very happy if you can make an update here, now it seems to have passed all the tests

@mertcanaltin mertcanaltin added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2024
mhdawson
mhdawson approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

@mertcanaltin I'm getting this warnings when landing -
GitHub cannot link the author of 'events: setting event cancelBubble calls stopPropagation' to their GitHub account.
⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork

Can you take a look? I'll land anyway but would be good to get that fixed up.

mhdawson pushed a commit that referenced this pull request Feb 26, 2024
@mhdawson
events: no stopPropagation call in cancelBubble
399654f 
PR-URL: #50405
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member

Landed in 399654f

@mhdawson mhdawson closed this Feb 26, 2024
@mertcanaltin
Copy link
Member Author

I am very sorry for the late response, thank you very much for correcting me @mhdawson ❤️ 🚀

marco-ippolito pushed a commit that referenced this pull request Feb 27, 2024
@marco-ippolito
events: no stopPropagation call in cancelBubble
78f8180 
PR-URL: #50405
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@richardlau
events: no stopPropagation call in cancelBubble
f8e715f 
PR-URL: #50405
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@richardlau
events: no stopPropagation call in cancelBubble
d9b5cd5 
PR-URL: #50405
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
@rdw-msft
events: no stopPropagation call in cancelBubble
af18ba1 
PR-URL: nodejs#50405
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanbern jeanbern jeanbern left review comments

@KhafraDev KhafraDev KhafraDev approved these changes

@mhdawson mhdawson mhdawson approved these changes

@deokjinkim deokjinkim deokjinkim approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mertcanaltin @anonrig @nodejs-github-bot @mhdawson @jasnell @jeanbern @benjamingr @deokjinkim @KhafraDev