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

fix(click): throw an error when trying to click with pointer-events set to none #647

Merged
merged 5 commits into from
Apr 14, 2021
Merged

fix(click): throw an error when trying to click with pointer-events set to none #647

merged 5 commits into from
Apr 14, 2021

Conversation

vicrep
Copy link
Contributor

@vicrep vicrep commented Apr 12, 2021

What:

As discussed here: #639 (comment) , the new behaviour which was recently added to prevent clicks from being emitted when pointer-events are set to none had the side-effect of silently no-oping when this was the case, making it:

  • non-trivial to figure out why your next assertion failed, given the click 'executed' in the PoV of the test
  • hard to work around libraries which set these pointer events while animating

I've added this behaviour to both the click and double-click.

Why:

This change follows the paradigm that testing-library provides for selectors, where getBy will throw an error if the element doesn't exist. Similarly, trying to click something which is not clickable should also throw an error, as this is an action which cannot be performed by a real user. Some other testing frameworks, such as Cypress, follow similar behaviour of failing when trying to click non-clickable elements.

This also makes dealing with animating elements which might set their pointer events to none while animating way more straightforward, using the waitFor utility:

await waitFor(() => userEvent.click(screen.getByText("OK")))

Checklist:

  • Documentation
  • Tests
  • Typings
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 12, 2021

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.

Latest deployment of this branch, based on commit 3ae2693:

Sandbox Source
userEvent-PR-template Configuration

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #647 (3ae2693) into master (0537940) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #647   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           50        50           
  Lines          918       922    +4     
  Branches       363       363           
=========================================
+ Hits           918       922    +4     
Impacted Files Coverage Δ
src/click.ts 100.00% <100.00%> (ø)
src/hover.ts 100.00% <100.00%> (ø)

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 0537940...3ae2693. Read the comment docs.

Copy link
Member

@ph-fritsche ph-fritsche left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. ❤️

README.md Outdated Show resolved Hide resolved
src/click.ts Outdated Show resolved Hide resolved
src/click.ts Outdated Show resolved Hide resolved
@ph-fritsche
Copy link
Member

ph-fritsche commented Apr 13, 2021

We should probably throw the same error on userEvent.hover and userEvent.unhover

Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
@vicrep
Copy link
Contributor Author

vicrep commented Apr 13, 2021

@ph-fritsche I actually considered doing it for hover too, but wasn't sure if out of scope for this PR. Given that hovers usually provide side-effects and aren't usually explicit actions, I'm not sure if we should error when hovering over something that's disabled.

FWIW, I find that Cypress has very sensible assertions they make on their commands to simulate user behaviour (such as, failing when trying to click a non-clickable element) and this is the inspiration I used for this PR. Interestingly, hover is basically the only user 'event' they don't provide a command for, due to how non-deterministic its behaviour can be depending on implementation. This further added to my uncertainty about adding this to the PR.

What do you think? I'd be happy to add it to this PR, or to propose it as a future (or separate) improvement.

@vicrep vicrep requested a review from ph-fritsche April 13, 2021 14:32
@ph-fritsche
Copy link
Member

@vicrep I think users calling userEvent.hover expect a mouseover to be dispatched. So the same logic as for the click applies. We don't have layout in jsdom so we could not determine the elements the pointer events would fall through to.

@vicrep
Copy link
Contributor Author

vicrep commented Apr 14, 2021

@ph-fritsche makes sense, I've added the same behaviour to hover/un-hover 👍

Please let me know if there's anything else you need from me to merge this :)

Copy link
Member

@ph-fritsche ph-fritsche left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@ph-fritsche ph-fritsche merged commit 6b2ce66 into testing-library:master Apr 14, 2021
@github-actions
Copy link

🎉 This PR is included in version 13.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vicrep vicrep deleted the fix/throw-when-clicking-unclickable-element branch April 14, 2021 22:31
@ph-fritsche
Copy link
Member

@all-contributors add @vicrep code

@allcontributors
Copy link
Contributor

@ph-fritsche

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

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.

None yet

2 participants