-
-
Notifications
You must be signed in to change notification settings - Fork 927
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 form checkValidity(), remove vnode.dom === .activeElement from setAttr() (Continued from #2257) #3002
Conversation
Please let me know if any additional test code is needed. |
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.
Verified by mirroring the changes in your input and textarea examples.
I would like to see a manual render test at minimum, though. Here's some smaller test cases for you:
I couldn't get the minlength
constraint to work because it's specified to only validate user input, not script input. I tried instead just doing <input type="url">
as it's possible to force constraint errors that way, but I couldn't trigger the bug.
Unfortunately, what's really going on is a counterintuitive interaction I've explained in a comment, and unless you feel like wiring up Puppeteer and everything, there's no real way to test this without some exceptionally awkward mocking.
render/render.js
Outdated
//setting input[value] to same value by typing on focused element moves cursor to end in Chrome | ||
//setting input[type=file][value] to same value causes an error to be generated if it's non-empty | ||
if ((vnode.tag === "input" || vnode.tag === "textarea") && vnode.dom.value === "" + value && (isFileInput || vnode.dom === activeElement(vnode.dom))) return | ||
//setting input[value] to same value sometimes causes checkValidity() to return true when form fields are invalid(#2256) |
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.
The precise issue is this: minlength
and maxlength
constraints in form controls are only checked against values generated from user edits, not from script edits. When a user edits it and a script reads and writes the same previous value, the current value is considered to be last edited by script, and thus minlength
and maxlength
constraints are no longer checked.
I'll leave it to you how to distill this into a comment. Here's the relevant spec links:
@dead-claudia |
39eea9a
to
a5bb562
Compare
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.
Just that one nit and it's ready to merge.
ba2fcfa
to
9c22699
Compare
Sorry for the noise. Didn't realize the merge rule was stopping me over my comment edit. |
Remove vnode.dom === activeElement(vnode.dom) from setAttribute() to fix validityCheck(), to fix #2256
Description
This is #2257 reopened. (#2257 is for the
next
branch and may not be reopened as is.)I cherry-picked commit #2257 with additional comments and refactoring.
Motivation and Context
It appears that #2257 was incorrectly closed due to a typo (with 2557, #2257 (comment)).
#2257 actually fixes #2256.
How Has This Been Tested?
npm run test
flems (input)
flems (textarea)
Types of changes
Checklist