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: button trigger space #409

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Expand Up @@ -185,6 +185,7 @@ The following special character strings are supported:
| Text string | Key | Modifier | Notes |
| ------------- | --------- | ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `{enter}` | Enter | N/A | Will insert a newline character (`<textarea />` only). |
| `{space}` | `' '` | N/A | |
| `{esc}` | Escape | N/A | |
| `{backspace}` | Backspace | N/A | Will delete the previous character (or the characters within the `selectedRange`). |
| `{del}` | Delete | N/A | Will delete the next character (or the characters within the `selectedRange`) |
Expand Down
6 changes: 3 additions & 3 deletions package.json
Expand Up @@ -41,14 +41,14 @@
"@babel/runtime": "^7.10.2"
},
"devDependencies": {
"@testing-library/dom": "^7.16.0",
"@testing-library/jest-dom": "^5.10.1",
"@testing-library/dom": "^7.21.4",
"@testing-library/jest-dom": "^5.11.1",
"is-ci": "^2.0.0",
"jest-serializer-ansi": "^1.0.3",
"kcd-scripts": "^6.2.3"
},
"peerDependencies": {
"@testing-library/dom": ">=7.16.0"
"@testing-library/dom": ">=7.21.4"
},
"eslintConfig": {
"extends": "./node_modules/kcd-scripts/eslint.js",
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/deselect-options.js
Expand Up @@ -52,6 +52,7 @@ test('blurs previously focused element', () => {
option[value="1"][selected=false] - mousemove: Left (0)
option[value="1"][selected=false] - pointerdown
option[value="1"][selected=false] - mousedown: Left (0)
button - focusout
select[name="select"][value=[]] - focusin
option[value="1"][selected=false] - pointerup
option[value="1"][selected=false] - mouseup: Left (0)
Expand Down
3 changes: 3 additions & 0 deletions src/__tests__/paste.js
Expand Up @@ -11,6 +11,7 @@ test('should paste text in input', () => {
Events fired on: input[value="Hello, world!"]
input[value=""] - focus
input[value=""] - focusin
input[value=""] - paste
input[value="Hello, world!"] - input
"{CURSOR}" -> "Hello, world!{CURSOR}"
Expand All @@ -28,6 +29,7 @@ test('should paste text in textarea', () => {
Events fired on: textarea[value="Hello, world!"]
textarea[value=""] - focus
textarea[value=""] - focusin
textarea[value=""] - paste
textarea[value="Hello, world!"] - input
"{CURSOR}" -> "Hello, world!{CURSOR}"
Expand All @@ -43,6 +45,7 @@ test('does not paste when readOnly', () => {
Events fired on: input[value=""]
input[value=""] - focus
input[value=""] - focusin
input[value=""] - paste
`)
})
Expand Down
175 changes: 175 additions & 0 deletions src/__tests__/type-modifiers.js
Expand Up @@ -443,6 +443,181 @@ test('{enter} on a button', () => {
`)
})

test('{space} on a button', () => {
const {element, getEventSnapshot} = setup('<button />')

userEvent.type(element, '{space}')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: button

button - pointerover
button - pointerenter
button - mouseover: Left (0)
button - mouseenter: Left (0)
button - pointermove
button - mousemove: Left (0)
button - pointerdown
button - mousedown: Left (0)
button - focus
button - focusin
button - pointerup
button - mouseup: Left (0)
button - click: Left (0)
Comment on lines +454 to +466
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking this is probably a special case where simulating mousing over and clicking wouldn't make sense right? Because anyone trying to do: userEvent.type(button, '{space}') is probably not wanting to fire two click events... Hmmm... Should we make this a special case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was asking myself the same question. For the sake of consistency I decided it should behave the same way it does with other elements.

Maybe we could add a section about this to the documentation? So it is clear that the most popular way to test keyboard events on a button would be:

button.focus()
userEvent.type(button, '{space}', { skipClick: true })

Another idea would be to use element.focus() instead of userEvent.click(element) as a default for type.

The benefit would be, that it is less "magic" and we could get rid of skipClick altogether.
Also the .focus() would just fire additional events, if the element doesn't have focus already. So it would just be a minimal safety net.

For tests that require a click beforehand, it would still be easy to do that:

userEvent.click(input)
userEvent.type(input, 'foo')

button - keydown: (32)
button - keypress: (32)
button - keyup: (32)
button - click: Left (0)
`)
})

test(`' ' on a button`, () => {
const {element, getEventSnapshot} = setup('<button />')

userEvent.type(element, ' ')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: button

button - pointerover
button - pointerenter
button - mouseover: Left (0)
button - mouseenter: Left (0)
button - pointermove
button - mousemove: Left (0)
button - pointerdown
button - mousedown: Left (0)
button - focus
button - focusin
button - pointerup
button - mouseup: Left (0)
button - click: Left (0)
button - keydown: (32)
button - keypress: (32)
button - keyup: (32)
button - click: Left (0)
`)
Comment on lines +505 to +530
Copy link
Member

Choose a reason for hiding this comment

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

Could we make it clear that all we're really testing is that ' ' is the same as '{space}'?

Suggested change
test(`' ' on a button`, () => {
const {element, getEventSnapshot} = setup('<button />')
userEvent.type(element, ' ')
expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: button
button - pointerover
button - pointerenter
button - mouseover: Left (0)
button - mouseenter: Left (0)
button - pointermove
button - mousemove: Left (0)
button - pointerdown
button - mousedown: Left (0)
button - focus
button - focusin
button - pointerup
button - mouseup: Left (0)
button - click: Left (0)
button - keydown: (32)
button - keypress: (32)
button - keyup: (32)
button - click: Left (0)
`)
test(`' ' on a button is the same as '{space}'`, () => {
const {element: e1, getEventSnapshot: snap1} = setup('<button />')
const {element: e2, getEventSnapshot: span2} = setup('<button />')
userEvent.type(e1, ' ')
userEvent.type(e2, '{space}')
expect(snap1()).toBe(snap2())

})

test('{space} on an input', () => {
const {element, getEventSnapshot} = setup(`<input value="" />`)

userEvent.type(element, '{space}')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value=" "]

input[value=""] - pointerover
input[value=""] - pointerenter
input[value=""] - mouseover: Left (0)
input[value=""] - mouseenter: Left (0)
input[value=""] - pointermove
input[value=""] - mousemove: Left (0)
input[value=""] - pointerdown
input[value=""] - mousedown: Left (0)
input[value=""] - focus
input[value=""] - focusin
input[value=""] - pointerup
input[value=""] - mouseup: Left (0)
input[value=""] - click: Left (0)
input[value=""] - keydown: (32)
input[value=""] - keypress: (32)
input[value=" "] - input
"{CURSOR}" -> " {CURSOR}"
input[value=" "] - keyup: (32)
`)
})

test('{enter} on an input type="color"', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as the space on a type color. Why is type color so special it needs a test? When this test fails, how does the developer know what part of the test is important for fixing?

const {element, getEventSnapshot} = setup(
`<input value="#ffffff" type="color" />`,
)

userEvent.type(element, '{enter}')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value="#ffffff"]

input[value="#ffffff"] - pointerover
input[value="#ffffff"] - pointerenter
input[value="#ffffff"] - mouseover: Left (0)
input[value="#ffffff"] - mouseenter: Left (0)
input[value="#ffffff"] - pointermove
input[value="#ffffff"] - mousemove: Left (0)
input[value="#ffffff"] - pointerdown
input[value="#ffffff"] - mousedown: Left (0)
input[value="#ffffff"] - focus
input[value="#ffffff"] - focusin
input[value="#ffffff"] - pointerup
input[value="#ffffff"] - mouseup: Left (0)
input[value="#ffffff"] - click: Left (0)
input[value="#ffffff"] - keydown: Enter (13)
input[value="#ffffff"] - keypress: Enter (13)
input[value="#ffffff"] - click: Left (0)
input[value="#ffffff"] - keyup: Enter (13)
`)
})

test('{space} on an input type="color"', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could we make it clear (probably via a comment or the test title) what about this test is important? It's hard to tell as it looks just like all the other tests. Is there some special reason we want to test the color type? (I just tried out an input[type=color] and realized in chrome it renders a button with a color picker so that makes sense why we've got this test here, but a comment would be helpful).

const {element, getEventSnapshot} = setup(
`<input value="#ffffff" type="color" />`,
)

userEvent.type(element, '{space}')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value="#ffffff"]

input[value="#ffffff"] - pointerover
input[value="#ffffff"] - pointerenter
input[value="#ffffff"] - mouseover: Left (0)
input[value="#ffffff"] - mouseenter: Left (0)
input[value="#ffffff"] - pointermove
input[value="#ffffff"] - mousemove: Left (0)
input[value="#ffffff"] - pointerdown
input[value="#ffffff"] - mousedown: Left (0)
input[value="#ffffff"] - focus
input[value="#ffffff"] - focusin
input[value="#ffffff"] - pointerup
input[value="#ffffff"] - mouseup: Left (0)
input[value="#ffffff"] - click: Left (0)
input[value="#ffffff"] - keydown: (32)
input[value="#ffffff"] - keypress: (32)
input[value="#ffffff"] - keyup: (32)
input[value="#ffffff"] - click: Left (0)
`)
})

test('" " on an input type="color"', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's do the same to this one as we do with the button one above.

const {element, getEventSnapshot} = setup(
`<input value="#ffffff" type="color" />`,
)

userEvent.type(element, ' ')

expect(getEventSnapshot()).toMatchInlineSnapshot(`
Events fired on: input[value="#ffffff"]

input[value="#ffffff"] - pointerover
input[value="#ffffff"] - pointerenter
input[value="#ffffff"] - mouseover: Left (0)
input[value="#ffffff"] - mouseenter: Left (0)
input[value="#ffffff"] - pointermove
input[value="#ffffff"] - mousemove: Left (0)
input[value="#ffffff"] - pointerdown
input[value="#ffffff"] - mousedown: Left (0)
input[value="#ffffff"] - focus
input[value="#ffffff"] - focusin
input[value="#ffffff"] - pointerup
input[value="#ffffff"] - mouseup: Left (0)
input[value="#ffffff"] - click: Left (0)
input[value="#ffffff"] - keydown: (32)
input[value="#ffffff"] - keypress: (32)
input[value="#ffffff"] - keyup: (32)
input[value="#ffffff"] - click: Left (0)
`)
})

test('{enter} on a textarea', () => {
const {element, getEventSnapshot} = setup('<textarea></textarea>')

Expand Down
45 changes: 45 additions & 0 deletions src/__tests__/type.js
Expand Up @@ -715,3 +715,48 @@ test('should give error if we are trying to call type on an invalid element', as
`"the current element is of type BODY and doesn't have a valid value"`,
)
})

test('typing on button should not alter its value', () => {
const {element} = setup('<button value="foo" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('foo')
})

test('typing on input type button should not alter its value', () => {
const {element} = setup('<input type="button" value="foo" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('foo')
})

test('typing on input type color should not alter its value', () => {
const {element} = setup('<input type="color" value="#ffffff" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('#ffffff')
})

test('typing on input type image should not alter its value', () => {
const {element} = setup('<input type="image" value="foo" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('foo')
})

test('typing on input type reset should not alter its value', () => {
const {element} = setup('<input type="reset" value="foo" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('foo')
})

test('typing on input type submit should not alter its value', () => {
const {element} = setup('<input type="submit" value="foo" />')
userEvent.type(element, 'bar')
expect(element).toHaveValue('foo')
})

test('typing on input type file should not trigger input event', () => {
const inputHandler = jest.fn()
const {element} = setup('<input type="file" />', {
eventHandlers: {input: inputHandler},
})
userEvent.type(element, 'bar')
expect(inputHandler).toHaveBeenCalledTimes(0)
})
4 changes: 1 addition & 3 deletions src/blur.js
@@ -1,14 +1,12 @@
import {fireEvent} from '@testing-library/dom'
import {getActiveElement, isFocusable, eventWrapper} from './utils'

function blur(element, init) {
function blur(element) {
if (!isFocusable(element)) return

const wasActive = getActiveElement(element.ownerDocument) === element
if (!wasActive) return

eventWrapper(() => element.blur())
fireEvent.focusOut(element, init)
}

export {blur}
4 changes: 1 addition & 3 deletions src/focus.js
@@ -1,14 +1,12 @@
import {fireEvent} from '@testing-library/dom'
import {getActiveElement, isFocusable, eventWrapper} from './utils'

function focus(element, init) {
function focus(element) {
if (!isFocusable(element)) return

const isAlreadyActive = getActiveElement(element.ownerDocument) === element
if (isAlreadyActive) return

eventWrapper(() => element.focus())
fireEvent.focusIn(element, init)
}

export {focus}