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

Svelte 5: Event delegation thoughts #9714

Closed
dummdidumm opened this issue Nov 30, 2023 · 16 comments
Closed

Svelte 5: Event delegation thoughts #9714

dummdidumm opened this issue Nov 30, 2023 · 16 comments
Milestone

Comments

@dummdidumm
Copy link
Member

Describe the problem

#9696 uncovered a major flaw in our event delegation logic: If an event is stopped from being propagated, and that event is delegated, then the propagation isn't really stopped. If there's now another event handler for the same event type above that delegated listener but below the root that listens to the delegated events, and that event handler is not delegated, then it will still get notified of the event.

Describe the proposed solution

I'm honestly not sure if we can fix this in a sensible way that covers all cases, at least not as long as we have on:x still around.

Alternatives considered

Live with the edge cases of this sometimes not working correctly

Importance

would make my life easier

@dummdidumm dummdidumm added this to the 5.0 milestone Nov 30, 2023
@chainlist
Copy link

chainlist commented Nov 30, 2023

I wrote the original issue where that MR came out.

I don't know how on{event} and on:event differ internally, but would it be possible to consider the old way of attaching the events exactly like the new one?
For this to work out I guess it would mean to handle correctly events modifiers like |preventDefault, |once and so on, which are not provided by default by svelte.

Is it possible that svelte exposes those new events modifier functions and use them internally when on:event|modifier is used but treated as the new way?

<script>
 // Hypotetical new export
import { preventDefault } from 'svelte/events'

function handler() {}
</script>

<!-- This event handler -->
<button on:click|preventDefault={handler}>Click me</button>

<!-- would be read and transformed as this -->
<button onclick={preventDefault(handler)}>Click me</button>

By doing that I hope we could completely get rid off the logic to handle on:event

The issue with that is I don't really know how we could have event modifier functions that change the listening behavior of the event (capture or bubble) or even changing the passive property of an event.

@Not-Jayden
Copy link
Contributor

@chainlist I've made this package to explore how event handling could be supported in Svelte 5 + provide the modifier wrapper functions https://github.com/Not-Jayden/svelte-event Still early stages and agree it would be nice if svelte provided them.

Also not sure if any of this is at all related to what Simon is discussing tbh haha.

@dummdidumm
Copy link
Member Author

Related: #9777

@trueadm
Copy link
Contributor

trueadm commented Dec 6, 2023

So what are the cases where we are mixing delegated events and non-delegated events of the same type? Surely Svelte should only be applying one kind here? If the user is adding their own custom event handlers, then we could improve interop like the event system does on React, but maybe we first need to understand all the cases.

@jakelazaroff
Copy link

jakelazaroff commented Dec 7, 2023

I'm not sure whether this happens for event handlers that Svelte manages, but one scenario that I've run into is when adding my own event handlers. For example:

  • I've been experimenting with HTML web components as a way to build framework-agnostic functionality, and using them within Svelte can run into this problem, since web component event handlers are generally bound directly to the custom element itself which is in the middle of the Svelte DOM tree.
  • Using Svelte's actions can lead you into the same issue — the action is called with the raw DOM node, which means that any event handlers it attaches will be bound to an element in the middle of the tree.

As an example, if you run this component and click the button, the logs will be in the "wrong" order:

<script>
	let i = 0;
	
	let defined = false;
	if (!defined) {
		defined = true;
		customElements.define("web-component", class extends HTMLElement {
		constructor() {
				super();
				this.addEventListener("pointerdown", this);
			}

			handleEvent() {
				console.log("web component", ++i);
			}
		});
	}

	function action(node) {
		node.addEventListener("pointerdown", () => console.log("action", ++i));
	}
</script>

<web-component>
	<div use:action>
		<button on:pointerdown={() => console.log("event handler", ++i)}>hello!</button>
	</div>
</web-component>

That is, you'll see

action 1
web component 2
event handler 3

rather than

event handler 1
action 2
web component 3

Here's a minimal repro in the Svelte 5 REPL: https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE41SPW_DIBT8K4TJVdKks-NY6hCpQ7p1qzs48Nwg4YdlHkkry_-9YOy4ljpUDPCOe3fHR8crpcHy9L3jWNbAU_7cNHzD6bsJhb2CJvC1Na4VAcmsaFVDeYEFaSCm2IE97UM1IRIqhSA9XpXawrCnKpasxo0H1g10monUusgrSDhLpj5qqAHJbiMnKfgNzo_C1I1Bjxd8w4QurWXwRYDSspe319PYNMkLg9YLCzJtcvcMw7oGPLSfEboouy2lPF59_0lZr-kZBW-MQoJWmhsGy0Cb23q_GJeXEqWGoXtpFTIYDVttPuMZ2OIM67Va6oUpIqN65VCQMsjKYUrQSLg7hOIfsX2kQ86WUaLclOFume3m58VscefDi2dSXZmzkMb-PObIzo7IZzSY_rI-dH8ZQ8jK4oW1k3-fX0Brs8p2USl67bxZHjItc_jfWBupKgWSp-Hn9B_9DzeJdiPIAgAA

I appreciate why event delegation is the default behavior, but maybe it's worth adding an additional modifier to bind event handlers directly? So I could opt in via something like this:

<button on:pointerdown|direct={() => console.log("event handler", ++i)}>hello!</button>

@dummdidumm
Copy link
Member Author

Others also have problems with edge cases: solidjs/solid#1786

dummdidumm added a commit that referenced this issue Jan 12, 2024
Only delegate event attributes, and don't take into account bindings/actions while determining that. Never delegate `on:` directives. This simplifies the logic and makes it easier to explain, while avoiding any accidental breaking changes when upgrading from Svelte 4 to 5 without changing code.
Fixes #10165
Related to #9714
trueadm pushed a commit that referenced this issue Jan 12, 2024
Only delegate event attributes, and don't take into account bindings/actions while determining that. Never delegate `on:` directives. This simplifies the logic and makes it easier to explain, while avoiding any accidental breaking changes when upgrading from Svelte 4 to 5 without changing code.
Fixes #10165
Related to #9714
trueadm pushed a commit that referenced this issue Jan 13, 2024
* fix: simplify event delegation logic

Only delegate event attributes, and don't take into account bindings/actions while determining that. Never delegate `on:` directives. This simplifies the logic and makes it easier to explain, while avoiding any accidental breaking changes when upgrading from Svelte 4 to 5 without changing code.
Fixes #10165
Related to #9714

* update types
@minht11
Copy link

minht11 commented Jan 14, 2024

I have event listener in the container action. Right now calling e.stopPropagation() on button click does not stop parent action listener. Is that expected limitation or a bug I should report? Reproduction

<script>
	function buttonHandler(e) {
		e.stopPropagation()
		console.log('clicked button')
	}

	const action = (node) => {
		node.addEventListener('click', () => {
			console.log('Clicked action')
		})
		
		return {}
	}
</script>

<div use:action>
  Container with action
  <button onclick={buttonHandler}>
    Button
  </button>
</div>

@trueadm
Copy link
Contributor

trueadm commented Jan 14, 2024

@minht11 This is to be expected. If you change the button to be on:click it will attach directly to the element without delegation and that should work.

@jonshipman
Copy link

This is what I've been doing: svelte 5 dev link

The demo link has event.stopPropagation in the listener action, no reason there can't be moved to the module scope in an export function or added as an additional prop in the listener. E.g. use:listener={{event:'custom', callback, stopPropagation: true}}

@minht11
Copy link

minht11 commented Apr 29, 2024

Since #11295 mixing new and old event systems is forbidden. Any solution to my use case in #9714 (comment)?

@vnphanquang
Copy link

vnphanquang commented Jun 5, 2024

Running to the same problem as @minht11 did with stopPropagation not working for event listeners registered on ancestor element via addEventListener. This is affecting some of my custom Svelte actions where using addEventListener is necessary.

Please find below for a example of working code in Svelte 4 and the equivalent but not working in Svelte 5. Happy to create a dedicated issue if necessary.

As of Svelte 4.2.17

See REPL here

<script>
	import { onMount } from 'svelte';
	let count = 0;

	function increment(e) {
		e.stopPropagation();
		count += 1;
	}

	let sectionEl
	onMount(() => {
		sectionEl.addEventListener('click', (e) => {
			console.log('logged from addEventListener');
		});
	});
</script>

<section bind:this={sectionEl} on:click={() => console.log('logged from on:click')}>
	<button on:click={increment}>
		clicks: {count}
	</button>
</section>

Clicking on button does not yield any logs.

As of Svelte 5.0.0-next.136

See REPL here

<script>
	let count = $state(0);

	function increment(e) {
		e.stopPropagation();
		count += 1;
	}

	let sectionEl
	$effect(() => {
		sectionEl.addEventListener('click', () => {
			console.log('logged from addEventListener');
		});
	});
</script>

<section bind:this={sectionEl} onclick={() => console.log('logged from onclick')}>
	<button onclick={increment}>
		clicks: {count}
	</button>
</section>

Clicking on button does not yield log from onclick but DOES yield log from sectionEl.addEventListener callback.

Workaround

onclickcapture does the trick:

- <button onclick={increment}>
+ <button onclickcapture={increment}>

@trueadm
Copy link
Contributor

trueadm commented Jun 5, 2024

#11912 should help here

@dummdidumm
Copy link
Member Author

Closing since the caveats are documented and we have on to deal with manual listening

@7nik
Copy link

7nik commented Aug 12, 2024

@Facius use on instead of node.addEventListener.

@Facius
Copy link

Facius commented Aug 12, 2024

@Facius use on instead of node.addEventListener.

@7nik I see, thank you for the link. Though I would still argue that it's kinda unintuitive, feels like a step further way from vanilla web.

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

No branches or pull requests

10 participants