Add support for minimized attributes in TagHelpers. #372

NTaylorMullen · 2015-05-01T23:56:45Z

Updated the Razor parser to understand minimized attributes instead of just treating them like plain text. This just involved encompassing minimized attributes in their own blocks just like the other attributes found on the HTML tag.
Updated TagHelperParseTreeRewriter to only accept minimized attributes for unbound attributes.
Updated IReadOnlyTagHelperAttribute/TagHelperAttribute to have a Minimized property to indicate that an attribute was minimized.
Updated parser level block structures to represent minimized attributes as null syntax tree nodes.
Updated chunk level structures to represent minimized attributes as null chunks.
Added parse level rewriting tests to validate new TagHelper rewritten structures for minimized attributes.
Updated existing parser tests to understand minimized attributes.
Added codegen test to validate understanding of minimized attributes.
Added TagHelperExecutionContext tests to validate maintaining of runtime TagHelperOutput tests.
Refactored part of the TagHelperParseTreeRewriterTest file into a base class file so we can make better rewriting tests.
TagHelper attributes with no value cannot be used in TagHelpers #220

NTaylorMullen · 2015-05-01T23:57:04Z

dougbu · 2015-05-02T00:09:54Z

Updated TagHelperParseTreeRewriter to only accept minimized attributes for unbound attributes.

why aren't minimized attributes handled? bound and unbound attributes use the same TagHelperAttribute class.

bound value of a string property named propertyName with a corresponding minimized attribute in Razor source is propertyName. bound value of a bool or bool? property with a corresponding minimized attribute in Razor source is true. any other property type is an error.

so far, this seems like #220 part 1 of at least 2.

NTaylorMullen · 2015-05-02T00:12:27Z

@dougbu #220 (comment)

dougbu · 2015-05-02T00:15:19Z

src/Microsoft.AspNet.Razor.Runtime/TagHelpers/TagHelperExecutionContext.cs

+        /// Tracks the minimized HTML attribute in <see cref="AllAttributes"/> and <see cref="HTMLAttributes"/>.
+        /// </summary>
+        /// <param name="name">The minimized HTML attribute name.</param>
+        public void AddHtmlAttribute([NotNull] string name)


this will confuse everyone. suggest either leaving out this "helper" or giving it a name that hints at minimization.

TagHelperExecutionContext isn't seen by users. It's only used by codegen.

this method will confuse us

I can't leave out the helper because it needs to do specific work to add a minimized attribute. As for renaming: I don't feel like it warrants an entirely new method name for a different form of an HTML attribute, that to me makes less sense. Going to leave as is.

This warrants a different name because it's doing something different. In particular, the effect is not the same as a call to AddHtmlAttribute("attribute-name", value: null) (the basic expectation of overloads). So please rename it to AddMinimizedAttribute() or AddMinimizedHtmlAttribute().

dougbu · 2015-05-02T01:04:13Z

@dougbu #220 (comment)

ok though I don't see anything like an understandable / helpful error message if a user writes <form asp-anti-forgery >.... what exactly happens in that case from a Razor author perspective? where is the error case tested?

dougbu · 2015-05-02T01:04:21Z

⌚

NTaylorMullen · 2015-05-02T06:58:42Z

In response to #372 (comment) : tons of validating tests in the TagHelperBlockRewriterTest.cs file. Razor author sees:

Attribute 'asp-anti-forgerery' on tag helper element 'form' requires a value. Tag helper bound attributes of type 'System.Boolean' cannot be empty or contain only whitespace.

pranavkm · 2015-05-04T14:46:54Z

src/Microsoft.AspNet.Razor.Runtime/TagHelpers/TagHelperAttribute.cs

+        /// <remarks>If <c>true</c>, <see cref="Value"/> will be ignored.</remarks>
+        public bool Minimized { get; set; }
+
+


dnfclas · 2015-05-06T18:07:54Z

@NTaylorMullen, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

NTaylorMullen · 2015-05-06T18:35:38Z

Updated.

dougbu · 2015-05-11T04:07:08Z

src/Microsoft.AspNet.Razor/Generator/GeneratedTagHelperContext.cs

@@ -58,6 +59,11 @@ public GeneratedTagHelperContext()
        public string ExecutionContextAddTagHelperAttributeMethodName { get; set; }

        /// <summary>
+        /// The name of the <see cref="ExecutionContextTypeName"/> method used to add minimized HTML attributes.
+        /// </summary>
+        public string ExecutionContextAddMinimizedHtmlAttributeMethodName { get; set; }


So there's going to be an MVC reaction PR?

dougbu · 2015-05-11T05:09:41Z

⌚ for less tests to wade through

NTaylorMullen · 2015-05-11T05:54:41Z

Updated. Addressed all except #372 (comment)

dougbu · 2015-05-11T15:15:37Z

src/Microsoft.AspNet.Razor/Parser/HtmlMarkupParser.Block.cs

@@ -471,11 +471,18 @@ private void BeforeAttribute()
                return;
            }

+            // Minimized attribute


This comment remains misplaced. Should be two lines down.

dougbu · 2015-05-11T16:35:38Z

⌚

NTaylorMullen · 2015-05-11T22:04:34Z

Updated.

NTaylorMullen · 2015-05-15T05:50:17Z

This has been open too long, Will get this in tomorrow afternoon and if more comments come in I will address them.

Current review commit: 2097006

I'm rebasing/squashing for the push so you can inspect the above commit for changes applied in the last CR iteration if comments are needed.

dougbu · 2015-05-15T18:22:24Z

src/Microsoft.AspNet.Razor/Generator/Compiler/CodeBuilder/CSharp/CSharpTagHelperCodeRenderer.cs

@@ -202,6 +202,13 @@ protected virtual string GenerateUniqueId()
                    // First attribute wins, even if there's duplicates.
                    var attributeValueChunk = matchingAttributes.First().Value;

+                    // Minimized attributes are not valid for bound attributes. There will be an error logged for this
+                    // bound attribute already so we can skip.


Please make this comment a bit more informative, describing what logs the error. "There will be" is just too vague.

dougbu · 2015-05-15T18:49:22Z

after addressing the few remaining comments

- Updated the Razor parser to understand minimized attributes instead of just treating them like plain text. This just involved encompassing minimized attributes in their own blocks just like the other attributes found on the HTML tag. - Updated TagHelperParseTreeRewriter to only accept minimized attributes for unbound attributes. - Updated IReadOnlyTagHelperAttribute/TagHelperAttribute to have a Minimized property to indicate that an attribute was minimized. - Updated parser level block structures to represent minimized attributes as null syntax tree nodes. - Updated chunk level structures to represent minimized attributes as null chunks. #220

- Added parse level rewriting tests to validate new TagHelper rewritten structures for minimized attributes. - Updated existing parser tests to understand minimized attributes. - Added codegen test to validate understanding of minimized attributes. - Added TagHelperExecutionContext tests to validate maintaining of runtime TagHelperOutput tests. - Refactored part of the TagHelperParseTreeRewriterTest file into a base class file so we can make better rewriting tests. #220

ghost added the cla-already-signed label May 1, 2015

dougbu reviewed May 2, 2015
View reviewed changes

pranavkm reviewed May 4, 2015
View reviewed changes

dnfclas added the cla-signed label May 6, 2015

NTaylorMullen force-pushed the nimullen/novalueattrs.220 branch from ca1d840 to 3b1981c Compare May 6, 2015 18:34

dougbu reviewed May 11, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/novalueattrs.220 branch 2 times, most recently from 023fedd to 29a2359 Compare May 11, 2015 22:04

NTaylorMullen force-pushed the nimullen/novalueattrs.220 branch from 29a2359 to 2097006 Compare May 13, 2015 00:45

NTaylorMullen mentioned this pull request May 13, 2015

Added the ability for the WriteTagHelperAsync in RazorPage.cs to understand minimized attributes. aspnet/Mvc#2555

Merged

dougbu reviewed May 15, 2015
View reviewed changes

N. Taylor Mullen added 2 commits May 15, 2015 12:23

NTaylorMullen force-pushed the nimullen/novalueattrs.220 branch from 2097006 to 0882ff4 Compare May 15, 2015 19:23

NTaylorMullen merged commit 0882ff4 into dev May 15, 2015

NTaylorMullen deleted the nimullen/novalueattrs.220 branch May 15, 2015 19:25

		/// <remarks>If <c>true</c>, <see cref="Value"/> will be ignored.</remarks>
		public bool Minimized { get; set; }

Add support for minimized attributes in TagHelpers. #372

Add support for minimized attributes in TagHelpers. #372

Uh oh!

Conversation

NTaylorMullen commented May 1, 2015

Uh oh!

NTaylorMullen commented May 1, 2015

Uh oh!

dougbu commented May 2, 2015

Uh oh!

NTaylorMullen commented May 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 2, 2015

Uh oh!

dougbu commented May 2, 2015

Uh oh!

NTaylorMullen commented May 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfclas commented May 6, 2015

Uh oh!

NTaylorMullen commented May 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 11, 2015

Uh oh!

NTaylorMullen commented May 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 11, 2015

Uh oh!

NTaylorMullen commented May 11, 2015

Uh oh!

NTaylorMullen commented May 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 15, 2015

Uh oh!

Uh oh!