BUG: pre-submit custom form validation not applied to radiobuttons and checkboxes

bigskysoftware / htmx

</> htmx - high power tools for HTML

https://htmx.org

Other

37.67k stars 1.27k forks source link

BUG: pre-submit custom form validation not applied to radiobuttons and checkboxes #2676

Open ehenighan opened 3 months ago

ehenighan commented 3 months ago

Found while testing a slightly niche use-case in a questionnaire form I'm building - when you apply custom validation across a form using the HTML5 hooks to enforce that the user has to fill out at least one of a set of answers on a page (i.e. no particular radiogroup is required, but at least one of the visible radiogroups must have a value), the onsubmit validation isn't respected.

This is because the 'shouldInclude' function excludes unticked checkboxes/radiobuttons but the 'validateElement' check is inside the 'shouldInclude' block.

It's easy to fix and I've tested locally - just separated out 'couldInclude' and 'shouldValidate' from 'shouldInclude' in order to lift up the validation outside the 'shouldInclude' block, but wasn't sure where you'd want a PR targeting now there's version 2 and 1.9.12 on the go at the same time.

Will check back shortly to add code examples!

ehenighan commented 3 months ago

HTMX Bug #2676 - Replication.zip

Zip file contains a basic HTML file with two forms demonstrating the issue:

First form has a number input and a text input and won't submit unless you set at least one of them
Second form has a checkbox input and a radiogroup and will show the same error but (try to) submit in the background anyway

N.B. that the validation's broken after the first pass due to the fake POST that goes nowhere.

Will update with proposed fix shortly

ehenighan commented 3 months ago

Forked and added the fix I've tested locally here, for comparison:

https://github.com/bigskysoftware/htmx/compare/dev...ehenighan:htmx:bug-2676-pre-submit-custom-validation

Haven't raised a PR yet as I'm neck-deep in a project at work and I'm mindful that I've not worked with this test suite before etc. Would you want the test going into test/core/validation.js next to the existing one about custom-validation? https://github.com/bigskysoftware/htmx/blob/6f83885de3e3881d262a6267fd55be69a18daa75/test/core/validation.js#L87

Telroshan commented 3 months ago

Hey!

wasn't sure where you'd want a PR targeting now there's version 2 and 1.9.12 on the go at the same time

I see you're using htmx 1.9.12 in your bug replication sample, would you mind checking if it also happens on htmx 2?

As it's a bug, and if it's occuring on both versions, I'd say htmx would benefit from the bugfix on both htmx 1 and 2. The easiest way to go about it would probably to make a first PR targeting dev (htmx 2) where the most active development happens at, then, if and once accepted, make another PR to port that fix to v1

Would you want the test going into test/core/validation.js next to the existing one about custom-validation?

Sounds good to me!

ehenighan commented 1 month ago

Hi, sorry for the delay in coming back to this. Since I last looked into it, the refactoring to use HTMLFormElement in processInputValue has resolved the issue in v2 but I've raised an initial PR for v2 anyway just to add the relevant tests to prevent regression: https://github.com/bigskysoftware/htmx/pull/2829

I'll raise another PR to actually apply the fix to the v1 branch and then update this issue after that.

ehenighan commented 1 month ago

...and here's the v1 fix PR: https://github.com/bigskysoftware/htmx/pull/2830

Slight formatting changes from my original branch, which I will delete - I think there were probably some interim changes to linting standards or something.