`event.currentTarget` wrongly changes reference to the <body> after an `await`, inside an onclick event

madacol commented 7 months ago

Describe the bug

<button onclick={async event => {
    console.log(event.currentTarget); // <button>
    await new Promise((resolve)=>resolve())
    console.log(event.currentTarget); // <body>
}}>Click me</button>

The first event.currentTarget correctly references the button, but after awaiting the promise (or in a then() ), it changes reference to the body

Problem disappears if I change the onclick to the old on:click

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA52Qvw6CMBDGX-XSCRIDO1oS4ws4uKlDLSdpLD3SXjGE8O6WiE5OTvfvu--Xu0ncjcUgqvMknOpQVGLf92IjeOyXIgxoGVMdKHq9dHa3yEwOyGlr9ENOKoxOAw7oGGQN08VdWJMLDKtSvoeFjt6neFK-Rd5-ZGSxsNRmPzQgpVxN8i2UJazsetlVT2UYHD7h6KkzAbPMY3IbMJf1mmV5_h-GmjFB5rk-LDdCh7vyi07P6Kgxd4ONqNhHnK_zC1Gr0X5HAQAA

Logs

No response

System Info

not relevant

Severity

annoyance

trueadm commented 7 months ago

There's not much we can do about this. We can list this as a breaking change, but the event object is shared along the listeners to improve performance. I thought we might be able to detect an async closure and bail-out but you could pass that along anywhere with the event object.

madacol commented 7 months ago

Yeah, I thought this was just a bug of svelte. I didn't realized it was almost standard behaviour (in firefox event.currentTarget goes to null in a setTimeout)

qupig commented 7 months ago

@madacol I don't know what happened to on:click in svelte5, but in svelte4, the behavior also exists. @trueadm So I don't think there's a "breaking change" there from svelte4.

The question instead is why the on:click behavior is different in svelte5 and svelte4?

Try the following code in the Svelte4 REPL and Svelte5 REPL:

<button on:click={async event => {
    const button = event.currentTarget;
    console.log(event.currentTarget === button); // true
    await new Promise((resolve) => setTimeout(resolve, 100));
    console.log(event.currentTarget === button); // false
}}>Click me</button>

RaiVaibhav commented 7 months ago

Hey @Conduitry this is actually a bug?, two things I noticed after I went through the code.

(For now we will just compare v4 and v5(which includes v4 behind the hood))

on:Click : I use on:Click then it's adding a signaling event i.e.,

    $.event(
        "click",
        button_1,
        async (event) => {
            debugger;

            const button = event.currentTarget;

            console.log(event.currentTarget === button); // true
            await new Promise((resolve) => setTimeout(resolve, 100));
            debugger;
            console.log(event.currentTarget === button); // false
        },
        false
    );

which means the currentTarget is button and the param dom is button and after await its uses the same event object with same currentTarget

function event(event_name, dom, handler, capture, passive)

onclick: this handled entirely in a very different way i.e.,

    const event_handle = (events) => {
        for (let i = 0; i < events.length; i++) {
            const event_name = events[i];
            if (!registered_events.has(event_name)) {
                registered_events.add(event_name);
                // Add the event listener to both the container and the document.
                // The container listener ensures we catch events from within in case
                // the outer content stops propagation of the event.
                target.addEventListener(
                    event_name,
                    bound_event_listener,
                    PassiveDelegatedEvents.includes(event_name)
                        ? {
                                passive: true
                            }
                        : undefined
                );
                // The document listener ensures we catch events that originate from elements that were
                // manually moved outside of the container (e.g. via manual portals).
                document.addEventListener(
                    event_name,
                    bound_document_event_listener,
                    PassiveDelegatedEvents.includes(event_name)
                        ? {
                                passive: true
                            }
                        : undefined
                );
            }
        }
    };

Here two event listeners are added one in the root, which can be body and another is dom, now first listnere got called but in this case the handler_element is not button its body or root div, and so svelte, internally assign the currentTarget to button,

Till now on the first check i.e., event.currentTarget === button it will say true but actually is not bubbled more like captured form the root of the app.

Now await is called in between 2nd listener is activated and now the handler element is dom means the current object is now changed, so on the second check it will say false i.e., event.currentTarget === button

Just wanted to understand, is making changes in the event object, will not create more bugs?

brunnerh commented 7 months ago

@trueadm How relevant is this to performance? I think this might be quite pernicious (and documentation usually will not cut it).

dummdidumm commented 7 months ago

What other frameworks do, and what we could also offer, is a way to opt out of event delegation through a compiler option. I'm not sure that this bug right here warrants changing anything though - especially considering that other frameworks like React and Solid have the same gotcha.

RaiVaibhav commented 7 months ago

Isn't supporting the v4 methodology is better? and make the on:click: same as onClick, I don't understand why rethinking is needed here, is js size is the issue?

trueadm commented 7 months ago

@brunnerh Event delegation has a significant impact on performance in real world applications. React has an event. persist() that will generate a static event object. We could do something the same but with an imported helper that we document.

madacol commented 7 months ago

For the record, these are the differences I have noticed about how and when event.currentTarget changes reference:

Svelte 5 `onclick`

Changes after any await, .then(), or inside a setTimeout (including when the Promise resolves synchronously)
Changes to <body>

Tested with

[REPL](https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA5WPwQqDMAyGXyX0pDDm3Wlh7AV22G3u0NUoZbWRNioivvva4QvslP9PyPcnm-iMxSDK5yacGlCU4jqO4iR4HZMJM1rG6ANNXqdO9Z6YyQE5bY3-1JsKq9OAMzqGWsLWuIY1uUAWz5b67Dc568n7WB_K98j5BYoCDpJMC2pRhsHhAndPgwmYZR4jYsa8lofK8vwPNrVrJO-7vKUzYcCqOPLiNwO1pjPYipL9hPtr_wIwnimTCAEAAA==) ```svelte ```

Svelte 5 `on:click`

never changes reference

Tested with

[REPL](https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA52PQQrCMBBFrzJk1YLYuq22IF7ARXfWRYyjBJOZkkwqUnp3W9ELuBr-G-b_P6O6WYdRVadRkfaoKrXve7VS8uoXEQd0grOOnIJZSEe7SxJhAqbKOGse9ajjiwzggCRQNzB21Ilhiuxw7fiefTZrk0KYZ6vDHSXfQlHA16pZDvRTWwHCJxwDexsxywLOFgPmdRNRWuuRk_zgalOWZZ7_ETVNzWHpDR53xZfOH3q-2pvFq6okJJzO0xv3Z0IAHAEAAA==) ```svelte ```

Svelte 4 `on:click` and in vanilla JS `onclick`

Changes reference after an operation that runs asynchronously (after an await and .then() where the Promise resolves asynchronously, also inside a setTimeout, etc)
Changes to null

E.g it changes when doing

await new Promise((resolve)=>setTimeout(resolve,1000))

but not on

await new Promise((resolve)=>resolve())

Tested with

```svelte ``` and ```html ```

Tested in Firefox, and some in chromium

trueadm commented 7 months ago

@madacol Yes, as I mentioned above, this is to be expected when using onclick in Svelte 5. We're doing our own event delegation for improved memory/performance (like many other frameworks do) and sharing the event object between listeners is an optimization. If you use on:click we don't do this in Svelte 5 as we opt out of event delegation to keep parity with Svelte 4.

qupig commented 7 months ago

@madacol

when the Promise resolves synchronously ???

No, there is no synchronous resolves async function.

What you see is that the entire handler is delayed asynchronously.

This is what await actually does. You can never predict the execution order and timing of async functions.

What we need to know is that according to the documentation, currentTarget always points to the element to which the event is currently propagated in the bubbling order.

So yes, when the async function finished before the event bubbles up to the next element, it behaves like currentTarget has not changed, which is why I gave it an explicit delay with setTimeout.

sveltejs / svelte