Fix MVC form data binding localization

[MackinnonBuck]

Fix MVC form data binding localization

This PR fixes an issue in which all form elements were being formatted and parsed using the "current" culture, while some form elements require culture-invariant formatting and parsing.

Description

Some form elements (e.g., <input type="text"/>) require culture-specific formatting and parsing because their values are directly entered by the user. However, other inputs (e.g., <input type="number"/>) use culture-invariant formatting both in the HTML source and in the form request. Thus, we need to ensure that the correct formatting will be applied for each type of form element.

The fix for this bug comes in two parts:

1. When generating HTML form elements, we use culture-invariant formatting for the value attribute when appropriate.
2. When parsing values in a form request, use the invariant culture when the value's corresponding form element uses culture-invariant formatting.

To achieve step 2, we generate an additional HTML element for all form elements that use invariant formatting: <input name="__Invariant" type="hidden" value="<model property name>"/>

This way, the form request gives us the information we need about which inputs need culture-invariant parsing.

Fixes #6566

[MackinnonBuck]

Note that the way this is currently implemented, the new behavior is the default for all existing and new projects. In order to opt-out and revert to the old behavior, apps will need to do something like this:

var builder = services.AddMvc()
    // ...
    .AddMvcOptions(options =>
    {
        options.SuppressCultureInvariantFormModelBinding = true;
    })
    .AddViewOptions(options =>
    {
        options.HtmlHelperOptions.SuppressCultureInvariantFormValueFormatting = true;
    });

There are two options to configure because one controls the "model" end of things (parsing form values) and one controls the HTML generation (formatting form element values). We could attempt to unify these, but this would probably require breaking changes to our public API (passing an MvcOptions) object through to the tag helper/HTML generation classes.

The decision to make this fix "opt-out" is not final. I'd love to hear more opinions about what the right choice is.

[brunolins16]

Note that the way this is currently implemented, the new behavior is the default for all existing and new projects. In order to opt-out and revert to the old behavior, apps will need to do something like this:

var builder = services.AddMvc()
    // ...
    .AddMvcOptions(options =>
    {
        options.SuppressCultureInvariantFormModelBinding = true;
    })
    .AddViewOptions(options =>
    {
        options.HtmlHelperOptions.SuppressCultureInvariantFormValueFormatting = true;
    });

There are two options to configure because one controls the "model" end of things (parsing form values) and one controls the HTML generation (formatting form element values). We could attempt to unify these, but this would probably require breaking changes to our public API (passing an MvcOptions) object through to the tag helper/HTML generation classes.

The decision to make this fix "opt-out" is not final. I'd love to hear more opinions about what the right choice is.

How bad it is if the user sets only one of them?

[dougbu]

How bad it is if the user sets only one of them?

Suspect the result of setting only SuppressCultureInvariantFormValueFormatting would be browser errors or incorrect display (though the current culture would need to be problematic, and the value would need to be large or small enough to require punctuation). In the SuppressCultureInvariantFormModelBinding case, again with problematic cultures and values, round trips would fail.

[dougbu]

How bad it is if the user sets only one of them?

This is worth testing @MackinnonBuck. The follow-on question is basically, "Should we instead have one MvcOptions.SuppressCultureInvariantRoundTripping option❔"

[MackinnonBuck]

How bad it is if the user sets only one of them?

If you only set SuppressCultureInvariantFormModelBinding, then the behavior remains the same as it is today from the perspective of the server, since the extra __Invariant form entries will be ignored in model binding. The main difference is that the generated HTML will not include incorrectly-formatted value attributes.

Only setting SuppressCultureInvariantFormValueFormatting means the both the HTML generation and model binding will behave exactly as they already do today (since model binding logic only changes when __Invariant entries are included in the form submission).

So, AFAICT, the behavior won't get worse if you only set one of these options. Either you will get a slight improvement (generating better HTML), or no difference in behavior at all.

Should we instead have one MvcOptions.SuppressCultureInvariantRoundTripping option❔

This is what I was hoping to do initially, but I couldn't find a good way to pass an MvcOptions through to the DefaultHtmlGenerator or InputTagHelper. It's not as simple as adding a new constructor (like I did for FormValueProvider and FormValueProviderFactory), because they're instantiated with DI and multiple constructors aren't permitted in that scenario.

[brunolins16]

Only setting SuppressCultureInvariantFormValueFormatting means the both the HTML generation and model binding will behave exactly as they already do today (since model binding logic only changes when __Invariant entries are included in the form submission).

So, AFAICT, the behavior won't get worse if you only set one of these options. Either you will get a slight improvement (generating better HTML), or no difference in behavior at all.

So, in this case, do we really need the MvcOptions.SuppressCultureInvariantFormModelBinding?

[MackinnonBuck]

So, in this case, do we really need the MvcOptions.SuppressCultureInvariantFormModelBinding?

True, we might be able to go without it. I suppose the only use I can think of for it would be if you want the new behavior for generating correctly-formatted HTML value attribute strings, but not the new model binding behavior (in case someone was relying on the old behavior). Whether this is a real scenario, I'm not sure. And you could still set SuppressCultureInvariantFormValueFormatting to true as a workaround if you're willing to live with the existing value attribute formatting bug.

EDIT: To cover the above scenario, we could possibly have one enum option, maybe something like HtmlHelperOptions.FormInputFormattingMode:

enum FormInputFormattingMode
{
    // The new behavior in its entirety.
    Default,

    // The new form value attribute formatting, but no hidden "__Invariant" input generation.
    ExcludeCultureInvariantFormRequestEntries,

    // The old behavior in its entirety.
    AlwaysUseCurrentCulture,
}

Or we could use a [Flags] enum which would let the customer opt-out of any combination of the two changes to HTML generation.

[brunolins16]

EDIT: To cover the above scenario, we could possibly have one enum option, maybe something like HtmlHelperOptions.FormInputFormattingMode:

I don't know much about HtmlHelperOptions but this option is very interesting.

[MackinnonBuck]

HtmlHelperOptions is just where the existing options controlling HTML generation live. It's where the SuppressCultureInvariantFormValueFormatting property exists as the PR currently stands. So we would essentially be replacing that with an enum property and removing SuppressCultureInvariantFormModelBinding.

[MackinnonBuck]

Merging now so this fix makes the cutoff - I'll open a follow-up PR that addresses the remaining nits.

[bassem-mf]

This change broke a lot of my CSS. The ':last-child' CSS selector no longer applies to the elements that have the "__Invariant" input added after them.

[ghost]

Hi @bassem-mf. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[shapeh]

What do to when you create the input fields manually - no __Invariant is created? The model binding still fails in 2023 (using latest bits 7.0.2).

<input type="number" value="1.2" step="0.1"/>

In model binding this gets converted to 12 instead of 1.2.