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

Support 'Enter' key press on links #534

Merged
merged 4 commits into from
Feb 13, 2021

Conversation

benadamstyles
Copy link
Contributor

@benadamstyles benadamstyles commented Dec 28, 2020

What: in handleEnter, there is currently a check for whether the element is clickable, and if it is, it fires a click event on that element. This PR includes anchor tags (with truthy href values) in that check.

Why: anchor tags are incorrectly missed out of this logic – browsers fire click events on anchor tags when they are focused and the user presses the Enter key.

How: included anchor tags in the isClickable util function.

Checklist:

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

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #534 (14c5f71) into master (217b487) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #534   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          696       699    +3     
  Branches       218       221    +3     
=========================================
+ Hits           696       699    +3     
Impacted Files Coverage Δ
src/type.js 100.00% <100.00%> (ø)
src/utils.js 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 217b487...6109baf. Read the comment docs.

@ph-fritsche
Copy link
Member

isClickable is used by handleSpace too, but space does not trigger a click on Anchor elements in the browser.
This would introduce a bug there.

I suggest to edit this instead of the util:

if (isClickable(currentElement())) {

if (isClickable(currentElement()) || isInstanceofElement(currentElement(), 'HTMLAnchorElement')) {

(Maybe slap some comment on isClickable (or even change the name) that it only covers clickable inputs.)

@benadamstyles
Copy link
Contributor Author

@ph-fritsche Thanks for the feedback! I think I've addressed everything.

src/type.js Outdated Show resolved Hide resolved
@ph-fritsche ph-fritsche merged commit 8a34d0b into testing-library:master Feb 13, 2021
@ph-fritsche
Copy link
Member

@all-contributors add @benadamstyles for code, tests

@allcontributors
Copy link
Contributor

@ph-fritsche

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

@github-actions
Copy link

🎉 This PR is included in version 12.7.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.

2 participants