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: exclude hidden type inputs from focusable selector #467

Merged
merged 2 commits into from Oct 14, 2020
Merged

fix: exclude hidden type inputs from focusable selector #467

merged 2 commits into from Oct 14, 2020

Conversation

lazytype
Copy link
Contributor

fix #466

What: tab attempts to focus hidden type inputs

Why: Hidden-type input elements do not receive tab focus in common browsers, but tab fails to exclude them from its tab-able element list which produces incorrect behavior.

How: Update FOCUSABLE_SELECTOR to exclude hidden inputs.

Checklist:

  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #467 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #467   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          476       476           
  Branches       132       132           
=========================================
  Hits           476       476           
Impacted Files Coverage Δ
src/utils.js 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 ce2dbf6...2d5c362. 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.

Thank you for the report and quick fix. :)

src/__tests__/tab.js Outdated Show resolved Hide resolved
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! One thing.

@@ -219,7 +219,7 @@ function setSelectionRangeIfNecessary(
}

const FOCUSABLE_SELECTOR = [
'input:not([disabled])',
'input:not([type=hidden]):not([disabled])',
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the hidden attribute could be applied to any element. I think we need to add this to all of the selectors here.

Copy link
Member

Choose a reason for hiding this comment

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

I just checked in Chrome: <div tabIndex="0" type="hidden"> has no effect.
I don't think it is specified to have any meaning on other elements.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, duh. I was thinking about the hidden attribute itself (like <button hidden="true">).

But yeah, the hidden type is what we're dealing with here and that works fine. Thanks!

I wonder whether we should/need to fix how we handle the hidden attribute though 🤔

I'll merge this as-is. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Solid, thanks!

@kentcdodds kentcdodds merged commit de5db36 into testing-library:master Oct 14, 2020
@kentcdodds
Copy link
Member

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

@allcontributors
Copy link
Contributor

@kentcdodds

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

@testing-library-bot
Copy link

🎉 This PR is included in version 12.1.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

jesujcastillom pushed a commit to jesujcastillom/user-event that referenced this pull request Oct 23, 2020
…ary#467)

* fix(tab): exclude hidden type inputs from focusable selector

* make test leaner

Co-authored-by: Michael Mitchell <michael.mitchell@mongodb.com>
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.

userEvent.tab tries to focus <input type="hidden" />, preventing active element from changing
4 participants