Skip to content

fix: make fireEvent mouseEnter/mouseLeave work with addEventListener #588

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

Merged

Conversation

zbrogz
Copy link
Contributor

@zbrogz zbrogz commented Feb 12, 2020

(#577)

What:
Make fireEvent.mouseEnter and fireEvent.mouseLeave work with events added using addEventListener.

Why:
There is no way to trigger a native mouseEnter/mouseLeave event using React Testing Library. A workaround is to manually use dispatchEvent, but it's not as convenient as fireEvent. The reason for this limitation is due to how React handles mouseEnter/mouseLeave: facebook/react#12978

How:
Instead of setting fireEvent.mouseEnter/Leave to fireEvent.mouseOver/Out, set fireEvent.mouseEnter/Leave to call both mouseEnter/Leave and fireEvent.mouseOver/Out.

This makes the behavior match other events that can be called with fireEvent. For example, if one click handler was added to an element with addEventListener and another was added to the component directly with <button onClick={handleClick}>..., fireEvent.click(button) calls both click handlers.

Checklist:

  • Documentation added to the N/A
    docs site
  • Tests N/A
  • Typescript definitions updated N/A
  • Ready to be merged

Credit to @jessethomson

@zbrogz zbrogz changed the title fix: make fireEvent mouseEnter/mouseLeave work with addEventListener (#577) fix: make fireEvent mouseEnter/mouseLeave work with addEventListener Feb 12, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #588 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #588   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          95    103    +8     
  Branches       15     15           
=====================================
+ Hits           95    103    +8
Impacted Files Coverage Δ
src/pure.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24fc9b0...a40842d. Read the comment docs.

@zbrogz zbrogz requested a review from kentcdodds February 13, 2020 01:23
Copy link

@greypants greypants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks! Seems like the right solution.

Maybe add the tests for the specific issue this solves. At least the one that would have previously failed?

test('fires native mouseEnter/mouseLeave events', () => {...}

They were enumerated here: #577 (comment)

@weyert
Copy link
Contributor

weyert commented Feb 14, 2020

Cool, thanks, @zbrogz, well done :D

@zbrogz
Copy link
Contributor Author

zbrogz commented Feb 14, 2020

I'll update this with tests.

@kentcdodds
Copy link
Member

Thank you so much for this. I would love a test as well.

Is there a real world scenario where people would have to callbacks called unexpectedly?

@zbrogz
Copy link
Contributor Author

zbrogz commented Feb 14, 2020

I added tests to verify that all native events can be triggered using fireEvent. A couple caveats:

  1. Native onSelect isn't passing for some reason.
  2. onDoubleClick maps to dblclick.

If the scope of this is too big, I'll remove this and just write a test limited to onMouseEnter/onMouseLeave, like mentioned earlier.

Edit: I have a fix for native onSelect test not passing. It's similar to the onMouseLeave/Enter issue. Let me know if I should commit the change or if that should go in a different PR.

@zbrogz
Copy link
Contributor Author

zbrogz commented Feb 14, 2020

Is there a real world scenario where people would have two callbacks called unexpectedly?

I don't believe so. This makes it so fireEvent.onMouseLeave/Enter is consistent with the way other events work: both the native (added with addEventListener) and react (passed using onEventName prop) event handlers should get called.

@zbrogz
Copy link
Contributor Author

zbrogz commented Feb 21, 2020

@kentcdodds Do you mind taking a look at this again?

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm liking this. Just a few thoughts on how to change this slightly. Thanks!

@@ -128,6 +128,9 @@ const eventTypes = [
},
]

// native select event isn't passing
const nonNativeEvents = {doubleClick: 'dblclick', select: true}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should doubleclick even be in the events list?

Also, select probably doesn't work because JSDOM, which is unfortunate.

I probably wouldn't implement things this way and instead I would just have if statements in the test itself for each of these individually with comments explaining why these are special cases. No reason to have this indirection/abstraction.

Copy link
Contributor Author

@zbrogz zbrogz Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kentcdodds I made some changes based on your feedback.

instead I would just have if statements in the test itself for each of these individually with comments explaining why these are special cases

Done

Should doubleclick even be in the events list?

I think it makes sense to test both the doubleClick (synthetic) and dblclick (native) events.

Also, select probably doesn't work because JSDOM, which is unfortunate.

I included a fix for this in the latest commit.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Terrific 👍👍

@kentcdodds kentcdodds merged commit c5a7206 into testing-library:master Feb 22, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @zbrogz for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @zbrogz! 🎉

@kentcdodds
Copy link
Member

🎉 This PR is included in version 9.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants