Bug: Form reset lost checkbox onChange event

[imagine10255]

Hi, I use checkbox uncontrolled mode, onChange in form reset after, lose onChange.

<input type="checkbox" onChange={onChange} />

but use add ref.addEventListener('change', onChange) is ok

const checkRef = useRef<HTMLInputElement>();
useEffect(() => {
        if (checkboxRef) {
            checkboxRef.current.addEventListener('change', onChange);
        }
    }, []);

<input type="checkbox" ref="checkboxRef" onChange={onChange} />

React version: 16.13 and old

Steps To Reproduce

1. checkbox => checked
2. form reset
3. checked => checked

Link to code example: not react is ok reset is lose target onChange

The current behavior

1. checkbox => checked (target onChange)
2. form reset
3. checked => checked (lose target onChange)

The expected behavior

1. checkbox => checked (target onChange)
2. form reset
3. checked => checked (target onChange)

[Jacco]

Did some research:

I used even simpler code:

function onChange() {
  console.log('onChange');
}

function App() {
  return (
        <form>
          <input type="checkbox" onChange={onChange} />
          <button type="reset">Reset</button>
        </form>
  );
}

• It only happens when then checkbox is checked when you reset the form then check it again.
• It does NOT happen when the checkbox is unchecked when you reset the form then check it.

In the first case it happens when checking the second time because in updateValueIfChanged lastValue and nextValue are both true. So the function returns false and the onChange will be skipped.

The tracker on the node did not record the reset. I see that a reset event is fired for the target form but nothing seems to happen with that event. I do not see any code in react-dom that collect all inputs of a form to do something, so I am a bit reluctant to start adding something that big.

I would be interested in trying to make a fix for this.

[imagine10255]

@Jacco Hi, thank you for your feedback. Is there a plan to fix it? Let him keep the same with other inputs

[vinodsai-a]

Hi @bvaughn, Shall I work on this? (I'm new to opensource contribution)

[bvaughn]

If you'd like to, sure thing

[vinodsai-a]

Thanks @bvaughn . I'll try.

[imagine10255]

vinodsai-a notifications@github.com 於 2020年6月22日 週一 上午12:57寫道：

Thanks @bvaughn https://github.com/bvaughn . I'll try.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/19078#issuecomment-647153975, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMBAWLX7VZBOFR2JIAGDHLRXY3YTANCNFSM4NTLCGGQ .

[kassiomaia]

Did some research:

I used even simpler code:

function onChange() {
  console.log('onChange');
}

function App() {
  return (
        <form>
          <input type="checkbox" onChange={onChange} />
          <button type="reset">Reset</button>
        </form>
  );
}

• It only happens when then checkbox is checked when you reset the form then check it again.
• It does NOT happen when the checkbox is unchecked when you reset the form then check it.

In the first case it happens when checking the second time because in updateValueIfChanged lastValue and nextValue are both true. So the function returns false and the onChange will be skipped.

The tracker on the node did not record the reset. I see that a reset event is fired for the target form but nothing seems to happen with that event. I do not see any code in react-dom that collect all inputs of a form to do something, so I am a bit reluctant to start adding something that big.

I would be interested in trying to make a fix for this.

I wrote a pice of code but I'm not sure if it's on the right place, did this directly on react-dom.development.js.

@bvaughn, Is dispatchEvent the right place for this kind of actions?

function attemptToResetTrackers(nativeEvent) {
  var target = nativeEvent.target

  if (target && target.tagName === 'FORM') {
    for (var i = 0; i <= nativeEvent.target.length; i++) {
      var node = nativeEvent.target[i]

      if (node) {
        var tracker = getTracker(node)

        if (tracker) {
          tracker.setValue(node.defaultValue)
        }
      }

    }
  }
}

function dispatchEvent(topLevelType, eventSystemFlags, container, nativeEvent) {
  if (!_enabled) {
    return;
  }

  // Reset trackers when reset is called
  if (topLevelType === 'reset') {
    attemptToResetTrackers(nativeEvent)
  }

  if (hasQueuedDiscreteEvents() && isReplayableDiscreteEvent(topLevelType)) {
    // If we already have a queue of discrete events, and this is another discrete
    // event, then we can't dispatch it regardless of its target, since they
    // need to dispatch in order.
    queueDiscreteEvent(null, // Flags that we're not actually blocked on anything as far as we know.
    topLevelType, eventSystemFlags, container, nativeEvent);
    return;
  }

  var blockedOn = attemptToDispatchEvent(topLevelType, eventSystemFlags, container, nativeEvent);

  if (blockedOn === null) {
    // We successfully dispatched this event.
    clearIfContinuousEvent(topLevelType, nativeEvent);
    return;
  }

  if (isReplayableDiscreteEvent(topLevelType)) {
    // This this to be replayed later once the target is available.
    queueDiscreteEvent(blockedOn, topLevelType, eventSystemFlags, container, nativeEvent);
    return;
  }

  if (queueIfContinuousEvent(blockedOn, topLevelType, eventSystemFlags, container, nativeEvent)) {
    return;
  } // We need to clear only if we didn't queue because
  // queueing is accummulative.

  clearIfContinuousEvent(topLevelType, nativeEvent); // This is not replayable so we'll invoke it but without a target,
  // in case the event system needs to trace it.

  {
    dispatchEventForLegacyPluginEventSystem(topLevelType, eventSystemFlags, nativeEvent, null);
  }
} // Attempt dispatching an event. Returns a SuspenseInstance or Container if it's blocked.

[bvaughn]

I'm probably not the right person to review event stuff. Can you make the changes to the source files, then submit a PR?

[mvattuone]

There is a workaround that doesn't require a DOM event - basically, have a key for the checkbox that changes when onReset fires

Example below: https://codesandbox.io/p/sandbox/crimson-monad-39s3fw?file=%2Fsrc%2FApp.js%3A1%2C1-25%2C1