Skip to content

Extend EventTarget to support event bubbling #84

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 29 commits into from
Jul 21, 2021

Conversation

mikemadest
Copy link
Contributor

This solves: ungap/event-target#8
As discussed there, EventTarget is extended to add bubbling and parent node knowledge only in the DOM context.

- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
@WebReflection
Copy link
Owner

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

@mikemadest
Copy link
Contributor Author

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

Thanks!
I can't really test the full use case without updating "ungap/event-target" (I could do a PR for that later if that's working for you), but I'll add more tests to cover stopImmediatePropagation here.

@WebReflection
Copy link
Owner

Couple of changes needed, and I'd love to have stopImmediatePropagation() tested too, otherwise, awesome job!

Thanks!
I can't really test the full use case without updating "ungap/event-target" (I could do a PR for that later if that's working for you), but I'll add more tests to cover stopImmediatePropagation here.

Oh ... I see. Sorry, busy days in here ... I'll try to update/change that later on but if you are, somehow, in a rush, please go ahead with the PR (ungap is a bit weird to work with though, no modern JS and stuff) ... as you prefer.

- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
@mikemadest
Copy link
Contributor Author

Oh ... I see. Sorry, busy days in here ... I'll try to update/change that later on but if you are, somehow, in a rush, please go ahead with the PR (ungap is a bit weird to work with though, no modern JS and stuff) ... as you prefer.

No worries, I didn't mean to rush you :)
I can't say I am in a particular hurry, but I can understand you would be busy and I don't mind at all doing a PR there (especially since I already pulled the code and played with it... so it's mostly ready).
To be honest the only thing I was confused about were the unit tests (in ungap) and for my local tests I used your assert there (but I'll refactor that to follow existing code).

used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@WebReflection
Copy link
Owner

So ... what are we missing here? is that still ungap for the event target? Thanks!

@mikemadest
Copy link
Contributor Author

So ... what are we missing here? is that still ungap for the event target? Thanks!

yes, I can make a PR for that next, it should be way simpler.

Mickael Meausoone added 5 commits July 20, 2021 20:59
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@WebReflection
Copy link
Owner

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

@mikemadest
Copy link
Contributor Author

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

Sorry I'm not sure what happened there, will cleanup

Mickael Meausoone and others added 15 commits July 21, 2021 11:20
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
* Adding NamedNodeMap to global export
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
- add getParent method to Node returning parentNode
- on dispatch trigger parent dispatchEvent
- unit tests
- method renamed to "_getParent"
- dispatchEvent return value now take EventTarget.dispatchEvent return into account as well as parent.dispatchEvent
- "stopImmediatePropagation" now tested in unit tests.

Todo: when ungap/event-target support "_stopImmediatePropagationFlag" more event listeners could be added to `buttonTarget` to ensure full behaviour is working.
used "protected" here instead of "private", because Node extend EventTarget and needs to overwrite "_getParent" while still allowing "dispatchEvent" to access it.
- Node 16.5 will throw if we try to dispatch the current event to the parent. So instead we need to create a new event with the same options.

- replaced "_stopPropagationFlag" by "cancelBubble" to be consistent with Node implementation of Event

- currently Node don't handle bubbling, but also don't properly set "_stopPropagationFlag" when calling "stopImmediatePropagation" creating a problem and inconsistency for us.
=> should we extend the Node Event object to ensure the proper behaviour?
@mikemadest
Copy link
Contributor Author

@mikemadest this looks good to me, but I don't understand why there are unrelated changes in this MR ... by any chance you can remove those and pull rebase from main instead?

git pull --rebase origin main

I'd rather keep changes per PR/MR, thanks. Once you do that, I'll squash merge and publish.

ok so I was just behind main because of other merge, this looks all good now

@WebReflection WebReflection merged commit 12c7235 into WebReflection:main Jul 21, 2021
@WebReflection
Copy link
Owner

v0.11.1 is up and running 🍻

Thanks a lot for your help ❤️

@mikemadest
Copy link
Contributor Author

v0.11.1 is up and running 🍻

Thanks a lot for your help ❤️

thank you for bearing with me :)

@mikemadest mikemadest deleted the event-bubbling branch July 22, 2021 17:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants