GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — events: add stop propagation flag to Event.stopImmediatePropagation by mikemadest · Pull Request #39463 · nodejs/node

Conversation

@mikemadest

@mikemadest mikemadest commented Jul 20, 2021

Copy link
Copy Markdown
Contributor

Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".

So the second is not supported by Node as there is no hierarchy and bubbling,
but the flags are both present as well as stopPropagation.

Would it make sense to follow specs on that?
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
WebReflection reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jul 20, 2021
@mikemadest
mikemadest force-pushed the stop-immediate-propagation-flags branch from 33c4ebc to 23744d2 Compare July 20, 2021 08:15
@mikemadest mikemadest changed the title Events: add stop propagation flag to Event.stopImmediatePropagation events: add stop propagation flag to Event.stopImmediatePropagation Jul 20, 2021
@mikemadest

mikemadest commented Jul 20, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I didn't want to make a description too long, but feel like a bit more context could be interesting:
As part of contributing to linkedom (https://github.com/WebReflection/linkedom), I was adding event bubbling there.

When Event and EventTarget are available in Node js, the choice was to extend them to provide what was needed (bubbling in dispatchEvent).
Since node don't need the bubbling part it was expected
(I had a good read at #33556 (comment)).

But since Event had everything needed I didn't think it would be required there, except for that tiny missing line in stopImmediatePropagation and it felt like maybe this should be here?
Hoping I don't waste anybody's time here, I understand it's not game changer.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jul 20, 2021
View reviewed changes
@mikemadest mikemadest mentioned this pull request Jul 20, 2021
Extend EventTarget to support event bubbling WebReflection/linkedom#84
Merged
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@mikemadest

mikemadest commented Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

Should this be closed?
This was approved but now is quite old and I'm not sure if there is any interest in merging it. I solved the conflicts just in case...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Jun 26, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

No this fix looks correct it was just missed - apologies this is our bad.
mikemadest reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Jun 26, 2023
View reviewed changes
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 26, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 26, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52488/
mikemadest reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikemadest

mikemadest commented Jun 26, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

No this fix looks correct it was just missed - apologies this is our bad.

No worries, thank you for the answer!
There seems to be errors on the tests, I'll check that and will be active on this so if you don't see any changes, I'm probably working on them or trying to understand what's wrong.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikemadest
mikemadest force-pushed the stop-immediate-propagation-flags branch from 527ff4d to 53f073d Compare June 27, 2023 11:00
@mikemadest

mikemadest commented Jun 27, 2023

Copy link
Copy Markdown
Contributor Author

Pull request updated:
solved conflicts, rebased, tested (everything passed) and pushed again. Everything should be fine now.
benjamingr reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Jun 27, 2023

Copy link
Copy Markdown
Member

Thank you for your patience we truly don't deserve it and apologies for the contribution experience.

Let's run CI and land this as soon as it's green.
MoLow and mikemadest reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Jun 27, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/52515/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 28, 2023
CI Reliability 2023-06-28 nodejs/reliability#601
Open
CI Reliability 2023-06-29 nodejs/reliability#602
Open
CI Reliability 2023-06-30 nodejs/reliability#603
Open
CI Reliability 2023-07-01 nodejs/reliability#604
Open
CI Reliability 2023-07-02 nodejs/reliability#605
Open
@mikemadest
mikemadest force-pushed the stop-immediate-propagation-flags branch from 53f073d to d7de14d Compare July 2, 2023 07:55
This was referenced Jul 3, 2023
CI Reliability 2023-07-03 nodejs/reliability#606
Open
CI Reliability 2023-07-04 nodejs/reliability#607
Open
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 4, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 4, 2023
@mikemadest
mikemadest force-pushed the stop-immediate-propagation-flags branch from c4739fa to d2b2e2c Compare August 8, 2023 12:58
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 9, 2023
benjamingr
benjamingr approved these changes Aug 9, 2023
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 10, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 10, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53118/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikemadest
mikemadest force-pushed the stop-immediate-propagation-flags branch from d2b2e2c to d712a39 Compare August 14, 2023 07:53
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 14, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53301/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Aug 14, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/39463
✔  Done loading data for nodejs/node/pull/39463
----------------------------------- PR info ------------------------------------
Title      events: add stop propagation flag to Event.stopImmediatePropagation (#39463)
Author     Mickael Meausoone  (@mikemadest, first-time contributor)
Branch     mikemadest:stop-immediate-propagation-flags -> nodejs:main
Labels     needs-ci
Commits    1
 - events: add stop propagation flag to Event.stopImmediatePropagation
Committers 1
 - Mickael Meausoone 
PR-URL: https://github.com/nodejs/node/pull/39463
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/39463
Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
Reviewed-By: James M Snell 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - events: add stop propagation flag to Event.stopImmediatePropagation
   ℹ  This PR was created on Tue, 20 Jul 2021 08:02:57 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/39463#pullrequestreview-710658397
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/39463#pullrequestreview-1569362103
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-08-14T09:35:27Z: https://ci.nodejs.org/job/node-test-pull-request/53301/
- Querying data for job/node-test-pull-request/53301/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/5859369572
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mikemadest @aduh95
events: add stop propagation flag to Event.stopImmediatePropagation
991ae2e 
Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".
So the second is not supported by Node as there is no
hierarchy and bubbling,
but the flags are both present as well as stopPropagation.
It would makes sense to follow specs on that.

Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
@aduh95
aduh95 force-pushed the stop-immediate-propagation-flags branch from d712a39 to 991ae2e Compare May 11, 2024 17:54
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59127/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59135/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 04cf8c2 into nodejs:main May 12, 2024
@aduh95

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 04cf8c2
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 12, 2024
@mikemadest @targos
events: add stop propagation flag to Event.stopImmediatePropagation
96566fc 
Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".
So the second is not supported by Node.js as there is no
hierarchy and bubbling,
but the flags are both present as well as stopPropagation.
It would makes sense to follow specs on that.

Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
PR-URL: #39463
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@mikemadest @marco-ippolito
events: add stop propagation flag to Event.stopImmediatePropagation
74753ed 
Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".
So the second is not supported by Node.js as there is no
hierarchy and bubbling,
but the flags are both present as well as stopPropagation.
It would makes sense to follow specs on that.

Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
PR-URL: #39463
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@mikemadest @soophoo
events: add stop propagation flag to Event.stopImmediatePropagation
39eb8e3 
Spec mention stopImmediatePropagation should set both flags:
"stop propagation" and "stop immediate propagation".
So the second is not supported by Node.js as there is no
hierarchy and bubbling,
but the flags are both present as well as stopPropagation.
It would makes sense to follow specs on that.

Refs: https://dom.spec.whatwg.org/#dom-event-stopimmediatepropagation
PR-URL: nodejs#39463
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mikemadest @benjamingr @nodejs-github-bot @aduh95 @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.