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

fireEvent.click() performs click on disabled input field #92

Closed
prasann opened this issue Aug 25, 2018 · 20 comments
Closed

fireEvent.click() performs click on disabled input field #92

prasann opened this issue Aug 25, 2018 · 20 comments
Labels
jsdom Issue with JSDOM environment

Comments

@prasann
Copy link

prasann commented Aug 25, 2018

  • dom-testing-library version: 3.4.0
  • react version: -NA-
  • node version: 9.11.1
  • npm (or yarn) version: -NA-

Relevant code or config:

fireEvent.click() on disabled elements.

What you did:

I was expecting fireEvent.click() not to have dispatched click event on disabled elements.

What happened:

fireEvent is ignoring the disabled attribute on the element.

Reproduction:

The following test case events.js is failing.

describe('Disabled element', () => {
  it(`should not fire any event on disabled element`, () => {
    const node = document.createElement('input')
    const disabledAttr = document.createAttribute('disabled')
    node.setAttributeNode(disabledAttr);
    const spy = jest.fn()
    node.addEventListener('click', spy)
    fireEvent.click(node)
    expect(spy).toHaveBeenCalledTimes(0)
  })
});

Problem description:

Currently, the fireEvent is ignoring the disabled attribute in elements.

Suggested solution:

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.

The HTML Spec

fireEvent can check whether the element has disabled attribute and skip dispatching the event.

However I'm not sure whether to ignore all MouseEvents (like Chrome/Safari) or just ignore click events.

@prasann prasann changed the title fireEvent.click() performs click on disabled input field fireEvent.click() performs click on disabled input field Aug 25, 2018
@kentcdodds
Copy link
Member

Interesting. I wonder what the browser does and if this may be a bug in jsdom 🤔

@kentcdodds
Copy link
Member

Can you try to run your code in the browser and see if dispatching a click event on a disabled button in the browser will call the click handler or not?

@prasann
Copy link
Author

prasann commented Aug 25, 2018

a quick scan in jsdom code, shows that they are handling this scenario here.

Now I'm puzzled why fireEvent went through on a disabled input in my case.

In browser (chrome, safari and ff) the click is not calling the handler.

@prasann
Copy link
Author

prasann commented Aug 25, 2018

Here is the code that I was trying. After spending some time now, I'm unclear about where the problem is.

@kentcdodds
Copy link
Member

Huh, weird. In addition, in codesandbox it's running in the browser rather than using jsdom... Hmm...

alexkrolick pushed a commit to alexkrolick/dom-testing-library that referenced this issue Sep 13, 2018
* docs: update examples to be more atomic

* docs: add SavePointSam to contributors list
@kentcdodds
Copy link
Member

If anyone can come up with what's going on here and a solution to the problem then please write here and we can re-open, but I'm going to go ahead and close this for now.

@ngtan
Copy link

ngtan commented Mar 27, 2019

I got the same problem.

const wrapper = render(<DumbComponent />);
const submitButtonElm = wrapper.getByTestId('submit-btn');
const handleClick = jest.fn();

console.log(submitButtonElm.disabled); // true
submitButtonElm.addEventListener('click', handleClick, false);
fireEvent.click(submitButtonElm);

expect(handleClick).not.toHaveBeenCalled();

I tried to run my code in the browser, the event handler is not called.

@kentcdodds
Copy link
Member

Can you please make a codesandbox of that?

@ngtan
Copy link

ngtan commented Mar 27, 2019

@kentcdodds Thanks for your quickly response.
Here is the code I don't know why it works well on codesandbox. I have the same code as on the codesandbox, but it's failed (the same react-testing-library@6.0.0 version). So weird.

@ngtan
Copy link

ngtan commented Mar 27, 2019

I have just pushed the code on the repo. Please take a few minutes to check it out. The same code on the Codesanbox, but it's failed.

@Irrelon
Copy link

Irrelon commented Jul 17, 2019

@ngtan Updated your codesandbox https://codesandbox.io/s/naughty-smoke-ckqh6?fontsize=14

Removed the index.js and just used a pure test.js so it doesn't try to locate and render to a DOM element that doesn't exist.

The test shows the problem exists and disabled inputs can and will receive a click event!

@Jessidhia
Copy link

Jessidhia commented Sep 11, 2019

I have encountered a form of this and almost thought it was a bug in jsdom, but it was here all along 😇

The trick here is that real user interaction clicks are required to always go into the user interaction task source first, and then it gets completely discarded if it turns out the target is a disabled form element; there's not even a capturing phase. Direct calls to .click() also respect the disabled property. However, .dispatchEvent() doesn't check it -- it never is supposed to be called to begin with!

The problem, though, is that there's no "user interaction task source" in a headless library like JSDOM. Perhaps it's something that can be done at the fireEvent / createEvent level by abusing the isTrusted flag.

JSDOM itself ignores it, but an option is to have createEvent (optionally?) set isTrusted to true ({ isTrusted: true } as the 2nd argument of new MouseEvent, but that's an implementation detail), and then fireEvent could check the target of isTrusted events for whether they are disabled form elements and drop the dispatchEvent if event.isTrusted && node.disabled. Or maybe testing-library's own symbol/weakmap to keep track of which event instances to treat as interactions and which events to treat as plain dispatches.

@kentcdodds
Copy link
Member

Thanks for your investigation @Jessidhia!

I'm hesitant to do much more in fireEvent than it's already doing, but this does violate the principle of least surprised (people are more surprised that does call the click handler on a disabled input than if it were to not call the handler). So I may be interested in a solution here.

Another option is to just encode this logic in user-event and recommend people use that for clicks 🤔

@kentcdodds kentcdodds reopened this Sep 11, 2019
@alexkrolick
Copy link
Collaborator

Another option is to just encode this logic in user-event and recommend people use that for clicks 🤔

Leaning towards that because of all the other focus handling and stuff that's enabled there

@kentcdodds
Copy link
Member

I'd like to hear more about why people want to write a text that clicks on a disabled input. What are you trying to test in this case? Is it not enough to simply assert that the input is disabled and trust the browser to do its job with that? I kinda think that trying to click it and asserting that the handler isn't called is basically testing the browser, not your code 🤔

@alexkrolick
Copy link
Collaborator

I suppose the disabled state could be considered an implementation detail - for example you could just remap the click handler to a no-op function, and unless there are significant accessibility issues with that, it's a valid approach.

@kentcdodds
Copy link
Member

unless there are significant accessibility issues with that

I believe there are. And if you remap the click handler to a no-op function then you actually do want the click event to call it so you can test that :)

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2019

element.click() will not execute the click handler if the element is disabled in a jsdom environment. dispatchEvent will ignore disabled state. The former should be a sufficient workaround right now. The latter is filed against jsdom in jsdom/jsdom#2665.

I'll check if this works if you use testing-library in a real browser. It should work as expected though. We can probably close this issue and fix it upstream.

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2019

Confirmed that a browser ignores click() as well as fireEvent.click on disabled elements: https://codesandbox.io/s/testing-library-disabled-input-ignores-click-owohf (Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.75 Safari/537.36)

This must be handled by the environment. If you want to follow progress on this bug subscribe to jsdom/jsdom#2665.

@jooeunlee-st
Copy link

I'd like to hear more about why people want to write a text that clicks on a disabled input. What are you trying to test in this case? Is it not enough to simply assert that the input is disabled and trust the browser to do its job with that? I kinda think that trying to click it and asserting that the handler isn't called is basically testing the browser, not your code 🤔

I have a case where I want to prevent rage-clicking on a button that causes side-effects. Wouldn't this be a case of "The more your tests resemble the way your software is used, the more confidence they can give you.", too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jsdom Issue with JSDOM environment
Projects
None yet
Development

No branches or pull requests

9 participants