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

click() doesn't quite work for div role="radio" #188

Closed
wKovacs64 opened this issue Nov 29, 2019 · 6 comments · Fixed by testing-library/react-testing-library#685 or #302
Closed
Labels
bug Something isn't working good first issue Good for newcomers released

Comments

@wKovacs64
Copy link

First, thanks for this library - great abstraction.

I just tried to use the userEvent.click() function on a radio button that is implemented as <div role="radio" /> (e.g. @palmerhq/radio-group) and it didn't quite yield the same expected behavior as directly calling fireEvent.click(). I see the click logic currently looks for <input type="radio" /> and calls clickBooleanElement, but this implementation of a radio button falls through to clickElement.

In the actual application, clicking said radio button moves the focus to an input field that becomes enabled when this radio button is selected. If I use fireEvent.click to select the radio button, asserting focus to be on the text input afterwards is successful. But using userEvent.click causes the focus assertion to fail.

The current published version of @testing-library/user-event results in the radio div retaining focus. I locally modified it to check for this type of element and call clickBooleanElement instead, but that doesn't seem to be quite right either. In that version, body ends up with focus.

Thoughts?

@Gpx Gpx added bug Something isn't working good first issue Good for newcomers help wanted labels Apr 15, 2020
@pranjaljately
Copy link

pranjaljately commented May 29, 2020

Hi @wKovacs64,

Happy to look into this.
Would it be possible for you to share the code so I can reproduce the issue?

Thanks,
Pranjal

@wKovacs64
Copy link
Author

Hey @pranjaljately, I've whipped up a quick reproduction CSB.

https://codesandbox.io/s/user-event-issue-188-j9brp?file=/src/__tests__/App.test.js

Thanks for taking a look!

@pranjaljately
Copy link

pranjaljately commented May 30, 2020

Thanks for CSB, @wKovacs64

So from understanding, because you are changing the focus from the radio button to the input, it is not assigned instantaneously therefore, you should wrap the expect inside a waitFor call:

  await waitFor(() => {
    expect(screen.getByLabelText(/enter/i)).toHaveFocus();
  })

fireEvent isn't exactly how the user interacts with the application (can read more about it here) whereas user-event tries to simulate the events that would happen in the browser as the user interacts with it.

Although I can see why having the test pass when using fireEvent and fail when userEvent can be confusing. Maybe we can get @kentcdodds's thoughts on this...

@wKovacs64
Copy link
Author

I guess my confusion then lies around why fireEvent would not require waiting, since the app is moving the focus in an effect with the value of the radio group as its dependency. I understand userEvent tries to simulate the events that would happen in the browser (that's precisely why I'm trying to use it), but fireEvent and userEvent both ultimately change the value, triggering the focus change effect. We're not waiting on something fireEvent or userEvent is doing, so their implementations shouldn't matter - we're waiting on an effect in the app. (At least, that's my understanding - must be incorrect?) Why wouldn't both require waitFor? 🤔

Regardless, wrapping the expectation in waitFor works and makes sense so I'm good to close this issue if you are. Thanks @pranjaljately!

@kentcdodds
Copy link
Member

I believe this actually is evidence of an issue we should address in user-event. We've discussed this in the past and I assumed that we didn't need to worry about wrapping userEvent in act, but based on this example, I think it's clear we need to figure out a way to automatically wrap userEvent in act.

https://codesandbox.io/s/user-event-issue-188-o0281?file=/src/__tests__/App.test.js:316-358

  act(() => { // <-- required for this to work
    user.click(custom)
  })

Any ideas on how to do this effectively would be welcome.

What I'm personally thinking is we could add a configuration option to @testing-library/dom for wrapping fireEvent in act just like we do for the async act stuff. https://github.com/testing-library/react-testing-library/blob/0afcbea3c3d1ddce218a36d963d39fa83f9a7cf6/src/pure.js#L12-L20

That way, if someone imported @testing-library/react, they would automatically configure @testing-library/dom to wrap fireEvent in act rather than having to do this fancy stuff.

To be clear, we wouldn't specifically include act in @testing-library/dom, but would instead enable people to do whatever they link whenever an event is fired.

I have a good idea of what this will be in my head and I'll probably work on it tomorrow morning.

kentcdodds added a commit to testing-library/dom-testing-library that referenced this issue Jun 1, 2020
This is intended for supporting `act` in React, but should be useful for
other frameworks (I think it could help with triggering change detection
for angular for example).

Ref: testing-library/user-event#188,
testing-library/user-event#255,
https://github.com/testing-library/user-event/issues/277
kentcdodds added a commit to testing-library/dom-testing-library that referenced this issue Jun 1, 2020
This is intended for supporting `act` in React, but should be useful for
other frameworks (I think it could help with triggering change detection
for angular for example).

Ref: testing-library/user-event#188,
testing-library/user-event#255,
https://github.com/testing-library/user-event/issues/277
kentcdodds added a commit to testing-library/react-testing-library that referenced this issue Jun 1, 2020
Now not only will React Testing Library's `fireEvent` be wrapped in
`act`, but so will DOM Testing Library's `fireEvent` (if
`@testing-library/react` is imported). It works very similar to async
act for the `asyncWrapper` config.

Closes: testing-library/user-event#188
Closes: testing-library/user-event#255
Reference: https://github.com/testing-library/user-event/issues/277
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 10.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers released
Projects
None yet
4 participants