Moving the generated hidden tags for checkbox to the end of the form #3311

ajaybhargavb · 2015-10-12T23:12:07Z

dougbu · 2015-10-13T04:45:06Z

src/Microsoft.AspNet.Mvc.Razor/TagHelpers/HiddenFormTagHelper.cs

+
+namespace Microsoft.AspNet.Mvc.Razor.TagHelpers
+{
+    [EditorBrowsable(EditorBrowsableState.Never)]


Why? For the EditorBrowsable bit?

So that it doesn't show up in intellisense.

dougbu · 2015-10-13T05:38:15Z

⌚

pranavkm · 2015-10-13T17:33:16Z

src/Microsoft.AspNet.Mvc.Razor.Host/MvcRazorHost.cs

@@ -42,6 +42,10 @@ public class MvcRazorHost : RazorEngineHost, IMvcRazorHost
            {
                LookupText = "Microsoft.AspNet.Mvc.Razor.TagHelpers.UrlResolutionTagHelper, Microsoft.AspNet.Mvc.Razor"
            },
+            new AddTagHelperChunk


Spoke to @Eilon - instead of doing this, move it to Microsoft.AspNet.Mvc.TagHelpers. Users will import it via _ViewImports.

@pranavkm @Eilon if we do this, @Html.CheckBox() and @Html.CheckBoxFor() will not work correctly inside a regular <form> element.

pranavkm · 2015-10-13T17:47:06Z

⌚

ajaybhargavb · 2015-10-14T01:30:45Z

Updated.

dougbu · 2015-10-14T05:08:38Z

src/Microsoft.AspNet.Mvc.TagHelpers/HiddenFormTagHelper.cs

+    /// </summary>
+    [EditorBrowsable(EditorBrowsableState.Advanced)]
+    [HtmlTargetElement("form")]
+    public class HiddenFormTagHelper : TagHelper


Need to be more general w/ these descriptions and names. Check boxes are one use of the infrastructure this helper provides. Perhaps ... to generate deferred content before the </form> end tag. and RenderBeforeFormEndTagTagHelper?

Per our discussion yesterday, this shouldn't say anything at all about anything being deferred. It's really entirely just about rendering stuff before the end of the form.

dougbu · 2015-10-14T05:49:14Z

⌚

NTaylorMullen · 2015-10-14T16:37:44Z

src/Microsoft.AspNet.Mvc.TagHelpers/HiddenFormTagHelper.cs

+    /// <summary>
+    /// <see cref="ITagHelper"/> implementation targeting all form elements to generate hidden tag for checkboxes.
+    /// </summary>
+    [EditorBrowsable(EditorBrowsableState.Advanced)]


EditorBrowsableState.Advanced wont do anything. Is there a follow up bug to make Razor treat Advanced like Never?

This should just be Never.

Eilon · 2015-10-14T18:13:50Z

Looking really nice!

NTaylorMullen · 2015-10-14T18:26:01Z

...nctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.EmployeeList.html

@@ -94,5 +94,5 @@
 <option>1002</option>
 </select>
 </div>            <input type="submit" />
-</form></body>
+<input name="[0].Remote" type="hidden" value="false" /><input name="[1].Remote" type="hidden" value="false" /><input name="[2].Remote" type="hidden" value="false" /></form></body>


Not important here but at some point we should probably change the rendering to output void input tags.

@NTaylorMullen we test both with and without the slash. Are you suggesting we test only without?

Nooo, testing isn't what I'm referring to. I'm saying that we should always generate without.

Thought we decided long ago not to mess w/ how the user wrote an element in the .cshtml file?

These aren't what the user wrote though. These are generated to pass data to the server on post.

Ah you're talking specifically about revisiting our decision to write these additional <input/> (or <input>) elements identically to the check boxes. Right?

Well, I'm curious what has changed since we made that decision but let's take this offline. Unrelated to @ajaybhargavb's work here.

We should be generating <self-closing /> tags to be XHTML compliant, which some people care about. It's never wrong, is it?

dougbu · 2015-10-14T20:09:18Z

@ajaybhargavb is this updated and ready for another review round?

ajaybhargavb · 2015-10-14T20:10:34Z

@dougbu yes. Forgot to mention.

ajaybhargavb · 2015-10-14T20:10:43Z

Updated.

dougbu · 2015-10-14T20:14:05Z

src/Microsoft.AspNet.Mvc.TagHelpers/RenderAtEndOfFormTagHelper.cs

+            var formContext = ViewContext.FormContext;
+            if (formContext.HasEndOfFormContent)
+            {
+                foreach (var hiddenTag in formContext.EndOfFormContent)


hiddenTag -> content

dougbu · 2015-10-14T20:35:09Z

with just a couple of teensy changes

Eilon · 2015-10-14T22:16:47Z

test/Microsoft.AspNet.Mvc.TagHelpers.Test/RenderAtEndOfFormTagHelperTest.cs

+
+        [Theory]
+        [MemberData(nameof(RenderAtEndOfFormTagHelperData))]
+        public async Task Process_AddsHiddenInputTag_FromFormData(List<TagBuilder> tagBuilderList, string expectedOutput)


Update test name to not say FormData

Eilon · 2015-10-14T22:26:46Z

Looking great, after tiny comments.

…form - #2994 - Affects both HtmlHelper and TagHelper scenarios - Checkboxes not enclosed in a form will generate inline hidden tags - Added necessary properties to FormContext - Added some functional and unit tests

dnfclas added the cla-already-signed label Oct 12, 2015

dougbu reviewed Oct 13, 2015
View reviewed changes

pranavkm reviewed Oct 13, 2015
View reviewed changes

dougbu reviewed Oct 14, 2015
View reviewed changes

NTaylorMullen reviewed Oct 14, 2015
View reviewed changes

ajaybhargavb force-pushed the checkbox-order branch 2 times, most recently from b7c9d9e to 6f5bdfe Compare October 14, 2015 18:51

dougbu reviewed Oct 14, 2015
View reviewed changes

Eilon reviewed Oct 14, 2015
View reviewed changes

ajaybhargavb force-pushed the checkbox-order branch from 6f5bdfe to d40e661 Compare October 14, 2015 22:50

ajaybhargavb merged commit d40e661 into dev Oct 14, 2015

ajaybhargavb deleted the checkbox-order branch October 14, 2015 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving the generated hidden tags for checkbox to the end of the form #3311

Moving the generated hidden tags for checkbox to the end of the form #3311

ajaybhargavb commented Oct 12, 2015

dougbu Oct 13, 2015

pranavkm Oct 13, 2015

ajaybhargavb Oct 13, 2015

dougbu commented Oct 13, 2015

pranavkm Oct 13, 2015

dougbu Oct 13, 2015

pranavkm commented Oct 13, 2015

ajaybhargavb commented Oct 14, 2015

dougbu Oct 14, 2015

Eilon Oct 14, 2015

dougbu commented Oct 14, 2015

NTaylorMullen Oct 14, 2015

Eilon Oct 14, 2015

Eilon commented Oct 14, 2015

NTaylorMullen Oct 14, 2015

dougbu Oct 14, 2015

NTaylorMullen Oct 14, 2015

dougbu Oct 14, 2015

NTaylorMullen Oct 14, 2015

dougbu Oct 14, 2015

NTaylorMullen Oct 14, 2015

dougbu Oct 14, 2015

Eilon Oct 14, 2015

dougbu commented Oct 14, 2015

ajaybhargavb commented Oct 14, 2015

ajaybhargavb commented Oct 14, 2015

dougbu Oct 14, 2015

dougbu commented Oct 14, 2015

Eilon Oct 14, 2015

Eilon commented Oct 14, 2015

Moving the generated hidden tags for checkbox to the end of the form #3311

Moving the generated hidden tags for checkbox to the end of the form #3311

Conversation

ajaybhargavb commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Oct 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Oct 13, 2015

ajaybhargavb commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Oct 14, 2015

ajaybhargavb commented Oct 14, 2015

ajaybhargavb commented Oct 14, 2015

Choose a reason for hiding this comment

dougbu commented Oct 14, 2015

Choose a reason for hiding this comment

Eilon commented Oct 14, 2015