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

Rework userEvent.type #521

Closed
4 of 8 tasks
ph-fritsche opened this issue Dec 14, 2020 · 11 comments
Closed
4 of 8 tasks

Rework userEvent.type #521

ph-fritsche opened this issue Dec 14, 2020 · 11 comments
Labels
accuracy Improves the accuracy of how behavior is simulated enhancement New feature or request

Comments

@ph-fritsche
Copy link
Member

ph-fritsche commented Dec 14, 2020

We've got multiple open issues regarding the implementation of userEvent.type.
And while a few people offered help on this, I've got the impression we're a bit lost regarding what the code should do and look like in general as well as how and where each smaller problem should be addressed.
I propose we collect those issues, discuss what we want to achieve and refactor and comment the code so that we get more confidence working on this.

Should I have overlooked something, please mention it and I'll add it to the list.

What do people expect userEvent.type to do? What should it do?
While userEvent.click is very clear and intuitive to use as userEvent click this element!,
I guess userEvent.type started the same way userEvent trigger the key events to add to the value of this element!,
but I think most people expect it to be more userEvent press those buttons on the keyboard!.

I think it should resolve around document.getSelection() and userEvent.type(null, "a{del}{shift}b{tab}c{/shift}[Semicolon]d{selectall}") should be a valid expression to test handling of hot keys and similar.

One problem that comes to my mind is how to handle the keypress events as they are triggered multiple times as long as the button is pressed. Maybe a configurable random timeout between key events? So that even an expression for Press [A] and then press [B] before releasing [A]. Then release [B]. would make sense.

@sethreidnz
Copy link

This makes a lot of sense to me, I am keen to try to pick up some part of this as I mentioned yesterday but digging into the code, it is very much written with inputs in mind and it is a little unclear the best way to improve this... This looks like a great summary!

@ph-fritsche
Copy link
Member Author

@kentcdodds Could you weigh in on this before we start any work here?

What do people expect userEvent.type to do? What should it do?
[...] I think most people expect it to be more userEvent press those buttons on the keyboard!.

I think it should resolve around document.getSelection() and userEvent.type(null, "a{del}{shift}b{tab}c{/shift}[Semicolon]d{selectall}") should be a valid expression to test handling of hot keys and similar.

@kentcdodds
Copy link
Member

All userEvent methods should be: "fire all the same events that a user would if they were to perform that action." There's some subjectivity here. For example, in type, we fire some mouse and click events before typing, but some people might tab into the input rather than clicking on it. But in general we try to make the most sensible decision.

@ph-fritsche
Copy link
Member Author

I guess it will be a step towards that premise:

  • Keep userEvent.type(element, "foo", {skipClick = false}) for backward compatibility and as a shortcut which people are used to.
  • Prefer userEvent.type(null, "foo", {skipClick = true}) to act on the activeElement right away.
value meaning
a {key: 'a'}
{shift}a{/shift}
  1. press and hold {key: 'Shift'}
  2. press {key: 'a', shiftKey: true}
  3. release {key: 'Shift'}
[ShiftLeft] {code: 'ShiftLeft'}
{[ShiftLeft]}[KeyA]{/[ShiftLeft]}
  1. press and hold {code: 'ShiftLeft'}
  2. press {code: 'KeyA', shiftKey: true}
  3. release {code: 'ShiftLeft'}
{a}[ShiftLeft]{/a}
  1. press and hold {key: 'a'}
  2. press {code: 'ShiftLeft'}
  3. release {key: 'a'}
  • Provide key-code mapping for common US-QWERTY layout so that event handlers for both could exist at the same time.

@nickserv
Copy link
Member

nickserv commented Dec 22, 2020

  • Keep userEvent.type(element, "foo", {skipClick = false}) for backward compatibility and as a shortcut which people are used to.
  • Prefer userEvent.type(null, "foo", {skipClick = true}) to act on the activeElement right away.

It's unusual to have an optional argument at the beginning, and I think it adds more confusion than it's worth when it represents something different. Perhaps we could name it userEvent.typeActive or userEvent.typeAnywhere?

@nickserv nickserv added accuracy Improves the accuracy of how behavior is simulated enhancement New feature or request help wanted labels Dec 22, 2020
@sethreidnz
Copy link

I tend to agree with @nickmccurdy. Would it make sense to think about the different types of thing a user is actually trying to do and approach them differently with a different API. Specifically

  • typing in an input (current type functionality)
  • pressing buttons as commands / keyboard navigation

I feel like there is some core functionality in there perhaps but all the assumptions of clicking first and things like that, don't make sense for someone pressing say shift + f to do a shortcut key in your app.

@ph-fritsche
Copy link
Member Author

As all Testing-Library packages aim for readability of the test, exposing the same core functionality per two APIs makes a lot of sense.

@sethreidnz What exactly do you have in mind regarding the assumptions apart from clicking first?
I think after clicking and setting the range the implementation already tries to follow the actual behavior in the browser regarding movement of focus etc and different behavior is considered a bug.

@martin-watts
Copy link

I've spotted another issue with userEvent.type and number fields. I think the approach suggested (fire key events at a specific element/the currently active element) will probably fix it. Any approach where type is too knowledgable about the element receiving the events is likely to be flaky.

If it's strictly necessary, I'd suggest an optional callback to allow the default 'type next key' behaviour to be overridden.

@psullivan6
Copy link

I have found what I believe to be a related issue to this one, which is documented in this CodeSandbox: https://codesandbox.io/s/userevent-unit-test-ncmgu?file=/src/App.test.js

Setup of the tested component

  1. contains a text input with an initial value of 1 string character (ex: '1')
  2. executes the HTMLInputElement.select() method on input focus so every new value completely overwrites the previous one

Test execution:

  1. Render the tested component with input
  2. Get the input by role
  3. Change the input value via userEvent.type(... (ex: '11123')
  4. Notice the received input value is '23' and not '11123'

@ph-fritsche
Copy link
Member Author

ph-fritsche commented Mar 11, 2021

@psullivan6 That seems to be a problem with jsdom. document.getSelection().rangeCount is 0 after element.select() in the test environment - but it is 1 in the browser. Nevermind, we're using element.selectionStart and element.selectionEnd in the implementation. Let's inspect it in the new implementation at #581

@ph-fritsche
Copy link
Member Author

The rewrite is merged. The remaining issues can be tackled individually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accuracy Improves the accuracy of how behavior is simulated enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants