asp-format is not respected on input[type=hidden] fields

[Haplois]

I have a form with couple of hidden fields. One of them is DateTime?, this field is not rendered in the RFC3389 format. Furthermore, if I define a asp-format attribute, tag helper simply dismisses this.

To fix this, I forked the repository and started to make code changes. I didn't issued a pull request since it felt like a huge change. Is it OK?

Changes are here: Haplois@623eeb3

[Eilon]

@dougbu can you have a look? I'm not super familiar with what the intended interaction of these properties and behaviors is.

[dougbu]

@Haplois this would be a breaking change since formats, especially default formats, have not applied to hidden fields in any previous version. If it were just adding asp-format support for hidden fields i.e. if GetFormat() was never called in the hidden case, I'd be less concerned. (Of course, adding the format parameter to IHtmlGenerator.GenerateHidden() is fine.)

More generally, please do not call the existing InputTagHelper.GenerateTextBox() method or rename it. That method brings in too many additional behaviour changes.

But your commit is close to what we'd like to see in a PR as long as we can do it in 1.1.0...

@Eilon in the past, Html.Hidden() and Html.HiddenFor<T>() didn't have a format parameter. The difference here is the <input> tag helper appears to support asp-format regardless of the type value. But supporting asp-format w/ type='hidden' would be a different behaviour from 1.0.0. As long as you're fine w/ that small behaviour change and we limit things to just that behaviour change, this commit is pretty close.

[Haplois]

@dougbu since InputTagHelper.GenerateTextBox() is a private method and I generalized it little bit I renamed it and thought it is OK.

I reverted that change in this commit 3bce280.

[Haplois]

BTW, since inputType and inputTypeHint variables in the InputTagHelper.Process() are only used in InputTagHelper.GenerateTextBox() is it OK there (or into the default statement of the switch)?

[dougbu]

Those variables are fine where they are but I'm not positive what you mean by "is it 🆗 there"?

[Haplois]

@dougbu somehow I accidentally deleted a part of message before I post it. What I tried to write was: "is it OK to move it there"

[dougbu]

Let's not do additional refactoring in this PR

[Haplois]

Should I issue a pull request?

[dougbu]

@Haplois let's have a look I think. @Eilon any concern about the slight behaviour change from 1.0.0?

[Eilon]

@dougbu regarding this comment>

@Eilon in the past, Html.Hidden() and Html.HiddenFor() didn't have a format parameter. The difference here is the tag helper appears to support asp-format regardless of the type value. But supporting asp-format w/ type='hidden' would be a different behaviour from 1.0.0. As long as you're fine w/ that small behaviour change and we limit things to just that behaviour change, this commit is pretty close.

I'm not sure I understand what the difference in behavior is. You're saying the Hidden HTML Helpers never supported format. But the Hidden Tag Helpers do support format? And what's the change? I.e. what's the before after that's changing?

Hidden HTML Helper:

• 1.0.0 --> Doesn't support format
• 1.1.0 --> Add support for format (?)

Hidden Tag Helper:

• 1.0.0 --> Already supports format (?)
• 1.1.0 --> No change (?)

[dougbu]

@Eilon sorry for throwing in the HTML helpers. That just served to make my comment more confusing.

The 1.0.0 <input> tag helper takes after the HTML helpers though the asp-format attribute exists and can be bound regardless of the type attribute generated. So,

Hidden HTML helper

No change.

<input/> tag helper

• 1.0.0 -> has asp-format but that attribute is ignored when the chosen type is 'hidden'
• 1.1.0 -> add support for (will no longer ignore) asp-format when the chosen type is 'hidden'

[Eilon]

Seems safe enough to add. Let's do it.

[Haplois]

I issued the pull request. Sorry for delay. I'm on vocation.

[dougbu]

4bda1cb and 34a4c8c
Thanks very much for 4bda1cb @Haplois!