`event.stopPropagation` is not working

chenzn1 commented 1 year ago

Describe the bug

I used event.stopPropagation in the event in the application, but the event can still be executed outside the application.

Your Example Website or App

https://codesandbox.io/p/sandbox/cool-thunder-mw586z?welcome=true

Steps to Reproduce the Bug or Issue

Solidjs component

export defualt App() {
  return (
    <button
          style="margin-left: 20px"
          onclick={(event) => {
            console.log("solidjs click");
            event.stopPropagation();
          }}
        >
          click
        </button>
  )
}

index.html

<body>
    <div id="root"></div>
    <script src="/src/index.jsx" type="module"></script>
  </body>
  <script>
    document.body.addEventListener('click', () => {
      console.log('click')
    })
  </script>

click button of solidjs app, the click event of document.body will execute

Expected behavior

event.stopPropagation works fine.

Screenshots or Videos

No response

Platform

OS: [e.g. macOS, Windows, Linux]
Browser: [e.g. Chrome, Safari, Firefox]
Version: [e.g. 91.1]

Additional context

No response

wjq990112 commented 1 year ago

This behavior is different between native and delegated event:

https://playground.solidjs.com/anonymous/12059b7a-e53d-4e08-a13e-7be34da1b27d

ryansolid commented 1 year ago

Yeah, it is because all events are delegated from the document. The main reason for doing this is to support Portal delegation to be fair which could be placed anywhere in the document. The solution I suppose would be to keep track of all delegated events and re-apply them over portals. I think this is a good suggestion but will need some reasonable changes. Mostly making DOM Expressions Portal aware and changing how event delegation works. This will require at least a minor change if not major.

Today this is very much by design but I'm open to updating this in the future.

nzdeep commented 1 year ago

Perhaps I'm misunderstanding, but as I understand in this scenario then, shouldn't e.currentTarget be document? Currently it looks like currentTarget is faked to the button? (https://developer.mozilla.org/en-US/docs/Web/API/Event/currentTarget)

ryansolid commented 1 year ago

The idea is that you can treat the delegated event as if it were not delegated within the system so we patch the currentTarget. This is a trick I got from Inferno and I imagine React does something similar as well.

rnovacek commented 1 year ago

Is there any workaround for this?

I have a link and I want to prevent the navigation and handle that in the component. But still want the link to be there so people can open the edit page in a new tab. I cannot figure out how to prevent the navigation.

<a
    href="/edit"
    onClick={(e) => {
        console.log('click', e)
        e.preventDefault();
        e.stopPropagation();
        setIsEditing((editing) => !editing);
        return false;
    }}
>

rnovacek commented 1 year ago

Ok, this seems to work:

on:click={(e: Event): boolean => {
    e.preventDefault();
    e.stopPropagation();
    setIsEditing((editing) => !editing);
    return false;
}}

But typescript doesn't like it:

Type '{ children: Element; href: string; class: string; "on:click": (e: Event) => boolean; }' is not assignable to type 'AnchorHTMLAttributes<HTMLAnchorElement>'.
  Property 'on:click' does not exist on type 'AnchorHTMLAttributes<HTMLAnchorElement>'. Did you mean 'onclick'?

trusktr commented 1 year ago

I too have had gripes with this. It will be nice for JSX to be simply sugar for DOM as much as possible, with delegation as an an opt-in feature.

For example, if anything, onclick={} could be the native event handler directly on the element, and on:click={} with special syntax could be the opt-in delegation form. Perhaps a syntax like delegate:onclick={} would be a lot better, less ambiguous.

I also don't see how this has anything to do with Portals: if we have the reference to the element, we add an event listener to it, and it would simply work, no matter where in the DOM it is located. This would be the same way as if you ran document.body.append(el); el.addEventListener('whatever'); it would simply work.

Solid might be trying to do too much in this case. It might be that delegation solves some performance aspects that are usually negligible (perhaps this helps with benchmark results) but a delegation opt-in would easily solve that, while up-keeping intuitive expectations in the average case.

In my years of web dev, individual event handlers haven't been an issue for me, but I would delegate if needed, f.e. the familiar table with thousands of rows, but that's a special case, and defaulting to delegation everywhere for that special case might cause more problems than it is worth.

yume-chan commented 1 year ago

I also don't see how this has anything to do with Portals:

Event delegation let events propagate through the JSX tree, not the DOM tree, for example:

https://playground.solidjs.com/anonymous/f787bc00-e21e-4ea8-8f57-7eca6d931e72

function Counter() {
  return (
    <div onKeyDown={() => console.log("portal key down")} onKeyPress={() => console.log("portal key press")}>
      <Portal>
        <div onKeyDown={() => console.log("container key down")} onKeyPress={() => console.log("container key press")}>
          <input onKeyDown={() => console.log("input key down")} onKeyPress={() => console.log("input key press")} />
        </div>
      </Portal>
    </div>
  );
}

input is not a DOM descendant of the outermost div, without event delegation, events trigger from the input won't bubble to the outermost div. In this example, keydown is delegated, but keypress isn't. Notice that when you press a key on keyboard in the input, it prints "portal key down" but not "portal key press".

That's why Ryan was talking about

The solution I suppose would be to keep track of all delegated events and re-apply them over portals.

We can add event handlers to every Portal, to dispatch events back to their JSX tree position, without relying on event delegation for basic event dispatching.

IMO, event delegation makes working with Portals easier, it definitely has its benefits. But, as Solid is a lower-level DOM framework, users might more frequently use it alongside with native DOM operations, including event handlers. Or maybe users use a third-party library which adds event handlers without knowing Solid's event delegation. Both situations can cause hard to debug bugs.

Anyway, it's possible to disable event delegation (except for Solid Start), although not very easily. First you need to disable it when compiling components:

import solid from "vite-plugin-solid";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [
    solid({
      delegateEvents: false,
    }),
  ],
});

Then to disable Dynamic from using event delegation, add this to the entry point:

import { DelegatedEvents } from "solid-js/web";

DelegatedEvents.clear();

Solid Start's Vite plugin doesn't follow the delegateEvents: false config for route pages:

https://github.com/solidjs/solid-start/pull/1029

this-spring commented 1 year ago

I think the root node for delegated events should be on the root(id is app div) instead of document.

nmocruz commented 10 months ago

I found myself on a wierd/complex situation with bootstrap dropdowns and stopprogation on div to prevent that to close the dropdown. I had to go to a few components used inside that div and change click to on:click so I could make prevent bootstrap to close the menu.

This was also new for me, I was not understanding the behavior and the conflicts between delegated and bound to target events. I came from knockout and click are by the default bound, delegate is optional.

I am not sure if is better or not to have a click and a delegate:click and on guidance where is recommend to the delegation only in cases where it could cause a big performance impact. Because making all event delegated even when the only component of that type in the page, make not gains exchanged for a not "natural" way on how click events works on DOM.

solidjs / solid