Preact: Extra event incorrectly triggered

[maltium]

Clicking on _Button 1_ causes both _onButton1Click()_ and _onButton2Click()_ to be triggered. Only _onButton1Click()_ should be called if _Button 1_ is clicked. Interestingly, setting the _key_ on the div containing _Button 2_ prevents this issue from happening.

Same code works as expected with React and testing indicates that the issue was introduced in Preact 5.x.

Here's a demo of the issue on jsfiddle.

import { Component, render } from "preact";

class App extends Component {
  constructor(props) {
    super();

    this.state = {
      showSection: true
    };
  }

  render() {
    const section = (
      <div>
        <div />
        <div>
          <div onClick={this.onButton1Click}>Button 1</div>
        </div>
      </div>
    );

    return (
      <div>
        {this.state.showSection && section}
        <div>
          <div>
            <div />
          </div>
          <div onClick={this.onButton2Click}>Button 2</div>
        </div>
      </div>
    );
  }

  onButton1Click = () => {
    console.log("button 1 click");
    this.setState({ showSection: false });
  };

  onButton2Click = () => {
    console.log("button 2 click");
  };
}

render(<App />, document.getElementById("root"));

[kidqueb]

No explanation behind it, but just another clue:

https://jsfiddle.net/c93jsm62/

Making each div with onClick a button or a actually fixes it

[maltium]

Yes, changing the DOM structure resolves the issue.

Looks like a recycling issue to me.

[developit]

I haven't fully finished looking into this, but if it's recycling-related, there's no recycling whatsoever in the upcoming major update. Let's hope this goes away then!

[marvinhagemeister]

This issue seems to be caused by the recycler that was present in the 8.x release line. We completely removed the recycler for our next version and bugs like this should be a thing of the past :+1:

[maltium]

Tested this on Preact 10 alpha0, and it's still happening.

https://codesandbox.io/s/vj1p70kz4y

[developit]

This is caused by microtasks being flushed between event propagation.
My understanding of the flow:

1. click event is dispatched at target Button 1
2. click handler is invoked on Button 1
3. setState() in handler enqueues rendering, which is implemented as a Promise.then()
4. click handler returns, and event propagation kicks in
5. event propagation flushes Promise tasks between each Node, so the enqueued render happens synchronously before propagation can continue
6. the synchronous render mutates the DOM tree in-place, resulting in Button 1 _becoming_ Button 2, with the new event handler now set as a click listener.
7. rendering completes, and propagation picks up, moving to the next event handler which is now the newly-installed one.

[JoviDeCroock]

@developit I have been reasoning about this quite a while, don't we also impose setTimeout in our hooks flushing? Losing Promise.resolve for setTimeout would allow these two more or am I mistaking here?

[developit]

@JoviDeCroock IIRC setTimeout is used to obtain an "after frame callback". It's possible we'd want to keep an eye on that, and if folks are hitting timer clamping we could switch the hook implementation back to MessageChannel.

[JoviDeCroock]

This seems to be resolved in the beta.0 release, if this pops up again feel free to reopen this issue.