Skip to content

Conversation

@sjorsdonkers
Copy link
Contributor

@sjorsdonkers sjorsdonkers commented Jul 8, 2025

Depends on: lightpanda-io/libdom#30
Support new EventTarget()

This is likely for custom elements.

@karlseguin
Copy link
Collaborator

I don't understand the toInterface issue?

@sjorsdonkers
Copy link
Contributor Author

@karlseguin

I don't understand the toInterface issue?

If toInterface is called with an event and EventTarget (created using the constructor), we are not able to detect that these are created as such.
Therefore the toInterface function is just going to cast the EventTarget to a Node which would be wrong.
Other eventtargets like xhr or abort_signal use the eventGetInternalType. This works because we can set this internal type ourselves for those events.

In this case however the user Js code is creating both the Event and the EventTarget directly, so the eventInternalType is not set. I don't know if toInterface is ever called with those user created events, but if it is it will likely crash.

Perhaps the constructor of Event should set the internalType, or is that constructor also used for Nodes?
I'll check what that does.

@karlseguin
Copy link
Collaborator

Yes, it would crash, and yes, it could be called (e.g. if they called this.currentTarget within the callback).

In the above snippet, where / how is new EventTarget being called (presumably in dispatchEvent ?).

With AbortSignal and XMLHttpRequestEventTarget, I was able to avoid adding a type to libdom's event target, because they are always created together, so I could use the event's type. But, for this case, it does seem like the discriminator needs to be on the event_target itself.

@sjorsdonkers sjorsdonkers requested a review from karlseguin July 10, 2025 14:03
if (try parser.eventTargetInternalType(et) == .plain) {
return .{ .plain = et };
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand keeping the PR focused is good. But, if the goal is to migrate AbortSignal and XMLHttpRequestEventTarget to this, then it might be worth including that change now. It's a good way to make sure the new field/behavior is capable of handling all know (and thus hopefully future) cases. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, Just wanted confirmation of the chosen solution first before doing the others.
Added the others and cleanup here: 14eeffd

@karlseguin
Copy link
Collaborator

LGTM

@sjorsdonkers sjorsdonkers merged commit 5123697 into main Jul 11, 2025
10 checks passed
@sjorsdonkers sjorsdonkers deleted the EventTarget-constructor branch July 11, 2025 07:55
@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants