-
Notifications
You must be signed in to change notification settings - Fork 252
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:type no longer clips final character with negative maxlength #239
Conversation
Codecov Report
@@ Coverage Diff @@
## master #239 +/- ##
=======================================
Coverage 98.63% 98.64%
=======================================
Files 1 1
Lines 147 148 +1
Branches 42 42
=======================================
+ Hits 145 146 +1
Misses 2 2
Continue to review full report at Codecov.
|
src/index.js
Outdated
@@ -183,7 +183,11 @@ const userEvent = { | |||
}; | |||
const opts = Object.assign(defaultOpts, userOpts); | |||
|
|||
const computedText = text.slice(0, element.maxLength || text.length); | |||
const maxLength = | |||
element.maxLength && element.maxLength >= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
element.maxLength && element.maxLength >= 0
seems slightly misleading because >= 0
makes it look like element.maxLength
will be applied if it's 0
, which isn't true. Particularly relevant in that jsdom seems to use 0 as its default/unlimited maxlength value for HTMLTextArea, which deviates from specs/browser implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a really good point that I overlooked. I've updated this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, I opened jsdom/jsdom#2927 since that's the real source of deviation
@@ -183,7 +183,8 @@ const userEvent = { | |||
}; | |||
const opts = Object.assign(defaultOpts, userOpts); | |||
|
|||
const computedText = text.slice(0, element.maxLength || text.length); | |||
const maxLength = element.maxLength > 0 ? element.maxLength : text.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the MDN documentation:
The maxlength attribute defines the maximum number of characters (as UTF-16 code units) the user can enter into an or <textarea>. This must be an integer value 0 or higher. If no maxlength is specified, or an invalid value is specified, the input or textarea has no maximum length.
So it seems like we wouldn't want to check for maxLength > 0
, because 0
is a valid value for maxLength. It needs to check for >= 0
instead, which necessitates another test for maxLength: 0
#263 resolves this; closing as dupe |
Thanks for the PR @cbacon91! Sorry I never noticed it before. I'm only now starting to pick up maintenance of this package a bit. |
Resolves #217.
I noticed this issue when writing tests in a private repo.
I'm not very familiar with
jest
, so I'm not sure if it's something with that or something else, but I noticed that elements created in the existing specs don't run into themaxLength
problem mentioned in the linked issue, so I handled it a bit heavy-handedly in the spec.I do know that creating a bland element with no attrs from a Chrome console can show the max length "bug".