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

Ensure checked boxes are preserved in OrchardCore.Forms even where th… #17024

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rjpowers10
Copy link
Contributor

@rjpowers10 rjpowers10 commented Nov 14, 2024

Fixes #17023

Ensure checked boxes are preserved in OrchardCore.Forms even where there isn't a default value on the InputPart

…ere isn't a default value on the InputPart
@sebastienros
Copy link
Member

I am still confused by this change. Here the template is supposed to render the <input> tag representing the widget "InputPart".

From what I understand the on value is what is sent by browsers when the <input> has no value attribute but the checkbox is checked. So if any, our logic to handle on should be on a server-side logic, not on the rendering part. I feel like we should instead just omit the value attribute if the part doesn't have any.

Another interpretation of this PR would be that we forcibly set value="on" to force the same behavior on the browser as it there were no value attribute.

Now I think I get it. In the current state when a form is submitted and checked, if the value is null there could be an issue if we generate the value attribute and set checked too. Then the next check would not be detected on our side. But if so then we should just ensure we don't render a value attribute. I would think that this is the case already (razor should detect fieldValue is null).

Either way this change seems to "work", but I'd like to understand why/what is not working today.

@rjpowers10
Copy link
Contributor Author

I would think that this is the case already (razor should detect fieldValue is null).

Good observation. I just tried my example again and yes, when fieldValue is null the value attribute is omitted from the output.

@rjpowers10
Copy link
Contributor Author

I think there's another case that doesn't work: multiple checkboxes with the same name.

View the form
image

Fill out the form, checking both boxes (they have the same HTML name)
image

Redirect back, the state is not preserved
image

In .NET the attempted value for the checkbox is "Red,Green".
image

@rjpowers10
Copy link
Contributor Author

rjpowers10 commented Nov 15, 2024

By the way, going back to #16975

The RawValue for a checkbox is a collection. You can see here I have a collection of 1 element for my checkbox. The type is List<object>. Normally I think the type is object[] (or maybe StringValues, still trying to figure this part out) but STJ deserialized it to List<object> when using the "preserve ModelState" trick provided by OrchardCore.Forms.

The reason why I was getting that InvalidOperationException in my app is because I'm trying to render a checkbox with the .NET input tag helper, and .NET doesn't know how to unwrap List<object> into a boolean value. But it does know how to unwrap object[] (see my comments in #16975). In OrchardCore.Forms though, the input tag helper isn't being used and the template is manually pulling the value out of ModelState. That's why you don't get the exception with an InputPart. But if you're like me and using the ModelState trick for a plain old MVC form, checkboxes cause exceptions.

Even though an InputPart doesn't get the exception, as we're seeing it has other problems with checkboxes.

image

image

@MikeAlhayek
Copy link
Member

@sebastienros anything else to add here?

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.

OrchardCore.Forms doesn't preserve checked checkbox state when the checkbox doesn't have a default value
3 participants