sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.55k stars 4.12k forks source link

Svelte 5: dispatched "native named" events not reaching handler #12147

Closed CaptainCodeman closed 2 months ago

CaptainCodeman commented 3 months ago

Describe the bug

Manually dispatched events with names matching native events don't reach handlers when new on... syntax is used vs when older on:... syntax is used.

Reproduction

Click button, 3 events are raised (and also raise if the input is typed in + left): https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE5WQwWrDMBBEf0XsJTYY525sQyn9iqoHR17HovJKWKuUIvTvRXJo2mNOYuZpZmEiLNqgh-49Ak0bQgcvzkED_O2y8Dc0jNCAt2FX2em92rXjUZJkgyw0ucCSslwCKdaWxDrRbHCvsBYxA8nKkrcGW2OvFba5vM4gFfo3uk_aY_WbK-3trL2bWK1vNySuCL_Ea_Bst0OfyqdTXT8RUetEV3wyU8Q9kyT158cU1JcKcdE0d7xqP8RiJGGpO44N8T5L8Qr9bx39D2-U1F8Cs6VCjVafQyz7pLE8_fnAIzSw2VkvGmfoeA-YPtIPUdMhYtoBAAA=

Press migrate to switch to runes syntax. Now only the custom event reaches the handler.

i.e. switching from on:input to oninput stops the event handler firing for manually dispatched events.

Logs

No response

System Info

System:
    OS: macOS 14.5
    CPU: (10) arm64 Apple M1 Pro
    Memory: 197.67 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.7.0 - ~/Library/pnpm/node
    npm: 10.1.0 - ~/Library/pnpm/npm
    pnpm: 9.4.0 - ~/Library/pnpm/pnpm
  Browsers:
    Chrome: 126.0.6478.63
    Edge: 126.0.2592.68
    Safari: 17.5
  npmPackages:
    svelte: 5.0.0-next.160 => 5.0.0-next.160

Severity

annoyance

CaptainCodeman commented 3 months ago

Looks like they now need "bubbles" set on the custom event:

input.dispatchEvent(new CustomEvent('input', { bubbles: true }))
CaptainCodeman commented 3 months ago

Hmmn, I say it works ... for the example it does, for something in a separate package it doesn't seem to. I'll test it some more.

paoloricciuti commented 3 months ago

The problem is that manually created events don't bubble by default and actual dom events without the colon use the global listener (which requires bubbling to work).

So when you switch to colonless events the native events gets delegated to the body but your dispatched event never reaches the body.

I don't think there's a way to fix this without either removing the delegated events thing (which I don't think is feasible now) or monkey patching dispatch event.

I wonder if a decent workaround could be listening to the capture phase and check if the event will bubble and if not dispatch a new event that bubbles. But I fear this could get more convoluted than necessary

7nik commented 3 months ago

Can dispatch from svelte/events be implemented to "fix" this?

paoloricciuti commented 3 months ago

Can dispatch from svelte/events be implemented to "fix" this?

I guess the problem is if libraries are using this, if it's user lang code you can just add bubble: true

CaptainCodeman commented 3 months ago

Are you missing the bit about behavior changing depending on the event name? Not only is it different to how it used to behave, it's also inconsistent.

I need to confirm if bubbles worked from library code, when I tried it last night it didn't appear to, but it was late and I may have been doing something stupid.

Update: I was doing something stupid! (set bubbles on event detail instead of the event itself)

paoloricciuti commented 3 months ago

Are you missing the bit about behavior changing depending on the event name? Not only is it different to how it used to behave, it's also inconsistent.

Nope I've actually explained why that happen.

need to confirm if bubbles worked from library code, when I tried it last night it didn't appear to, but it was late and I may have been doing something stupid.

With bubble it should work.

CaptainCodeman commented 3 months ago

Maybe I'm not explaining it well. I know the trusted events that the browser dispatches work, but all the events dispatched from clicking the button in the example are manually dispatched, and the one with the name "custom" works without bubbles which is why it seems to be inconsistent.

Another question: what if you don't want an event to bubble? The listener and dispatch are both on the same DOM element so bubbling shouldn't be needed.

paoloricciuti commented 3 months ago

Maybe I'm not explaining it well. I know the trusted events that the browser dispatches work, but all the events dispatched from clicking the button in the example are manually dispatched, and the one with the name "custom" works without bubbles which is why it seems to be inconsistent.

Because the only events that are delegated are the "non custom" ones

CaptainCodeman commented 3 months ago

Because the only events that are delegated are the "non custom" ones

Why? They are all custom events, and none of the events are set to bubble, but one does?

It changes how events worked with the on: syntax, so would at least be a breaking change, but it's inconsistent and different to the normal expectations of how events work with the DOM. Having onwhatever behave differently to dom.addEventListener('whatever', () => {}) or dom.onwhatever = () => {} in an action, but only for events with certain names is going to be very confusing.

For comparison, without Svelte, it works how I'd expect - the events only reach the div listeners if they are set to bubble, and they always reach the input listeners without or without bubbling (because they are dispatched from that element):

<!DOCTYPE html>
<div id="div">
    <input id="input" type="text">
    <button id="bubbles">bubbles</button>
    <button id="regular">regular</button>
</div>
<script>
    function handle(name) {
        return function(e) {
            console.log(name, e.type)
        }
    }

    div.addEventListener('input', handle('div'))
    div.addEventListener('custom', handle('div'))

    input.addEventListener('input', handle('input'))
    input.addEventListener('custom', handle('input'))

    bubbles.addEventListener('click', () => {
        input.dispatchEvent(new CustomEvent('input', { bubbles: true }))
        input.dispatchEvent(new CustomEvent('custom', { bubbles: true }))
    })

    regular.addEventListener('click', () => {
        input.dispatchEvent(new CustomEvent('input'))
        input.dispatchEvent(new CustomEvent('custom'))
    })
</script>
paoloricciuti commented 3 months ago

Why? They are all custom events, and none of the events are set to bubble, but one does?

What I mean is that click is a known Dom event so it's delegated to the body because they can add a global click listener and run all the events that bubbles. "custom" is not a known Dom event so they can't do that and so they add the listener to the Dom element itself. Since none of your events bubbles the only one logged is "custom" that is attached to the Dom element and not to the body.

I'm not saying is desiderabile. I'm just explaining why it's happening.

CaptainCodeman commented 3 months ago

It seems like a simplistic / fundamentally flawed approach to me from my (possibly limited) understanding. You can't really tell from the event name / type whether the event you're going to get will be a custom event or a native event (as in "dispatched by the browser" vs "dispatched programatically"). AFAIK the only way to check is by looking at the event itself, e.g. for the isTrusted property on it.

This seems like a step backwards. It breaks expectations of how HTML + JS work. I'd much rather have correct & consistent than something that may only usually be of benefit to benchmark runs.

CaptainCodeman commented 3 months ago

A different example, where an imported use:action may want to communicate back to the node via an event. If it happens to use one of the special event names, it stops working when migrated unless bubbles is added, easier if the code is within your app, but may trip people up if they don't know about this breaking change and the special behavior for the affected event types isn't documented.

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE5VSTY-bMBD9KyP3ECIRdvuhHtjsStWqPbVSj63KHlh7CE7Bg-yBxUL898o4JOn21AOHmed5vDdvJlHpBp3If03ClC2KXHzqOpEK9l0o3IANo0iFo97K0Nk7aXXHD4UpWLcdWYYJZG8dWZihstTCJruJjezoNoUJLxtkGOEebtfCnwpTcNUbyZoMkHmsS3PABLcwBaTgMIOZQi51k40Z0xc9okrebiPsr2H_Cp4Ls7-5qDX77uEbDQieegst9Q6BBrTANcIzjfubLj5TeoDeYX4yRSaXi6z7aRU4L-6ncU5h8stvlB7isGPf4AIHmpMLpV3XlD6HqsHxLvbKRh_MTjO2LgeJhtGekGPvWFd-J8kwGn6FvmjFdQ7vP952K1WN-lBzDu8-XHrPpfx9sNQbtZPUkM3hjZTy7rKXRadIRUtKVxqVyNn2OKfnO4j2L6dwdNdngOMS_Tm7-DoxpNbwrmL9Tjo4CNu_ylaScfEqMKOqcsg_rgF_BfyMQGDPwjJLlvXnAQ0nBl_gsXdMbaw3MatNChPEu8hhgjEFDzPMsF0vo-DwLYSlUsvsV-0YDdpk00W9LQ2B6C_923XUIvfWnBNGx5Z8cjZ34rYYSP6XPgqcCzP_G9DT_Af33bmHtAMAAA==

paoloricciuti commented 3 months ago

A different example, where an imported use:action may want to communicate back to the node via an event. If it happens to use one of the special event names, it stops working when migrated unless bubbles is added, easier if the code is within your app, but may trip people up if they don't know about this breaking change and the special behavior for the affected event types isn't documented.

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE5VSTY-bMBD9KyP3ECIRdvuhHtjsStWqPbVSj63KHlh7CE7Bg-yBxUL898o4JOn21AOHmed5vDdvJlHpBp3If03ClC2KXHzqOpEK9l0o3IANo0iFo97K0Nk7aXXHD4UpWLcdWYYJZG8dWZihstTCJruJjezoNoUJLxtkGOEebtfCnwpTcNUbyZoMkHmsS3PABLcwBaTgMIOZQi51k40Z0xc9okrebiPsr2H_Cp4Ls7-5qDX77uEbDQieegst9Q6BBrTANcIzjfubLj5TeoDeYX4yRSaXi6z7aRU4L-6ncU5h8stvlB7isGPf4AIHmpMLpV3XlD6HqsHxLvbKRh_MTjO2LgeJhtGekGPvWFd-J8kwGn6FvmjFdQ7vP952K1WN-lBzDu8-XHrPpfx9sNQbtZPUkM3hjZTy7rKXRadIRUtKVxqVyNn2OKfnO4j2L6dwdNdngOMS_Tm7-DoxpNbwrmL9Tjo4CNu_ylaScfEqMKOqcsg_rgF_BfyMQGDPwjJLlvXnAQ0nBl_gsXdMbaw3MatNChPEu8hhgjEFDzPMsF0vo-DwLYSlUsvsV-0YDdpk00W9LQ2B6C_923XUIvfWnBNGx5Z8cjZ34rYYSP6XPgqcCzP_G9DT_Af33bmHtAMAAA==

Yeah as I've said this either needs to be documented as a breaking change or fixed in some way.

trueadm commented 3 months ago

We just need to document what events are delegated.

We only delegate a specific amount of events: https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/constants.js#L37-L61

You only need to pass bubbles: true when dispatching events that are delegated.

CaptainCodeman commented 3 months ago

I'm guessing using capture mode isn't an option for some reason, even if just to detect and "patch" the behavior for consistency?

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE51UbYubQBD-K4MtFwOed32hH7wkcFyv7UFbQl-gpZay0THuVXdld8wp4n8vu2uiSTkK_aLuPLMzjzPPTOdlvEDtRT86T7ASvci7riov8KitzEHvsCD0Ak_LWiXGstCJ4hWtYhETLyupCDpIaqWlgh4yJUuYhRfOEN7rWSyMZ4EEDSzhqSZG6F_O98b2xChiymqREJcCpLjJmdiij3PoDBKTiYFhisR4ETYhyTe8wdR_NndwO4XbE7g_CZ-wimqFvpDpmOCA5kykBapJ7ph4Bj6GuENB65xphOVyCbfmGN5cr798_XT38e2v9bvrz7dwdgYYburNpkBt3TJW6GmsmDDUJKu1khXbMpPTn5-Cd2WJKWeEj3pVytJ5jRmrC5piiRSawKKwBIEPcFNrkqXl62NoOhxAB2EYYgCuZtGhegEM5CMgVSP0R1mJqS1SmHJdMUryIaR5Htx69zG83NNUOmRpav3fc00oUPmzxHZ5FuxrblgNzZlmtyEUUq3EWMYUNSnZRuDPYbmaltcmU1jKHf5fvr9-o4_F4mKUv1iUjAuoNUbDbTsUi2r1Qe4QWlkrKGWtEeQOFVCOsJHN4qKybtY15Tt3382PFI7asttLv3e-1DV9AF1raSwuUr5bGS4mv2OiqS1cehNyqIJpT8HaCLICmytnYwXfinNOWOoIEhSEakDua008a88TKQgFnaAPPKU8ghevLqt9qBz5NqcInr8cbRuW_N4qWYv0PJGFVBE8SZLkaqye5ekFXilTnnFMvcgUvA8O68eVYtxA93q6fbCxG2ecYes9HeHJ9lhLbv7AdGMyxm4u3CKRWaaRvk2BdgJ8n8j2WOun4zRqqjuMUgdNAC300MP8sIBs3x-Zg8rxNYqdBcf85_urx_of1O9P9srjsv9n-IPK-78b9LP_AxmUVxcrBgAA

dummdidumm commented 2 months ago

Adding bubbles: true fixes all these cases, and this is what should be done when dispatching events that happen to have the same name as native ones that are delegated. This is mentioned in the soon-to-appear final docs, therefore closing.