From e4049c07ebf8ae46e1f32472a0fd9d4d56eae911 Mon Sep 17 00:00:00 2001 From: Ajay Bhargav Baaskaran Date: Mon, 27 Jul 2015 14:56:06 -0700 Subject: [PATCH] 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 | 143 +++++++++--------- .../HtmlGenerationTest.cs | 3 + ...e.AttributesWithBooleanValues.Encoded.html | 14 ++ ...tion_Home.AttributesWithBooleanValues.html | 15 ++ .../RazorPageTest.cs | 118 +++++++++++++++ .../HtmlGeneration_HomeController.cs | 5 + .../AttributesWithBooleanValues.cshtml | 20 +++ 7 files changed, 248 insertions(+), 70 deletions(-) create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.Encoded.html create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.html create mode 100644 test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/AttributesWithBooleanValues.cshtml diff --git a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs index 29c1e2a9e9..6066c64a1d 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs @@ -519,96 +519,62 @@ namespace Microsoft.AspNet.Mvc.Razor [NotNull] PositionTagged 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 val = attrVal.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 + var attributeValue = values[i]; + var positionTaggedAttributeValue = attributeValue.Value; - 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 - // value might be a bool. If the value is the bool 'true' we want to write the attribute name - // instead of the string 'true'. If the value is the bool 'false' we don't want to write anything. - // 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); - } + 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 // 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); - // 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); - } + var stringValue = positionTaggedAttributeValue.Value as string; - EndContext(); - wroteSomething = true; - } - if (wroteSomething) - { - WritePositionTaggedLiteral(writer, suffix); + WriteAttributeValue(writer, attributeValue, stringValue, sourceLength); } } + + 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); diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs index c06f3b3723..c58c7e687e 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/HtmlGenerationTest.cs +++ b/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 diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.Encoded.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.Encoded.html new file mode 100644 index 0000000000..db26d46264 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.Encoded.html @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.html new file mode 100644 index 0000000000..1882d6d649 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.AttributesWithBooleanValues.html @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Razor.Test/RazorPageTest.cs b/test/Microsoft.AspNet.Mvc.Razor.Test/RazorPageTest.cs index e587a82e20..5bd857b149 100644 --- a/test/Microsoft.AspNet.Mvc.Razor.Test/RazorPageTest.cs +++ b/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("prefix", 0), + new PositionTagged("suffix", 10), + new AttributeValue(new PositionTagged("", 6), + new PositionTagged("true", 6), + literal: false)); + }); + var context = new Mock(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 WriteAttributeData + { + get + { + // AttributeValues, ExpectedOutput + return new TheoryData + { + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged("", 9), + new PositionTagged(true, 9), + literal: false) + }, + "someattr=HtmlEncode[[someattr]]" + }, + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged("", 9), + new PositionTagged(false, 9), + literal: false) + }, + "" + }, + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged("", 9), + new PositionTagged(null, 9), + literal: false) + }, + "" + }, + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged(" ", 9), + new PositionTagged(false, 11), + literal: false) + }, + "someattr= HtmlEncode[[False]]" + }, + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged(" ", 9), + new PositionTagged(null, 11), + literal: false) + }, + "someattr=" + }, + { + new AttributeValue[] { + new AttributeValue( + new PositionTagged(" ", 9), + new PositionTagged(true, 11), + literal: false), + new AttributeValue( + new PositionTagged(" ", 15), + new PositionTagged("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("someattr=", 0); + var suffix = new PositionTagged("", 0); + + // Act + page.WriteAttributeTo(writer, "someattr", prefix, suffix, attributeValues); + + // Assert + Assert.Equal(expectedOutput, writer.ToString()); + } + [Fact] public async Task Write_WithHtmlString_WritesValueWithoutEncoding() { diff --git a/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_HomeController.cs b/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_HomeController.cs index cb2d245258..4fa3387783 100644 --- a/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_HomeController.cs +++ b/test/WebSites/HtmlGenerationWebSite/Controllers/HtmlGeneration_HomeController.cs @@ -195,5 +195,10 @@ namespace HtmlGenerationWebSite.Controllers { return View(); } + + public IActionResult AttributesWithBooleanValues() + { + return View(); + } } } diff --git a/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/AttributesWithBooleanValues.cshtml b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/AttributesWithBooleanValues.cshtml new file mode 100644 index 0000000000..c34d4286ea --- /dev/null +++ b/test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/AttributesWithBooleanValues.cshtml @@ -0,0 +1,20 @@ +@{ + var trueVar = true; + var falseVar = false; + var stringVar = "value"; + string nullVar = null; +} + + + + + + + + + + + + + + \ No newline at end of file