fix: button trigger space #409

visualjerk · 2020-07-22T23:19:32Z

What:

This fixes #406

Why:

Because clickable inputs and buttons should be handled differently by type.

How:

Added clickable input checks in type where it was necessary
Added support for a special character {space}
Refactored type.js to keep it understandable with these additions

Checklist:

Documentation
Tests
Typings (not needed)
Ready to be merged

visualjerk · 2020-07-22T23:30:29Z

Unfortunately the CI fails 😄

During implementation I needed to update the focus and blur behavior, because of a recent fix in jsdom: jsdom/jsdom#2708

I also updated @testing-library/dom and @testing-library/jest-dom versions to make sure the new jsdom in the CI.

However it looks like Travis is still loading a cached version. Maybe the new jsdom version is not defined in jest-dom.

Do you know how to tell Travis to rerun with a cleared cache?

kentcdodds · 2020-07-23T19:58:42Z

Awesome! Just triggered a run of CI without the cache. This is a big change! I'll try to review it now.

kentcdodds

This looks great! Thank you for taking so much time on this. It's obviously quality work. I have a few nit-picky things I hope you don't mind.

kentcdodds · 2020-07-23T20:01:20Z

src/__tests__/type-modifiers.js

+    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)


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?

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')

kentcdodds · 2020-07-23T20:05:40Z

src/__tests__/type-modifiers.js

+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)
+  `)


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())

kentcdodds · 2020-07-23T20:06:02Z

src/__tests__/type-modifiers.js

+  `)
+})
+
+test('" " on an input type="color"', () => {


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

kentcdodds · 2020-07-23T20:06:40Z

src/__tests__/type-modifiers.js

+  `)
+})
+
+test('{space} on an input type="color"', () => {


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).

kentcdodds · 2020-07-23T20:07:45Z

src/__tests__/type-modifiers.js

+  `)
+})
+
+test('{enter} on an input type="color"', () => {


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?

kentcdodds · 2020-07-23T20:22:39Z

src/type.js

+
+function getTypeCallback(remainingString) {
+  const character = remainingString[0]
+  const callback = createTypeCharacter(character)


I'm not sure we need the createTypeCharacter function. This is a quick and easy one-liner.

Suggested change

const callback = createTypeCharacter(character)

const callback = context => typeCharacter(character, context)

Sorry, just had time to read this. That's true. Initially I used createTypeCharacter in more places, but now it is a bit too much 😄

kentcdodds · 2020-07-23T20:23:01Z

src/type.js

+function createTypeCharacter(character) {
+  return context => typeCharacter(character, context)
+}


With the definition inline above we can remove this.

Suggested change

function createTypeCharacter(character) {

return context => typeCharacter(character, context)

}

kentcdodds · 2020-07-23T20:25:20Z

Oh, and the CI failures are ESLint related:

[lint] 
[lint] /home/travis/build/testing-library/user-event/src/__tests__/click.js
[lint]    82:19  error  Use not.toBeChecked() instead of toHaveProperty('checked', false)  jest-dom/prefer-checked
[lint]   110:19  error  Use toBeChecked() instead of toHaveProperty('checked', true)       jest-dom/prefer-checked
[lint]   128:19  error  Use not.toBeChecked() instead of toHaveProperty('checked', false)  jest-dom/prefer-checked
[lint] 
[lint] ✖ 3 problems (3 errors, 0 warnings)
[lint]   3 errors and 0 warnings potentially fixable with the `--fix` option.
[lint] 
[lint] npm run lint --silent exited with code 1

If you could fix those too that'd be super :) Thanks!

codecov · 2020-07-23T20:36:06Z

Codecov Report

Merging #409 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #409   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          390       418   +28     
  Branches       111       116    +5     
=========================================
+ Hits           390       418   +28

Impacted Files	Coverage Δ
src/blur.js	`100.00% <ø> (ø)`
src/focus.js	`100.00% <ø> (ø)`
src/type.js	`100.00% <100.00%> (ø)`
src/utils.js	`100.00% <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 e5db332...bc380bc. Read the comment docs.

kentcdodds · 2020-07-23T20:45:39Z

Fantastic. Thank you!

kentcdodds · 2020-07-23T20:45:48Z

@all-contributors please add @visualjerk for code, tests, and docs

allcontributors · 2020-07-23T20:45:58Z

@kentcdodds

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

kentcdodds · 2020-07-23T20:46:26Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

kentcdodds · 2020-07-23T20:52:26Z

🎉 This PR is included in version 12.0.13 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

visualjerk · 2020-07-23T20:57:35Z

@kentcdodds Thanks for your kind words 😄

As this is already merged, I can add the improvements you mentioned to the next PR.

kentcdodds · 2020-07-23T20:59:46Z

Oh, whoops 😅 I didn't check and just assumed you'd made the improvements in that one commit. Thanks!

visualjerk added 5 commits July 22, 2020 19:44

fix(406): add button space handling on type

7ff1a53

fix: remove duplicate focusIn and focusOut

3d1fd25

refactor: break down type in smaller functions

393fcd6

refactor: make typeImpl more readable

f3c982e

fix: handle clickable input types

a5816a2

visualjerk changed the title ~~Fix 406 button trigger space~~ fix: button trigger space Jul 22, 2020

visualjerk added 2 commits July 23, 2020 09:24

refactor: create new remaining string in callbacks

f2e8104

refactor: split into smaller functions

a5a7925

kentcdodds reviewed Jul 23, 2020

View reviewed changes

test: add test for space preventDefault

bc380bc

kentcdodds merged commit ee1be1c into testing-library:master Jul 23, 2020

allcontributors bot mentioned this pull request Jul 23, 2020

docs: add visualjerk as a contributor #410

Merged

kentcdodds added the released label Jul 23, 2020

visualjerk mentioned this pull request Jul 23, 2020

Error when using type on an element without a value property #407

Closed

jesujcastillom pushed a commit to jesujcastillom/user-event that referenced this pull request Oct 23, 2020

fix: button trigger space (testing-library#409)

1d57a79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: button trigger space #409

fix: button trigger space #409

visualjerk commented Jul 22, 2020

visualjerk commented Jul 22, 2020

kentcdodds commented Jul 23, 2020

kentcdodds left a comment

kentcdodds Jul 23, 2020

visualjerk Jul 23, 2020

kentcdodds Jul 23, 2020

kentcdodds Jul 23, 2020

kentcdodds Jul 23, 2020

kentcdodds Jul 23, 2020

kentcdodds Jul 23, 2020

visualjerk Jul 23, 2020

kentcdodds Jul 23, 2020

kentcdodds commented Jul 23, 2020

codecov bot commented Jul 23, 2020 •

edited

kentcdodds commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

allcontributors bot commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

visualjerk commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

	const callback = createTypeCharacter(character)
	const callback = context => typeCharacter(character, context)

	function createTypeCharacter(character) {
	return context => typeCharacter(character, context)
	}

Navigation Menu

fix: button trigger space #409

fix: button trigger space #409

Conversation

visualjerk commented Jul 22, 2020

visualjerk commented Jul 22, 2020

kentcdodds commented Jul 23, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jul 23, 2020

codecov bot commented Jul 23, 2020 • edited

Codecov Report

kentcdodds commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

allcontributors bot commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

visualjerk commented Jul 23, 2020

kentcdodds commented Jul 23, 2020

codecov bot commented Jul 23, 2020 •

edited