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.preventDefault in click handler does not prevent onChange from being called #9023

Open
aweary opened this issue Feb 17, 2017 · 7 comments

Comments

@aweary
Copy link
Contributor

aweary commented Feb 17, 2017

Do you want to request a feature or report a bug?

Bug!

What is the current behavior?

When rendering an input element of type checkbox with an onClick and onChange handler, onChange is still called even though event.preventDefault() is called in the onClick handler.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

http://jsfiddle.net/rf3w7apc/

What is the expected behavior?

Calling event.preventDefault in the onClick handler should prevent the default action from occurring (or undo its effect), which is to update the value of the input element. This should stop any change event listener from being invoked. See https://jsfiddle.net/L1eskzsq/ for expected behavior

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Tested using a build from master, macOS 10.12.2, verified in:

  • Chrome 56.0.2924.87 (64-bit)
  • Firefox 51.0.1 (64-bit)

Safari 10.0.2 calls the change event listener in both cases.

@hejld
Copy link
Contributor

hejld commented Feb 18, 2017

The change event is being created by the ChangeEventPlugin as a response to topChange, before your onClick handler is even called (so before the preventDefault call), here: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L338

This plugin is trying to figure out if the value of the checkbox will change, and if yes it will queue the change event. It uses the inputValueTracking functionality to compare the previous value with the next value to make this decision, here: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/inputValueTracking.js#L154

The nextValue is take directly from the DOM like this: value = isCheckable(node) ? '' + node.checked : node.value; https://github.com/facebook/react/blob/master/src/renderers/dom/shared/inputValueTracking.js#L56

The problem is that the node.checked value is temporarily true at this point, so after the first click it is:

lastValue == false (correct)
nextValue == true (incorrect)

and on the next click on the checkbox, the change event is no longer being fired, because the values are:

lastValue == true (incorrect)
nextValue == true (incorrect)

Overall the problem seems to be that React is asking the DOM what the checked value of the node is, before the preventDefault in onClick could be called.

Thanks @karelskopek and @Mintaffee for your help with digging into this.

@hejld
Copy link
Contributor

hejld commented Mar 12, 2017

@aweary Do you think this is worth solving?

@powah
Copy link

powah commented Mar 21, 2018

No progress with this one? I'm using React v16.2 and the behaviour is the same - onChange is triggered first time and subsequent clicks doesn't trigger it. I would say quite high priority issue here..

@megamaddu
Copy link

Is this related? preventDefault called in a checkbox onChange causes React and the browser state to go out of sync: https://codesandbox.io/s/0qpr936xkv

@ezze
Copy link

ezze commented Apr 19, 2019

Still an issue in React 16.8.6. One possible workaround is to do everything in onClick handler:

const radios = ['one', 'two', 'three'];

class Row extends Component {
  constructor(props) {
    super(props);
    this.onClick = this.onClick.bind(this);
    this.onChange = this.onChange.bind(this);
  }

  render() {
    const { value } = this.props;
    return (
      <div className="row">
        <div className="radio-group">
          {radios.map(radio => (
            <label key={radio} className="radio">
              <input
                type="radio"
                name="radio"
                value={radio}
                checked={radio === value}
                onClick={this.onClick}
                onChange={this.onChange}
              /><span>Radio</span>
            </label>
          ))}
        </div>
      </div>
    );
  }

  onClick(event) {
    // Check something to prevent radio change
    if (...) {
      event.preventDefault();
      return;
    }
    // Sync value in the store
    this.props.setValue(event.target.value);
  }
  onChange() {
    // We need this empty handler to suppress React warning:
    // "You provided a `checked` prop to a form field without an `onChange` handler.
    // This will render a read-only field. If the field should be mutable use `defaultChecked`.
    // Otherwise, set either `onChange` or `readOnly`"
  }
}

If you use controlled component another option I found is to simply leave value in the state unchanged:

onChange(event) {
  // Check something to prevent radio change
  if (...) {
    return;
  }
  // Sync value in the store
  this.props.setDateTime(event.target.value);
}

@ronn9419
Copy link

For a simpler solution, you can do e.preventDefault on onMouseDown event to prevent onChange from firing.

onMouseDown(e) { if (...) { e.preventDefault(); return; } }

@eps1lon
Copy link
Collaborator

eps1lon commented Nov 30, 2020

WARNING: Only tested in React 17 + Chrome 87

As a workaround you can wrap the built-in input component with a custom one that checks for event.nativeEvent.defaultPrevented like so:

function Checkbox(props) {
  const { onChange, ...other } = props;

  function handleChange(event) {
    if (event.nativeEvent.defaultPrevented === false) {
      onChange(event);
    }
  }

  return <input {...other} onChange={handleChange} type="checkbox" />;
}

-- https://codesandbox.io/s/checkbox-click-preventdefault-change-quirk-usljx

Original source: mui/material-ui#23709 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants