Event fired twice [bug only appears in latest version v10.13]

[quentinadam]

• Check if updating to the latest Preact version resolves the issue

The issue appears in latest v10.13 only (v10.12.1 and before seem to be working fine).

Describe the bug

I have a simple component with two states. When no option has been selected, the component should show a list of options. Clicking on one option should switch the component in the second state that only shows the selected option.

When preact v10.13 is used, clicking on an option seems to fire a click even on both alternative states of the component, hence the component does not work as expected.

To Reproduce

Here is a codesandbox link with preact v10.13 that exhibits the bug: https://codesandbox.io/s/condescending-rui-5own5r

Here is the code:

function Combobox({ options }) {
  const [option, setOption] = useState(undefined);
  if (option !== undefined) {
    return (
      <div style={{ border: "1px solid #ccc", margin: "10px", padding: "10px", cursor: "pointer" }}
        onClick={() => {
          console.log("Clear option");
          setOption(undefined);
        }}
      >
        {option}
      </div>
    );
  } else {
    return (
      <div style={{ border: "1px solid #ccc", margin: "10px", cursor: "pointer" }}>
        {options.map((option) => {
          return (
            <div
              style={{ padding: "10px" }}
              onClick={() => {
                console.log("Set option", option);
                setOption(option);
              }}
            >
              {option}
            </div>
          );
        })}
      </div>
    );
  }
}

const root = document.getElementById("root") ?? document.getElementById("app");

render(<Combobox options={["option 1", "option 2"]} />, root);

Steps to reproduce the behavior:

1. Clicking on "option1" or "option2" should normally change the component, but nothing happens
2. You can check in the console that the click event is being fired twice on both alternative states of the component, toggling it back to the initial state.

As mentioned earlier, when selecting preact v10.12.1 or below, the component works as expected.

Here is a codesandbox link using preact v10.12.1 that works fine: https://codesandbox.io/s/confident-keldysh-r4f5ow

Bug is also present when copy/pasting the code on the preact repl (https://preactjs.com/repl)

[JoviDeCroock]

This is a side-effect of us switching back to microticks for events in 10.13.0, we are reevaluating how our scheduler works internally as the DOM seems to bite us sometimes for these. You can yourself bypass this by doing something like options.debounceRendering = setTimeout but we also found issues there 😅 sorry that I can't offer you a better explanation right now.

The gist of this issue is that you click an option, you setState and that gets triggered after a micro-tick in Preact itself, which is actually faster than what the DOM schedules for event-bubbling so by the time the event bubbles up the new div with an onClick is there to catch it.

I have written a bit about it in here when we did the switch to setTimeout which did not actually end up being the best solution. Maybe requestIdleCallback will solve our problems but we'll have to check further, I am sorry for this inconvenience.

[ritschwumm]

@JoviDeCroock thanks a lot for the detailed explanation!

i was a bit scared this issue might bite me in my current project, but from this i can deduce it probably won't.

[developit]

@ritschwumm @quentinadam you can avoid this issue by stopping event propagation:

function Combobox({ options }) {
  const [option, setOption] = useState(undefined);
  if (option !== undefined) {
    return (
      <div style={{ border: "1px solid #ccc", margin: "10px", padding: "10px", cursor: "pointer" }}
        onClick={() => {
          console.log("Clear option");
          setOption(undefined);
        }}
      >
        {option}
      </div>
    );
  } else {
    return (
      <div style={{ border: "1px solid #ccc", margin: "10px", cursor: "pointer" }}>
        {options.map((option) => {
          return (
            <div
              style={{ padding: "10px" }}
+             onClick={(e) => {
+               e.stopPropagation();
                console.log("Set option", option);
                setOption(option);
              }}
            >
              {option}
            </div>
          );
        })}
      </div>
    );
  }
}

[krskrs]

Hi all, but in the end, what's the official position on this behavior? Is it considered good enough and will stay as it is? Or it is considered not satisfying enough yet and you will be looking to fix it for good (given there's at least a way in which it can be considered fixed :) ).

I have been following all developments about the switching between the 2 ways (the issues reported, the final revert, etc.) and I must admin I'm afraid to upgrade my project from the Preact version (a quite old one now) I know the project works well with, because I'm afraid that this thing will bite me, on my code, or on any of the external dependencies I use (react mui for example, etc.).

Maybe the best option would be to make it as most as compatible with React as possible, at least for compatibility with their ecosystem, but I'm aware they use a custom event system, and so maybe they are not facing the issues you must handle instead?

[developit]

@krskrs React's implementation requires reimplementing events, which is a non-starter for Preact.

If you're on an old version (pre-10.9), 10.13 uses the exact same Promise-based batching that you're already using, so there's nothing scheduler-related in the upgrade that would impact your code or dependencies. We reverted in a minor because we were reverting back to the previous behavior.

There is still very much a desire to find an elegant solution to preventing re-renders during event bubbling, but we are searching for a solution that does not impact rendering performance or allow preemption. One possible solution we've discussed involves scanning up the DOM tree to check if a given Preact event handler is the last event of a given type that will be called before needing to render. Unfortunately, this would require custom logic to handle cases where multiple event types are grouped together (eg: onclick immediately triggering onchange).