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

typing fails if input matches selection #583

Closed
ph-fritsche opened this issue Mar 16, 2021 · 9 comments · Fixed by #623
Closed

typing fails if input matches selection #583

ph-fritsche opened this issue Mar 16, 2021 · 9 comments · Fixed by #623
Labels
bug Something isn't working released

Comments

@ph-fritsche
Copy link
Member

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'

Originally posted by @psullivan6 in #521 (comment)

@ph-fritsche
Copy link
Member Author

This is probably caused by fireInputEventIfNeeded. The key events fire as expected, the input event is missing.

@ph-fritsche ph-fritsche added the bug Something isn't working label Mar 16, 2021
@fergusmcdonald
Copy link
Contributor

fergusmcdonald commented Mar 21, 2021

Hi @ph-fritsche 👋

What is reasoning behind not firing input events if newValue is the same as prevValue in fireInputEventIfNeeded? https://github.com/testing-library/user-event/blob/master/src/keyboard/shared/fireInputEventIfNeeded.ts#L27-L32

Is this a generalization which is mostly true for how the browser decides to fire input events? 🤔

I can see an input event is fired when replicating the issue in the sandbox.

issue-583-sandbox-clip.mp4

Removing the rule will result in input events being fired in many scenarios where they shouldn't (see snapshots of commit referenced below).

@ph-fritsche
Copy link
Member Author

@fergusmcdonald Thanks for taking the time to look into this. 😃

What is reasoning behind not firing input events if newValue is the same as prevValue in fireInputEventIfNeeded? https://github.com/testing-library/user-event/blob/master/src/keyboard/shared/fireInputEventIfNeeded.ts#L27-L32

Is this a generalization which is mostly true for how the browser decides to fire input events?

This is one of the parts of which I'm not sure yet, why they were implemented the way they are.

Removing the rule will result in input events being fired in many scenarios where they shouldn't (see snapshots of commit referenced below).

When rewriting the implementation for userEvent.type I tried to maintain the behavior that was established before, unless it was already determined to be a bug caused by the previous implementation of userEvent.type.
One of the reasons for the rewrite was that it should allow us to isolate concerns like the one tackled by fireInputEventIfNeeded and verify if the expectations laid out by the existing tests are correct.

https://codesandbox.io/s/keyevent-vtdcc?file=/src/App.js

@fergusmcdonald
Copy link
Contributor

fergusmcdonald commented Mar 22, 2021

This is one of the parts of which I'm not sure yet, why they were implemented the way they are.

@ph-fritsche - This condition may have originated from the following spec: https://html.spec.whatwg.org/multipage/input.html#common-input-element-events

Examples of a user changing the element's value would include the user typing into a text control, pasting a new value into the control, or undoing an edit in that control. Some user interactions do not cause changes to the value, e.g., hitting the "delete" key in an empty text control, or replacing some text in the control with text from the clipboard that happens to be exactly the same text.

@ph-fritsche
Copy link
Member Author

Yes, a misunderstanding of that paragraph might be the reason. There is no change event on blur if the value did not change.

@ph-fritsche
Copy link
Member Author

There is no input event on <input type="number"/> though, if the added character leads to a value that can not result in a valid input by typing more characters. I.e. 1ee the second e is ignored.
But this should be addressed at

if (!/[\d.\-e]/.test(keyDef.key as string)) {
return
}
const oldValue =
state.carryValue ?? getValue(element) ?? /* istanbul ignore next */ ''

@ph-fritsche
Copy link
Member Author

There is also no input event on <input type="date"/> for incomplete values.
The input events have no inputType or data.

ph-fritsche added a commit that referenced this issue Mar 24, 2021
ph-fritsche added a commit that referenced this issue Mar 25, 2021
* wip: clean up fireInputEvent helper

* fix(type): setSelectionRange on elements without selectionRange

* refactor: maxLength

* test: demonstrate #583

* fix: refactor calculateNewValue

* fix: relative imports
@ph-fritsche
Copy link
Member Author

@fergusmcdonald Thanks for starting the work on this. The main problem is solved now. But there are still a few edge cases that might need investigation.

@github-actions
Copy link

🎉 This issue has been resolved in version 13.0.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants