-
Notifications
You must be signed in to change notification settings - Fork 244
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
fix: exclude hidden type inputs from focusable selector #467
Conversation
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 12
Lines 476 476
Branches 132 132
=========================================
Hits 476 476
Continue to review full report at Codecov.
|
There was a problem hiding this 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. :)
There was a problem hiding this 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])', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lazytype, I mean, I'm happy to merge after you sort out @ph-fritsche's comment https://github.com/testing-library/user-event/pull/467/files#r504487490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid, thanks!
@all-contributors please add @lazytype for code and tests |
I've put up a pull request to add @lazytype! 🎉 |
🎉 This PR is included in version 12.1.8 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
…ary#467) * fix(tab): exclude hidden type inputs from focusable selector * make test leaner Co-authored-by: Michael Mitchell <michael.mitchell@mongodb.com>
fix #466
What:
tab
attempts to focus hidden type inputsWhy: 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: