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

Propagate document event to window #88

Closed
wants to merge 1 commit into from

Conversation

Kal-Aster
Copy link
Contributor

Hi!

A code of mine broke, beacause it depends on an event propagated to the window from the document.
This PR fixes this behavior.

I also noticed that the capture and bubbling phase are not handled in dispatchEvent, I think that would be helpful to implement that logic, having some code depends on the order of the events.
Since I already done something like that, I could add it to this PR or maybe another one, to make things cleaner.

@WebReflection
Copy link
Owner

Capture is not interesting here , and it would complicate a lot the events implantation, not sure it’ll ever land, as there are zero use cases on the server . Bubbling was already a stretch, and I don’t want LinkeDOM to became a test oriented library only, there’s JSDOM already for that.

composed: event.composed,
};
// in Node 16.5 the same event can't be used for another dispatch
return view.dispatchEvent(new event.constructor(event.type, options));
Copy link
Contributor

@mikemadest mikemadest Jul 25, 2021

Choose a reason for hiding this comment

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

A note about bubbling as I'm still working on it:
After working on #84 I noticed there was still issues there.

For a page having a button, a div#parent and a div#container, dispatching a click Event on the button should keep "button" as target during all bubbling while "currentTarget" would be the current element the bubbling is on.

I see the current PR is also missing that, as the original target is lost.

Proper bubbling with correct target could be difficult to achieve while keeping the ungap/event-target or Node EventTarget extends, if I have a reasonable proposition I'll do a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that instead of adding dispatchEvent there, all you need could be to define _getParent for document that would return this.defaultView.
This should work as document also extends EventTarget.
dispatchEvent is already defined there and use that _getParent method.

Copy link
Owner

Choose a reason for hiding this comment

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

currentTarget is always where the listener was attached, never nodes in between. target is where the initial dispatch happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kal-Aster I opened a PR for solving the target issue but also with an update for the window propagation based on what I suggested: #89
This should be more in line with existing code than adding the dispatchEvent logic to document, maybe you can check it to see if that works for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, thank you for taking care of it, I couldn't find time for it these weeks but I think I couldn't make a better implementation!
It seems working for me, haven't test it but as long as the event propagates to the window, like it does, it works.
If @WebReflection agrees with it, we can also close this PR

@WebReflection
Copy link
Owner

Should this land before, after, or it replaces, #89 ???

@mikemadest
Copy link
Contributor

Should this land before, after, or it replaces, #89 ???

It should replace it:

If @WebReflection agrees with it, we can also close this PR

here: #88 (comment)

@WebReflection
Copy link
Owner

@mikemadest apologies for my confusion ... do we still want this in and, if that's the case, do you mind rebasing or be sure everything is fine in here? Thank you!

@mikemadest
Copy link
Contributor

@mikemadest apologies for my confusion ... do we still want this in and, if that's the case, do you mind rebasing or be sure everything is fine in here? Thank you!

I think this PR can be closed now as this issue should be fixed.
cc: @Kal-Aster

@WebReflection
Copy link
Owner

Done, please let me know if this should be re-opened, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants