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

feat: focus trap on tab #193

Merged
merged 5 commits into from
Dec 17, 2019

Conversation

benmonro
Copy link
Member

@benmonro benmonro commented Dec 12, 2019

This feature allows you to easily test tabbing in focus traps.

example:

userEvent.tab({focusTrap: containerElement})

also adds tab to readme and converts expects to use jest dom for tabs

@benmonro benmonro requested a review from Gpx December 12, 2019 18:37
@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #193   +/-   ##
=======================================
  Coverage   97.05%   97.05%           
=======================================
  Files           1        1           
  Lines         136      136           
  Branches       35       35           
=======================================
  Hits          132      132           
  Misses          4        4
Impacted Files Coverage Δ
src/index.js 97.05% <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 9ab5b2b...9ea7015. Read the comment docs.

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.

Thanks! Looks good. Just wondering about the use case and implications.

Options:

- `shift` (default `false`) can be true or false to invert tab direction.
- `focusTrap` (default `document`) a container element to restrict the tabbing
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation here? Doesn't this create a test user scenario?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kentcdodds so let me start with the motivation for a focus trap option. We are using focus-trap-react which keeps tabbing confined within an element. Because jsdom doesn't have any way to deal with tabbing, these types of components won't work in jsdom.

So since jsdom doesn't seem to support tabs yet, focus trap basically won't work with userEvent.tab(). So does this introduce a test user? Maybe. But my hope is that jsDom can add support for tab and this entire feature can basically just adopt that. In the meantime this seems like the least bad option. That said if you have another way of writing a test that can let the user tab through a focus trap, I'd love to get some better ideas.

Here's the code for focus-trap which ftR uses under the hood.
https://github.com/davidtheclark/focus-trap/blob/master/index.js

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense. I would add a note about the original intent for the option and why it should be used sparingly.

Then you've got a 👍 from me :) Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do! Thx Kent!

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.

LGTM. As @Gpx is the primary maintainer on this repo, I'll wait for him to merge, but I'm good with these changes.

Thank you @benmonro. Pleasure as always :)

@benmonro
Copy link
Member Author

@Gpx any chance you can merge this? we could really use it. :)

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.

Looks like @Gpx is currently unavailable, so I'll go ahead and merge this. Thanks!

@afontcu
Copy link
Member

afontcu commented Dec 17, 2019

Vue tests are missing 🙈 no problem though, I guess I can work on them even after merging! 😄

@kentcdodds
Copy link
Member

I'm kinda thinking we should just ditch the framework specific stuff and use raw DOM stuff like in DOM Testing Library.

@kentcdodds kentcdodds merged commit bcb3c5c into testing-library:master Dec 17, 2019
@Gpx
Copy link
Member

Gpx commented Dec 17, 2019

🎉 This PR is included in version 7.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Gpx Gpx added the released label Dec 17, 2019
@calebeby
Copy link
Contributor

I'm kinda thinking we should just ditch the framework specific stuff and use raw DOM stuff like in DOM Testing Library.

Fully agree 👍

@afontcu
Copy link
Member

afontcu commented Dec 17, 2019

I'm kinda thinking we should just ditch the framework specific stuff and use raw DOM stuff like in DOM Testing Library.

Yeah, makes sense.

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.

5 participants