Skip to content

fix(form-core): touch all fields regardless of canSubmit in <form>.handleSubmit() #1367

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

Merged

Conversation

LeCarbonator
Copy link
Contributor

@LeCarbonator LeCarbonator commented Apr 2, 2025

The early return on this line prevents fields from being marked as touched.

Test cases have been added to address this inconsistency. It's assumed that the following statements must be true:

  • handleSubmit() touches all fields, even if it canSubmit === false
  • handleSubmit() does not call form validators if field validators errored
  • handleSubmit() calls form validators if field validators succeeded

Reasoning for the first assumption

validateAllFields is called during handleSubmit, which touches all fields. However, that code is not reached if validation errored beforehand.

This can lead to results where an optional field is edited, causing an onChange error in a required field that is not yet touched (a form-level validation schema, for example).

The form is now invalid and will not validate all fields, despite no obvious error appearing as no required fields were touched.

This PR closes #1360 if merged.

The early return prevents fields from being marked as touched.
The check has been removed as field validation prevents final submission anyways.
Copy link

nx-cloud bot commented Apr 2, 2025

View your CI Pipeline Execution ↗ for commit 5ee9576.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ✅ Succeeded 1m 56s View ↗
nx run-many --target=build --exclude=examples/** ✅ Succeeded 19s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-17 20:27:42 UTC

Copy link

pkg-pr-new bot commented Apr 2, 2025

More templates

@tanstack/angular-form

npm i https://pkg.pr.new/@tanstack/angular-form@1367

@tanstack/form-core

npm i https://pkg.pr.new/@tanstack/form-core@1367

@tanstack/lit-form

npm i https://pkg.pr.new/@tanstack/lit-form@1367

@tanstack/react-form

npm i https://pkg.pr.new/@tanstack/react-form@1367

@tanstack/solid-form

npm i https://pkg.pr.new/@tanstack/solid-form@1367

@tanstack/vue-form

npm i https://pkg.pr.new/@tanstack/vue-form@1367

commit: 5ee9576

Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.57%. Comparing base (ba66490) to head (5ee9576).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1367      +/-   ##
==========================================
+ Coverage   88.52%   88.57%   +0.05%     
==========================================
  Files          28       28              
  Lines        1316     1322       +6     
  Branches      343      345       +2     
==========================================
+ Hits         1165     1171       +6     
  Misses        134      134              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@juanvilladev
Copy link
Contributor

Looks like you're completely removing the canSubmit check. Instead I think something safer would be to move the validateAllFields line above canSubmit. If we simply remove the canSubmit flag, I don't see the point in having it?

validateAllFields is what marks them all as touched.

Even then I'm not sure if maintainers would be okay with always allowing validateAllFields to run despite canSubmit being false?

@LeCarbonator
Copy link
Contributor Author

LeCarbonator commented Apr 2, 2025

Looks like you're completely removing the canSubmit check. Instead I think something safer would be to move the validateAllFields line above canSubmit. If we simply remove the canSubmit flag, I don't see the point in having it?

validateAllFields is what marks them all as touched.

Even then I'm not sure if maintainers would be okay with always allowing validateAllFields to run despite canSubmit being false?

Indeed. It's the main reason I marked it as draft, as the implemented solution isn't a solution at all. However, some of the test cases are set up, and there's still some questions left up for discussion about how the behaviour should work.

I think the main point of the canSubmit flag is that the user can access it in the frontend and, say, disable the submit button. Once handleSubmit is called, the question is what should always happen and what should be conditional.

@juanvilladev
Copy link
Contributor

Looks like you're completely removing the canSubmit check. Instead I think something safer would be to move the validateAllFields line above canSubmit. If we simply remove the canSubmit flag, I don't see the point in having it?

validateAllFields is what marks them all as touched.

Even then I'm not sure if maintainers would be okay with always allowing validateAllFields to run despite canSubmit being false?

Indeed. It's the main reason I marked it as draft, as the implemented solution isn't a solution at all. However, some of the test cases are set up, and there's still some questions left up for discussion about how the behaviour should work.

I think the main point of the canSubmit flag is that the user can access it in the frontend and, say, disable the submit button. Once handleSubmit is called, the question is what should always happen and what should be conditional.

Oh! Completely missed this was a draft. My bad! Nice, I understand what you mean. Perhaps some might find it weird if canSubmit is false, yet user CAN technically submit without users of the library explicitly disabling the button.

…bmit

along with test cases to ensure proper form meta, restore the canSubmit check in
FormApi#handleSubmit(). Mark all fields as touched before checking canSubmit.
@LeCarbonator
Copy link
Contributor Author

Looks like you're completely removing the canSubmit check. Instead I think something safer would be to move the validateAllFields line above canSubmit. If we simply remove the canSubmit flag, I don't see the point in having it?
validateAllFields is what marks them all as touched.
Even then I'm not sure if maintainers would be okay with always allowing validateAllFields to run despite canSubmit being false?

Indeed. It's the main reason I marked it as draft, as the implemented solution isn't a solution at all. However, some of the test cases are set up, and there's still some questions left up for discussion about how the behaviour should work.
I think the main point of the canSubmit flag is that the user can access it in the frontend and, say, disable the submit button. Once handleSubmit is called, the question is what should always happen and what should be conditional.

Oh! Completely missed this was a draft. My bad! Nice, I understand what you mean. Perhaps some might find it weird if canSubmit is false, yet user CAN technically submit without users of the library explicitly disabling the button.

Along with the test cases, the canSubmit check should now be restored. It simply marks all fields as touched before doing so.

That leaves the other questions up for debate, but this is a start.

@danharper
Copy link

Would love to see a fix landed for this - first time using tanstack form and hit this exact issue! Landed on the same solution myself - either removing the "canSubmit" early return, or explicitly touching all fields :)


The "standard schema" example shows this bug pretty well, plus oddities with "canSubmit".

Type into first name, and you won't see any errors for last name until you focus & blur that field - "Submit" is disabled so you'll only know why it can't be submitted until you click in & out of each field.

riewj.mov

Along with removing the disabled={!canSubmit} logic on the button in the example, this PR fixes everything -- the other field errors are shown after clicking the button.

@LeCarbonator
Copy link
Contributor Author

LeCarbonator commented Apr 7, 2025

Would love to see a fix landed for this - first time using tanstack form and hit this exact issue! Landed on the same solution myself - either removing the "canSubmit" early return, or explicitly touching all fields :)

The "standard schema" example shows this bug pretty well, plus oddities with "canSubmit".

Type into first name, and you won't see any errors for last name until you focus & blur that field - "Submit" is disabled so you'll only know why it can't be submitted until you click in & out of each field.
riewj.mov

Along with removing the disabled={!canSubmit} logic on the button in the example, this PR fixes everything -- the other field errors are shown after clicking the button.

I initially waited because of the quick and dirty fix I implemented and to hear more of the contributors, but since properly implementing it and genuine issues coming up with this bug, I'll mark the PR ready for review. I can always make a future PR if needed :)

@LeCarbonator LeCarbonator marked this pull request as ready for review April 7, 2025 05:20
@ibnumusyaffa
Copy link

i think its solved with the new canSubmitWhenInvalid form option in this PR

#1234

@LeCarbonator
Copy link
Contributor Author

i think its solved with the new canSubmitWhenInvalid form option in this PR

#1234

Not quite. The linked PR addresses validation to still run even if canSubmit is false (using the provided option). This PR addresses that it should always touch all fields, regardless of canSubmit.

Copy link
Member

@Balastrong Balastrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're saying that clicking the submit button should always touch all fields?

To be honest I don't like it. To me a field is touched only if the user has actually interacted with it, which is not what is happening here.
However, validateAllFields is already touching everything anyway.

I believe checking on validationAttempts > 0 || isTouched should be the right condition to show errors without having to touch everything but until we change the isTouched behaviour (I'm not saying it's gonna happen) I believe this PR goes into the right direction making for consistency.

Thank you, great work!

@LeCarbonator
Copy link
Contributor Author

We're saying that clicking the submit button should always touch all fields?

To be honest I don't like it. To me a field is touched only if the user has actually interacted with it, which is not what is happening here.

However, validateAllFields is already touching everything anyway.

I believe checking on validationAttempts > 0 || isTouched should be the right condition to show errors without having to touch everything but until we change the isTouched behaviour (I'm not saying it's gonna happen) I believe this PR goes into the right direction making for consistency.

Thank you, great work!

True! However, I see the submit button as "I am sending what I have seen and written your way". If a field is untouched, submitting would therefore imply that the user saw no reason to touch it and is okay with the given default.

@LeCarbonator LeCarbonator force-pushed the fix-form-validators-canSubmit-behaviour branch from a637c76 to 67ab191 Compare April 17, 2025 20:21
@LeCarbonator LeCarbonator force-pushed the fix-form-validators-canSubmit-behaviour branch from 67ab191 to 5ee9576 Compare April 17, 2025 20:24
@Balastrong Balastrong merged commit 75af027 into TanStack:main Apr 18, 2025
6 checks passed
@LeCarbonator LeCarbonator deleted the fix-form-validators-canSubmit-behaviour branch April 18, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handleSubmit does not always mark all fields as touched
5 participants