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

Prevent element to be clicked if pointer events are disabled for the element #631

Conversation

MohitPopli
Copy link
Contributor

What: Before firing click event on element, added check to validate if element is clickable.

Why: Though we trust browser to do it's job perfectly, but still if element have pointer-events disabled then it should not propagate any events or callbacks.

How: Created a utility function to check for pointer-events css value, if it's set to none then we should not click the element and return.

Checklist:

  • Documentation N/A
  • Tests
  • Typings N/A
  • Ready to be merged

Closes #630

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 28, 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 e9c3327:

Sandbox Source
userEvent-PR-template Configuration

@codecov
Copy link

codecov bot commented Mar 28, 2021

Codecov Report

Merging #631 (e9c3327) into master (f633a52) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #631   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           48        49    +1     
  Lines          891       911   +20     
  Branches       351       361   +10     
=========================================
+ Hits           891       911   +20     
Impacted Files Coverage Δ
src/click.ts 100.00% <100.00%> (ø)
src/hover.ts 100.00% <100.00%> (ø)
src/select-options.ts 100.00% <100.00%> (ø)
src/utils/misc/hasPointerEvents.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 f633a52...e9c3327. 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 a lot for taking the time to contribute ❤️

Could you rename the isElementClickable to hasPointerEvents and add it to dblClick, hover and unhover too?

We should also exclude the pointerEvents in selectOptionsBase.

@MohitPopli
Copy link
Contributor Author

Thanks a lot for taking the time to contribute ❤️

Could you rename the isElementClickable to hasPointerEvents and add it to dblClick, hover and unhover too?

We should also exclude the pointerEvents in selectOptionsBase.

Sure. I'll update PR with your suggestions

src/select-options.ts Outdated Show resolved Hide resolved
src/utils/misc/hasPointerEvents.ts Outdated Show resolved Hide resolved
Co-authored-by: Philipp Fritsche <ph.fritsche@gmail.com>
@MohitPopli
Copy link
Contributor Author

I'm not sure if I understood use case here,
https://github.com/testing-library/user-event/pull/631/files#diff-a681e050ffe760739876a36e05c9cfb1bb1ce0b6dae27c54152e16b9f6fac2f4R12

As once I merged this suggestion, my existing test cases started failing that I added to validate that we should return and should not fire any other events.

Please advise @ph-fritsche

src/utils/misc/hasPointerEvents.ts Outdated Show resolved Hide resolved
@ph-fritsche
Copy link
Member

Have a look at MohitPopli#1

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 now. Thanks a lot for your work on this 😃
I will have another look later and merge it if nothing leaps out at me then. 🚀

@ph-fritsche ph-fritsche merged commit 32e9712 into testing-library:master Mar 30, 2021
@github-actions
Copy link

🎉 This PR is included in version 13.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ph-fritsche
Copy link
Member

@all-contributors add @MohitPopli bug, code, test

@allcontributors
Copy link
Contributor

@ph-fritsche

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

@rbung
Copy link

rbung commented Apr 13, 2021

Hello,

I use reactstrap and test my components with RTL and userEvent and since this feature, I encounter an error when I want to test my dropdown. I made a codesandbox to reproduce my error : https://codesandbox.io/s/dropdown-userevent-mo7h1?file=/src/MyDropdown.spec.tsx

It looks like the click is not triggered on my menu item. To make it work, I found a workaround by setting the pointer-events style to auto. But maybe we should consider the element to have pointerEvents if this property is not set ?

@MohitPopli
Copy link
Contributor Author

@rbung I tried to triage the issue on my system and everything seems to work fine with me without having to set pointer-events property explicitly on any of the control.

I have used below queries used to click on dropdown and then menu item,

userEvent.click(screen.getByRole('button', { name: /dropdown/i })) // clicks on dropdown button

userEvent.click(screen.getByRole('menuitem', { name: /click me/i, hidden: false })) // clicks on menu item

Playground link: https://testing-playground.com/gist/1e9acc0b446eba5d32d611b0475997b4/bd9fec04e960bf99a17138efda8327c03003b149

Let me know if you have any questions.

@ph-fritsche
Copy link
Member

@rbung Your problem is described in #639 (comment)
Your menuitem button renders inside a parent with pointer-events: none. The component removes this style property later.

Update user-event to v13.1.3 and you can work around per:

await waitFor(() => userEvent.click(screen.getByRole("menuitem")))

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.

Clicking on elements having css property set as pointer-events to none still invoke onClick
3 participants