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

Event handling is different between Preact and React #2887

Closed
just-boris opened this issue Dec 28, 2020 · 8 comments · Fixed by #3608
Closed

Event handling is different between Preact and React #2887

just-boris opened this issue Dec 28, 2020 · 8 comments · Fixed by #3608
Labels

Comments

@just-boris
Copy link
Contributor

Reproduction

There are two code sandboxes implementing the same thing in Preact and React

Steps to reproduce

  1. Click on "Expand me"
  2. Click on "close"

Expected Behavior

The panel should close (and it does in React)

Actual Behavior

The panel does not close in Preact


This seem to be related to the way these frameworks handle events. React uses event delegation and Preact applies direct event listeners. Hence the click event bubbles up and reaches the panel root when it already was re-rendered, which breaks the toggle implementation.

I found a similar issue #838 but since that one was already closed as resolved I opened a new one.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jan 5, 2021

I guess this is one of the core differences between using a custom event system and the one from the browser like we do. The browser's events bubble up like you described, whereas react changed that behavior.

To prevent events from bubbling up there is the event.stopPropagation() function:

<button onClick={e => {
    e.stopPropagation(); // Prevents event from bubbling up
    setExpanded(false);
  }}
/>

@just-boris
Copy link
Contributor Author

The workaround could work if I would have a control over that code.

Unfortunately, this is a 3rd party library and I can't change that. I might convince them to give preact 1st-class support, but this might take a while.

For now it is great to have this ticket as an indicator of things that you can't do with Preact

@marvinhagemeister
Copy link
Member

Which library are we talking about? Happy to PR a fix to them.

@just-boris
Copy link
Contributor Author

The library is a closed-source enterprise. I could submit a PR myself, but this will not be enough.

To make this work out, I need to also contribute them a full compatibility test suite, so they run all existing tests with Preact as well, to ensure there are no regressions. And this part is what makes it hard to sell.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jan 5, 2021

Your in luck as the same API is present in React too, although. So adding the event.stopPropagation() call will keep their test suite green. If your test runner supports aliasing (most runners do), you can alias React to Preact and verify that the code is working with both frameworks.

Jason wrote an article a while back on the nuanced differences of delegation vs direct binding and why delegation isn't a good default: https://jasonformat.com/event-delegation-vs-direct-binding/

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jan 5, 2021

You could also try doing:

import { options } from 'preact';

const oldEventOptions = options.event;
options.event = (e) => {
  if (e.type === 'click' && e.target.id === 'myFlakeyButton') e.stopPropagation()
  if (oldEventOptions) oldEventOptions(e);
}

You can adjust that first check to just target the button you like

@JoviDeCroock
Copy link
Member

This is fixed with switching to setTimeout for debouncing as well, we should include this or an alternative scheduler for 11 https://codesandbox.io/s/cocky-star-bw9sgv?file=/src/index.js

@JoviDeCroock
Copy link
Member

Fixed in #3608

KonnorRogers added a commit to KonnorRogers/preact-www that referenced this issue Nov 30, 2022
I couldn't find something that clearly stated if Preact used event delegation by default. I see it does not use the synthetic event system, but I think it would be nice to be clear that there is also not event delegation by default.

I had to dig through the issues to verify this was the case.

preactjs/preact#2887 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants