OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.45k stars 2.41k forks source link

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

Open rjpowers10 opened 1 week ago

rjpowers10 commented 1 week ago

Fixes #17023

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

sebastienros commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 4 days ago

@sebastienros anything else to add here?