Razor boolean and null attribute special case handled correctly

- Issue #2769
 - Special case is only applied to null and bool value with no surrounding
   whitespace

src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs

@@ -519,96 +519,62 @@ namespace Microsoft.AspNet.Mvc.Razor
             [NotNull] PositionTagged<string> suffix,
             params AttributeValue[] values)
         {
-            var first = true;
-            var wroteSomething = false;
             if (values.Length == 0)
             {
-                // Explicitly empty attribute, so write the prefix and suffix
+                // Explicitly empty attribute, so write the prefix
                 WritePositionTaggedLiteral(writer, prefix);
-                WritePositionTaggedLiteral(writer, suffix);
+            }
+            else if (values.Length == 1 && values[0].Prefix == "" &&
+                (values[0].Value.Value is bool || values[0].Value.Value == null))
+            {
+                // Value is either null or a bool with no prefix.
+                var attributeValue = values[0];
+                var positionTaggedAttributeValue = attributeValue.Value;
+
+                if (positionTaggedAttributeValue.Value == null || !(bool)positionTaggedAttributeValue.Value)
+                {
+                    // The value is null or just the bool 'false', don't write anything.
+                    return;
+                }
+
+                WritePositionTaggedLiteral(writer, prefix);
+
+                var sourceLength = suffix.Position - positionTaggedAttributeValue.Position;
+
+                // The value is just the bool 'true', write the attribute name instead of the string 'True'.
+                WriteAttributeValue(writer, attributeValue, name, sourceLength);
             }
             else
             {
+                // This block handles two cases.
+                // 1. Single value with prefix.
+                // 2. Multiple values with or without prefix.
+                WritePositionTaggedLiteral(writer, prefix);
                 for (var i = 0; i < values.Length; i++)
                 {
-                    var attrVal = values[i];
+                    var attributeValue = values[i];
-                    var val = attrVal.Value;
+                    var positionTaggedAttributeValue = attributeValue.Value;
-                    var next = i == values.Length - 1 ?
-                        suffix : // End of the list, grab the suffix
-                        values[i + 1].Prefix; // Still in the list, grab the next prefix
 
-                    if (val.Value == null)
+                    if (positionTaggedAttributeValue.Value == null)
                     {
                         // Nothing to write
                         continue;
                     }
 
-                    // The special cases here are that the value we're writing might already be a string, or that the
+                    var next = i == values.Length - 1 ?
-                    // value might be a bool. If the value is the bool 'true' we want to write the attribute name
+                        suffix : // End of the list, grab the suffix
-                    // instead of the string 'true'. If the value is the bool 'false' we don't want to write anything.
+                        values[i + 1].Prefix; // Still in the list, grab the next prefix
-                    // Otherwise the value is another object (perhaps an HtmlString) and we'll ask it to format itself.
-                    string stringValue;
-
-                    // Intentionally using is+cast here for performance reasons. This is more performant than as+bool?
-                    // because of boxing.
-                    if (val.Value is bool)
-                    {
-                        if ((bool)val.Value)
-                        {
-                            stringValue = name;
-                        }
-                        else
-                        {
-                            continue;
-                        }
-                    }
-                    else
-                    {
-                        stringValue = val.Value as string;
-                    }
-
-                    if (first)
-                    {
-                        WritePositionTaggedLiteral(writer, prefix);
-                        first = false;
-                    }
-
-                    if (!string.IsNullOrEmpty(attrVal.Prefix))
-                    {
-                        WritePositionTaggedLiteral(writer, attrVal.Prefix);
-                    }
 
                     // Calculate length of the source span by the position of the next value (or suffix)
-                    var sourceLength = next.Position - attrVal.Value.Position;
+                    var sourceLength = next.Position - attributeValue.Value.Position;
 
-                    BeginContext(attrVal.Value.Position, sourceLength, isLiteral: attrVal.Literal);
+                    var stringValue = positionTaggedAttributeValue.Value as string;
-                    // The extra branching here is to ensure that we call the Write*To(string) overload where
-                    // possible.
-                    if (attrVal.Literal && stringValue != null)
-                    {
-                        WriteLiteralTo(writer, stringValue);
-                    }
-                    else if (attrVal.Literal)
-                    {
-                        WriteLiteralTo(writer, val.Value);
-                    }
-                    else if (stringValue != null)
-                    {
-                        WriteTo(writer, stringValue);
-                    }
-                    else
-                    {
-                        WriteTo(writer, val.Value);
-                    }
 
-                    EndContext();
+                    WriteAttributeValue(writer, attributeValue, stringValue, sourceLength);
-                    wroteSomething = true;
-                }
-                if (wroteSomething)
-                {
-                    WritePositionTaggedLiteral(writer, suffix);
                 }
             }
+
+            WritePositionTaggedLiteral(writer, suffix);
         }
 
         public virtual string Href([NotNull] string contentPath)
@@ -621,6 +587,43 @@ namespace Microsoft.AspNet.Mvc.Razor
             return _urlHelper.Content(contentPath);
         }
 
+        private void WriteAttributeValue(
+            TextWriter writer,
+            AttributeValue attributeValue,
+            string stringValue,
+            int sourceLength)
+        {
+            var positionTaggedAttributeValue = attributeValue.Value;
+
+            if (!string.IsNullOrEmpty(attributeValue.Prefix))
+            {
+                WritePositionTaggedLiteral(writer, attributeValue.Prefix);
+            }
+
+            BeginContext(attributeValue.Value.Position, sourceLength, isLiteral: attributeValue.Literal);
+
+            // The extra branching here is to ensure that we call the Write*To(string) overload where
+            // possible.
+            if (attributeValue.Literal && stringValue != null)
+            {
+                WriteLiteralTo(writer, stringValue);
+            }
+            else if (attributeValue.Literal)
+            {
+                WriteLiteralTo(writer, positionTaggedAttributeValue.Value);
+            }
+            else if (stringValue != null)
+            {
+                WriteTo(writer, stringValue);
+            }
+            else
+            {
+                WriteTo(writer, positionTaggedAttributeValue.Value);
+            }
+
+            EndContext();
+        }
+
         private void WritePositionTaggedLiteral(TextWriter writer, string value, int position)
         {
             BeginContext(position, value.Length, isLiteral: true);

test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs

@@ -53,6 +53,8 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         [InlineData("Image", null)]
         // Testing InputTagHelper with File
         [InlineData("Input", null)]
+        // Testing attribute values with boolean and null values
+        [InlineData("AttributesWithBooleanValues", null)]
         public async Task HtmlGenerationWebSite_GeneratesExpectedResults(string action, string antiforgeryPath)
         {
             // This uses FileVersionProvider which uses Uri.TryCreate - https://github.com/aspnet/External/issues/21
@@ -117,6 +119,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
         [InlineData("OrderUsingHtmlHelpers", "/HtmlGeneration_Order/Submit")]
         [InlineData("Product", null)]
         [InlineData("Script", null)]
+        [InlineData("AttributesWithBooleanValues", null)]
         public async Task HtmlGenerationWebSite_GenerateEncodedResults(string action, string antiforgeryPath)
         {
             // This uses FileVersionProvider which uses Uri.TryCreate - https://github.com/aspnet/External/issues/21

test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.Encoded.html

@@ -0,0 +1,14 @@
+<input checked="HtmlEncode[[checked]]" />
+<input checked="HtmlEncode[[checked]]" />
+<input />
+<input />
+<input checked="  HtmlEncode[[True]]    " />
+<input checked="  HtmlEncode[[False]]    " />
+<input checked="    HtmlEncode[[value]]: HtmlEncode[[True]]   " />
+<input checked="    value: HtmlEncode[[False]]   " />
+<input checked="HtmlEncode[[True]] HtmlEncode[[True]]" />
+<input checked="   HtmlEncode[[False]]  HtmlEncode[[True]]" />
+<input />
+<input checked="" />
+<input checked="   " />
+<input checked=" HtmlEncode[[value]] HtmlEncode[[True]]" />

test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.html

@@ -0,0 +1,15 @@
+
+<input checked="checked" />
+<input checked="checked" />
+<input />
+<input />
+<input checked="  True    " />
+<input checked="  False    " />
+<input checked="    value: True   " />
+<input checked="    value: False   " />
+<input checked="True True" />
+<input checked="   False  True" />
+<input />
+<input checked="" />
+<input checked="   " />
+<input checked=" value True" />

test/Microsoft.AspNet.Mvc.Razor.Test/RazorPageTest.cs

@@ -686,6 +686,37 @@ namespace Microsoft.AspNet.Mvc.Razor
             context.Verify();
         }
 
+        [Fact]
+        public async Task WriteAttribute_WithBoolValue_CallsBeginAndEndContext_OnPageExecutionListenerContext()
+        {
+            // Arrange
+            var page = CreatePage(p =>
+            {
+                p.HtmlEncoder = new CommonTestEncoder();
+                p.WriteAttribute("href",
+                                 new PositionTagged<string>("prefix", 0),
+                                 new PositionTagged<string>("suffix", 10),
+                                 new AttributeValue(new PositionTagged<string>("", 6),
+                                                    new PositionTagged<object>("true", 6),
+                                                    literal: false));
+            });
+            var context = new Mock<IPageExecutionContext>(MockBehavior.Strict);
+            var sequence = new MockSequence();
+            context.InSequence(sequence).Setup(f => f.BeginContext(0, 6, true)).Verifiable();
+            context.InSequence(sequence).Setup(f => f.EndContext()).Verifiable();
+            context.InSequence(sequence).Setup(f => f.BeginContext(6, 4, false)).Verifiable();
+            context.InSequence(sequence).Setup(f => f.EndContext()).Verifiable();
+            context.InSequence(sequence).Setup(f => f.BeginContext(10, 6, true)).Verifiable();
+            context.InSequence(sequence).Setup(f => f.EndContext()).Verifiable();
+            page.PageExecutionContext = context.Object;
+
+            // Act
+            await page.ExecuteAsync();
+
+            // Assert
+            context.Verify();
+        }
+
         [Fact]
         public async Task WriteAttribute_CallsBeginAndEndContext_OnPrefixAndSuffixValues()
         {
@@ -711,6 +742,93 @@ namespace Microsoft.AspNet.Mvc.Razor
             context.Verify();
         }
 
+        public static TheoryData<AttributeValue[], string> WriteAttributeData
+        {
+            get
+            {
+                // AttributeValues, ExpectedOutput
+                return new TheoryData<AttributeValue[], string>
+                {
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("", 9),
+                                new PositionTagged<object>(true, 9),
+                                literal: false)
+                        },
+                        "someattr=HtmlEncode[[someattr]]"
+                    },
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("", 9),
+                                new PositionTagged<object>(false, 9),
+                                literal: false)
+                        },
+                        ""
+                    },
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("", 9),
+                                new PositionTagged<object>(null, 9),
+                                literal: false)
+                        },
+                        ""
+                    },
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("  ", 9),
+                                new PositionTagged<object>(false, 11),
+                                literal: false)
+                        },
+                        "someattr=  HtmlEncode[[False]]"
+                    },
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("  ", 9),
+                                new PositionTagged<object>(null, 11),
+                                literal: false)
+                        },
+                        "someattr="
+                    },
+                    {
+                        new AttributeValue[] {
+                            new AttributeValue(
+                                new PositionTagged<string>("  ", 9),
+                                new PositionTagged<object>(true, 11),
+                                literal: false),
+                            new AttributeValue(
+                                new PositionTagged<string>("  ", 15),
+                                new PositionTagged<object>("abcd", 17),
+                                literal: true),
+                        },
+                        "someattr=  HtmlEncode[[True]]  abcd"
+                    },
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(WriteAttributeData))]
+        public void WriteAttributeTo_WritesAsExpected(AttributeValue[] attributeValues, string expectedOutput)
+        {
+            // Arrange
+            var page = CreatePage(p => { });
+            page.HtmlEncoder = new CommonTestEncoder();
+            var writer = new StringWriter();
+            var prefix = new PositionTagged<string>("someattr=", 0);
+            var suffix = new PositionTagged<string>("", 0);
+
+            // Act
+            page.WriteAttributeTo(writer, "someattr", prefix, suffix, attributeValues);
+
+            // Assert
+            Assert.Equal(expectedOutput, writer.ToString());
+        }
+
         [Fact]
         public async Task Write_WithHtmlString_WritesValueWithoutEncoding()
         {

test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_HomeController.cs

@@ -195,5 +195,10 @@ namespace HtmlGenerationWebSite.Controllers
         {
             return View();
         }
+
+        public IActionResult AttributesWithBooleanValues()
+        {
+            return View();
+        }
     }
 }

test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/AttributesWithBooleanValues.cshtml

@@ -0,0 +1,20 @@
+@{
+    var trueVar = true;
+    var falseVar = false;
+    var stringVar = "value";
+    string nullVar = null;
+}
+<input checked="@true" />
+<input checked="@trueVar" />
+<input checked="@false" />
+<input checked="@falseVar" />
+<input checked="  @true    " />
+<input checked="  @falseVar    " />
+<input checked="    @stringVar: @trueVar   " />
+<input checked="    value: @false   " />
+<input checked="@true @trueVar" />
+<input checked="   @falseVar  @true" />
+<input checked="@null" />
+<input checked="  @nullVar" />
+<input checked="@nullVar   " />
+<input checked="  @null @stringVar @trueVar" />