Skip to content

events: setting event cancelBubble calls stopPropagation#50405

Closed
mertcanaltin wants to merge 3 commits intonodejs:mainfrom
mertcanaltin:dev-50401
Closed

events: setting event cancelBubble calls stopPropagation#50405
mertcanaltin wants to merge 3 commits intonodejs:mainfrom
mertcanaltin:dev-50401

Conversation

@mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Oct 26, 2023

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
deokjinkim
deokjinkim reviewed Oct 26, 2023
View reviewed changes
@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
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

nodejs-github-bot commented Dec 9, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2023

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

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Dec 30, 2023

I wonder if I should do an update here?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 31, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 1, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 2, 2024

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

mertcanaltin commented Jan 6, 2024

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

nodejs-github-bot commented Jan 6, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 9, 2024

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

nodejs-github-bot commented Jan 17, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 21, 2024

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

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 21, 2024

passed 🎉🚀

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 25, 2024

@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

mhdawson commented Feb 26, 2024

@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

mhdawson commented Feb 26, 2024

Landed in 399654f

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

mertcanaltin commented Feb 27, 2024

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

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@deokjinkim deokjinkim deokjinkim approved these changes

@mhdawson mhdawson mhdawson approved these changes

@KhafraDev KhafraDev KhafraDev approved these changes

+1 more reviewer

@jeanbern jeanbern jeanbern left review comments

Reviewers whose approvals may not affect merge requirements

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