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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ab7329
fix(tab): exclude hidden type inputs from focusable selector
lazytype Oct 14, 2020
2d5c362
make test leaner
Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/__tests__/focus.js
View file
Open in desktop
Expand Up @@ -43,6 +43,15 @@ test('no events fired on a disabled focusable input', () => {
expect(element).not.toHaveFocus()
})

test('no events fired on a hidden input', () => {
const {element, getEventSnapshot} = setup(`<input type="hidden" />`)
focus(element)
expect(getEventSnapshot()).toMatchInlineSnapshot(
`No events were fired on: input[value=""]`,
)
expect(element).not.toHaveFocus()
})

test('no events fired if the element is already focused', () => {
const {element, getEventSnapshot, clearEventCalls} = setup(`<button />`)
focus(element)
Expand Down
21 changes: 20 additions & 1 deletion src/__tests__/tab.js
View file
Open in desktop
Expand Up @@ -221,7 +221,7 @@ test('should respect tab index order, then DOM order', () => {
expect(checkbox).toHaveFocus()
})

test('should suport a mix of elements with/without tab index', () => {
test('should support a mix of elements with/without tab index', () => {
setup(`
<div>
<input data-testid="element" tabIndex="0" type="checkbox" />
Expand Down Expand Up @@ -287,6 +287,25 @@ test('should not tab to <a> with no href', () => {
expect(link).toHaveFocus()
})

test('should not tab to <input> with type="hidden"', () => {
setup(`
<div>
<input data-testid="element" tabIndex="0" type="checkbox" />
<input type="hidden">
<input data-testid="element" type="text" />
</div>`)

const [checkbox, text] = document.querySelectorAll('[data-testid="element"]')
ph-fritsche marked this conversation as resolved.
Show resolved Hide resolved

userEvent.tab()

expect(checkbox).toHaveFocus()

userEvent.tab()

expect(text).toHaveFocus()
})

test('should stay within a focus trap', () => {
setup(`
<>
Expand Down
2 changes: 1 addition & 1 deletion src/utils.js
View file
Open in desktop
Expand Up @@ -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.

@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

'button:not([disabled])',
'select:not([disabled])',
'textarea:not([disabled])',
Expand Down