sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.77k stars 1.96k forks source link

Sveltekit filebase router uses anchor click events. So do not stop propagation of these events. #4347

Open voscausa opened 2 years ago

voscausa commented 2 years ago

Describe the bug

I used a click event in a pulldown menu components of a NavBar to close the pulldown after a menu item was clicked. The click handler in the pulldown menu component used stopPropagtion. Doing so the click event could not reach the filebased router and the browser took over.

This should in my opinion be clear in the docs.

Reproduction

Block the click bubble using stopPropagation of routing anchors

Logs

No response

System Info

Not applicable

Severity

annoyance

Additional Information

Took me a couple of hours to find out why the Svelkit router ignored the anchor click of items in the pulldown menu. For the pulldown menu component I used code, which worked fine in a Svelte project using my own hash router. But the Sveltekit file-based router uses clicks to route.

voscausa commented 2 years ago

See also this Stackoverflow question: How can I close my dropdown menu when sveltekit route changes?

Rich-Harris commented 2 years ago

I wonder if we should listen for the event in the capture phase instead, so we're not affected by stopPropagation. Maybe that's undesirable, and this is just a docs issue?

nathancahill commented 1 year ago

Oh, this bit me hard. Spent a whole day debugging why a link was causing a full page reload instead of client-side navigation. If not making capture phase the default, it would be hugely helpful to add it as a compiler option.

zwergius commented 1 year ago

I ran into this issue having a <tr on:click={...}> where there were <a> inside the table rows. In the end I had to make a workaround:


function handleClick(e, id) {
  if ((e.target as HTMLElement).tagName === 'A') return
  goto(`route/${id}`)
} 
eltigerchino commented 1 year ago

I wonder if we should listen for the event in the capture phase instead, so we're not affected by stopPropagation. Maybe that's undesirable, and this is just a docs issue?

I'm not sure if listening during the capture phase will work. It would prevent obvious things such as <a on:click|preventDefault> from stopping navigation. Documenting seems like the better approach.

On a side note: https://javascript.info/bubbling-and-capturing#stopping-bubbling

Don’t stop bubbling without a need! Bubbling is convenient. Don’t stop it without a real need: obvious and architecturally well thought out.

Sometimes event.stopPropagation() creates hidden pitfalls that later may become problems.

For instance:

We create a nested menu. Each submenu handles clicks on its elements and calls stopPropagation so that the outer menu won’t trigger.

Later we decide to catch clicks on the whole window, to track users’ behavior (where people click). Some analytic systems do that. Usually the code uses document.addEventListener('click'…) to catch all clicks.

Our analytic won’t work over the area where clicks are stopped by stopPropagation. Sadly, we’ve got a “dead zone”.

There’s usually no real need to prevent the bubbling. A task that seemingly requires that may be solved by other means. One of them is to use custom events, we’ll cover them later. Also we can write our data into the event object in one handler and read it in another one, so we can pass to handlers on parents information about the processing below.

ptrxyz commented 1 year ago

Oh, this bit me hard. Spent a whole day debugging why a link was causing a full page reload instead of client-side navigation. If not making capture phase the default, it would be hugely helpful to add it as a compiler option.

Ended up in exactly the same situation and I would also love to see a setting for this. :)

ptrxyz commented 1 year ago

In case someone wants to look into this, I think the relevant part here is this: https://github.com/sveltejs/kit/blob/ce4fd764e271b1a461979dfaf9698af6f36e3714/packages/kit/src/runtime/client/client.js#L1508

And for a workaround, you can always call goto manually:

<a href="/main" on:click|preventDefault={() => goto('/main')}>Back</a>