onChange is called when checkbox is disabled

[rug1]

Hi 👋 Thanks for the great library! Think I may have found a bug but if not I would love to know what I'm doing wrong here.

• react-testing-library version: 5.4.4
• react version: 16.7.0
• node version: 10.15.0
• npm (or yarn) version: yarn 1.10.1

Relevant code or config:

test('disabled checkbox cannot be selected', () => {
  const onChange = jest.fn()
  const {getByTestId} = render(
    <input
      type="checkbox"
      onChange={onChange}
      data-testid="checkbox"
      disabled
    />,
  )
  const checkbox = getByTestId('checkbox')
  fireEvent.click(checkbox)
  expect(onChange).not.toHaveBeenCalled() // this fails
})

What you did:

I wrote a test to check that a checkbox onChange function would not be called if the checkbox is disabled.

What happened:

The test failed saying that the onChange function had been called.

Reproduction:

https://codesandbox.io/s/91684p354

Problem description:

The test should pass. onChange functions should not be called in the test if the checkbox is disabled.

Suggested solution:

Not sure, haven't looked into the code yet.

[kentcdodds]

Confirmed! This is pretty odd! I'm not sure what it could be. Anyone wanna dig into this one? Pretty good learning opportunity here :)

[puemos]

short answer

You should use onClick listener for checkbox inputs

example

https://codesandbox.io/s/j22y6pl21y

source

https://github.com/facebook/react/blob/c954efa70f44a44be9c33c60c57f87bea6f40a10/packages/react-dom/src/events/ChangeEventPlugin.js#L211

/**
 * SECTION: handle `click` event
 */
function shouldUseClickEvent(elem) {
  // Use the `click` event to detect changes to checkbox and radio inputs.
  // This approach works across all browsers, whereas `change` does not fire
  // until `blur` in IE8.
  var nodeName = elem.nodeName;
  return nodeName && nodeName.toLowerCase() === 'input' && (elem.type === 'checkbox' || elem.type === 'radio');
}

[kentcdodds]

Thanks @puemos, but the fact is that the tests function differently from when you do this in an app. You definitely should be able to use onChange like this and fire a click event. Something else is up 🤔

[lourenci]

I don't know if this is a problem, but either for disabled or disabled={true}, render.debug() shows me <input disabled="" />. If I create a component in React in this way, the input is not disabled, so you can click on it.

Could it be a problem with the jsdom?

[Tolsee]

@kentcdodds My feeling is there is a problem with firEvent of dom-testing-library somehow. Did not get the problem when a click is triggered in an element is self. Can you suggest me the best way to figure out the problem?

https://codesandbox.io/embed/yp996rqlyj

[kentcdodds]

This is interesting. I created a vanilla JavaScript version of this:

https://codesandbox.io/s/q70qj31m46

const app = document.getElementById('app')

app.innerHTML = ''

const input = document.createElement('input')
input.setAttribute('type', 'checkbox')
input.setAttribute('disabled', 'true')
let clickCalled = false
input.addEventListener('click', () => {
  clickCalled = true
})
let changeCalled = false
input.addEventListener('change', () => {
  changeCalled = true
})
let inputCalled = false
input.addEventListener('input', () => {
  inputCalled = true
})

app.appendChild(input)

// this is basically what fireEvent.click does
const event = new MouseEvent('click', {
  bubbles: true,
  cancelable: true,
  button: 0,
})
input.dispatchEvent(event)

The result is that the input and change handlers are called, but the click handler is not called. And the input actually gets checked despite being disabled.

I'm guessing this has something to do with a strange specification or something. If someone wants to investigate this further then I'd welcome help there...

I'm afraid I don't have time to dedicate to this problem at the moment.

[ken-kenware]

It looks like this may be a case where the spec was followed a little too closely. According to the Spec:

A form control that is disabled must prevent any click events that are queued on the user interaction task source from being dispatched on the element.

In the vanilla JS example, we can see that it is in fact not dispatching the click event, but dispatching the remaining events.

My hypothesis is that browsers are doing some special handling for dispatching click events as opposed to other events in order to control ad clicking (I may be totally off basis here though I'm no expert). This is based entirely off of this method:
https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/core/dom/events/event_dispatcher.cc#L77

Which has this line that immediately returns if it's a form element that is disabled:
https://github.com/chromium/chromium/blob/master/third_party/blink/renderer/core/dom/events/event_dispatcher.cc#L90

So yeah...my guess is that the right way to handle this is to make sure we are more closely simulating what the browser appears to be doing, and swallowing the click event if the element is disabled.

[kentcdodds]

But that would be a change for react right? Not react-testing-library

[ken-kenware]

I don't think so. Since React is just proxying the event, they don't do anything around calling dispatchEvent to my knowledge.

My guess is that we need to do something here: https://github.com/kentcdodds/dom-testing-library/blob/master/src/events.js#L310

To account for the click event on ignored elements, and just swallow the event. This will mimic what the browser is doing under the hood (and I'm guessing the feature we would get if we could actually click a button rather than just dispatching the click event).

Thoughts?

[kentcdodds]

I think it will make more sense when I see a pull request. Anyone willing to open that up?

[ken-kenware]

I'll work on it!

…

On Tue, Feb 19, 2019, 2:37 PM Kent C. Dodds ***@***.***> wrote: I think it will make more sense when I see a pull request. Anyone willing to open that up? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADw5WQaWqaXZsNbMRT4HlCr3AM3Z_a7Uks5vPGCJgaJpZM4aPLjm> .

[ken-kenware]

PR:
testing-library/dom-testing-library#217