Bug: onChange handler is lost between re-renders

[subha84]

React version: 18 In our application, we have a checkbox and a button on a page. The button opens a popup which in turn attaches a onClick event of type capture at body level. Now, when we click on the checkbox we observed the onChange handler never gets a chance to execute.

Steps To Reproduce

1. Click on the button to open a popup.
2. popup attaches a click handler at body level. This event is of type 'capture' to close the popup whenever some random click happens anywhere on the page.
3. Now, if we click on the checkbox the body level click event is executed before the checkbox's onChange event.
4. This click event in turn updates some state, which causes re-render of the checkbox component.
5. popup is closed.
6. The onChange event attached with the checkbox never got a chance to be executed. It is lost somewhere.

Link to code example:

The current behavior

The checkbox remains either checked or unchecked.

The expected behavior

The checkbox should get checked and unchecked with every other click.

[umerprogramm]

Hey @subha84 I'm interested to work on it So right now, you are working on it or not?

[subha84]

Yes, right now working on it. @umerprogramm

[subha84]

@eps1lon @gaearon Any ideas ?

[LuBustos]

Hey @subha84! I saw you error in the codesanbox if you remove the third option in the event listener method, it seems to work. The useEffect should look like this:

  useEffect(() => {
    let onclick = function (event) {
      setRandom(Math.floor(Math.random() * 10));
    };
    document.addEventListener("click", onclick);
    return function () {
      document.removeEventListener("click", onclick);
    };
  }, []);

[riamahajan08]

Hey @subha84! I saw you error in the codesanbox if you remove the third option in the event listener method, it seems to work. The useEffect should look like this:

  useEffect(() => {
    let onclick = function (event) {
      setRandom(Math.floor(Math.random() * 10));
    };
    document.addEventListener("click", onclick);
    return function () {
      document.removeEventListener("click", onclick);
    };
  }, []);

@LuBustos What would be an alternative if the event needs to be processed in the capture phase?

[ujjawalD07]

Is anyone working on this issue? I am new to open source contribution and I want to give this a go.

[gaearon]

I don't see any button in the reproducing case. How do I follow these instructions?

[gaearon]

OK, so the button in the description seems irrelevant. The issue I'm seeing is that commenting out setRandom call makes the checkbox work. So the setRandom call somehow breaks the checkbox.

I think this looks like a bug, though I'm not sure if it's fixable. I assume what happens here is:

• Suppose controlled checked is false in the beginning.
• When you click, the document capture level event fires.
• The document capture level event handler updates the random state, re-rendering the component with checked={false}. This updates the controlled checkbox to have node.checked = false, even though it was true just before.
• By the time React handles the checkbox change event, node.checked is false. So setChecked(false) keeps it unchecked.

Intuitively I'd expect that the issue is with the bold step — I think a re-render shouldn't have forced the controlled value to change if we haven't had a chance to run React handlers yet. But I'm not sure if this is something we can fix. Someone would need to dig deeper into this.

Is anyone working on this issue? I am new to open source contribution and I want to give this a go.

You're always welcome to try — note this might be a difficult one since you need to figure out how controlled inputs are implemented, and which assumption is being broken here.

[subha84]

OK, so the button in the description seems irrelevant. The issue I'm seeing is that commenting out setRandom call makes the checkbox work. So the setRandom call somehow breaks the checkbox.

I think this looks like a bug, though I'm not sure if it's fixable. I assume what happens here is:

• Suppose controlled checked is false in the beginning.
• When you click, the document capture level event fires.
• The document capture level event handler updates the random state, re-rendering the component with checked={false}. This updates the controlled checkbox to have node.checked = false, even though it was true just before.
• By the time React handles the checkbox change event, node.checked is false. So setChecked(false) keeps it unchecked.

Intuitively I'd expect that the issue is with the bold step — I think a re-render shouldn't have forced the controlled value to change if we haven't had a chance to run React handlers yet. But I'm not sure if this is something we can fix. Someone would need to dig deeper into this.

Is anyone working on this issue? I am new to open source contribution and I want to give this a go.

You're always welcome to try — note this might be a difficult one since you need to figure out how controlled inputs are implemented, and which assumption is being broken here.

@gaearon exactly, setRandom is just simulate the use case to re-render the checkbox before the 'onChange' handler gets a chance to execute. We debugged into React code and we found the same reason you stated above, If we stop the re-render thinks work. We still haven't figured out the way to fix it considering the way the whole event system is designed. @riamahajan08

[smchinna]

@gaearon and @subha84 I have a few thoughts on this

• Add Event listener function executed on capturing phase (document → html → body → parent → child) because of the third params passed as true on the event listener function - it won't wait to execute state updates.
• It can resolve if executed in the bubbling phase. like document.addEventListener("click", onclick, false);

[ayomidebajo]

I think the problem is from the code itself. onclick function runs perfectly fine for the addEventListener method. You don't need to return an extra function

 useEffect(() => {
    let onclick = function (event) {
      setRandom(Math.floor(Math.random() * 10));
      // or here
      //  document.addEventListener("click", onclick, true);
    };
     document.addEventListener("click", onclick, true);
    document.removeEventListener("click", onclick, true)
  }, []);

[subha84]

I think the problem is from the code itself. onclick function runs perfectly fine for the addEventListener method. You don't need to return an extra function

 useEffect(() => {
    let onclick = function (event) {
      setRandom(Math.floor(Math.random() * 10));
      // or here
      //  document.addEventListener("click", onclick, true);
    };
     document.addEventListener("click", onclick, true);
    document.removeEventListener("click", onclick, true)
  }, []);

We are returning a function to stop event leakage. @ayomidebajo

[ayomidebajo]

Please explain how there will be an event leakage.

The dom Eventlistener is already handling the onclick function in its scope, recall that the dom event listener actually consumes the function, so it handles all the clearing up and there's no need to return an extra onclick function since you add the event listener directly into the dom without using react's attributes for event listeners.

Please try the code below and test whether it works.

  useEffect(() => {
    let onclick = function (event) {
      setRandom(Math.floor(Math.random() * 10));
      setCheck(!check);
      // or here
      //  document.addEventListener("click", onclick, true);
    };
    document.addEventListener("click", onclick, true);
    document.removeEventListener("click", onclick, true);
  }, [check]);