Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Add support for minimized attributes in TagHelpers. #372

Merged
merged 2 commits into from
May 15, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,11 @@ public interface IReadOnlyTagHelperAttribute : IEquatable<IReadOnlyTagHelperAttr
/// Gets the value of the attribute.
/// </summary>
object Value { get; }

/// <summary>
/// Gets an indication whether the attribute is minimized or not.
/// </summary>
/// <remarks>If <c>true</c>, <see cref="Value"/> will be ignored.</remarks>
bool Minimized { get; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ public TagHelperAttribute(string name, object value)
/// </summary>
public object Value { get; set; }

/// <summary>
/// Gets or sets an indication whether the attribute is minimized or not.
/// </summary>
/// <remarks>If <c>true</c>, <see cref="Value"/> will be ignored.</remarks>
public bool Minimized { get; set; }

/// <summary>
/// Converts the specified <paramref name="value"/> into a <see cref="TagHelperAttribute"/>.
/// </summary>
Expand All @@ -61,7 +67,8 @@ public bool Equals(IReadOnlyTagHelperAttribute other)
return
other != null &&
string.Equals(Name, other.Name, StringComparison.OrdinalIgnoreCase) &&
Equals(Value, other.Value);
Minimized == other.Minimized &&
(Minimized || Equals(Value, other.Value));
}

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ internal TagHelperExecutionContext(string tagName, bool selfClosing)
/// <param name="tagName">The HTML tag name in the Razor source.</param>
/// <param name="selfClosing">
/// <see cref="bool"/> indicating whether or not the tag in the Razor source was self-closing.</param>
/// <param name="items">The collection of items used to communicate with other
/// <param name="items">The collection of items used to communicate with other
/// <see cref="ITagHelper"/>s</param>
/// <param name="uniqueId">An identifier unique to the HTML element this context is for.</param>
/// <param name="executeChildContentAsync">A delegate used to execute the child content asynchronously.</param>
Expand Down Expand Up @@ -133,6 +133,26 @@ public void Add([NotNull] ITagHelper tagHelper)
_tagHelpers.Add(tagHelper);
}

/// <summary>
/// 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 AddMinimizedHtmlAttribute([NotNull] string name)
{
HTMLAttributes.Add(
new TagHelperAttribute
{
Name = name,
Minimized = true
});
AllAttributes.Add(
new TagHelperAttribute
{
Name = name,
Minimized = true
});
}

/// <summary>
/// Tracks the HTML attribute in <see cref="AllAttributes"/> and <see cref="HTMLAttributes"/>.
/// </summary>
Expand Down Expand Up @@ -168,7 +188,7 @@ public Task ExecuteChildContentAsync()
/// </summary>
/// <returns>A <see cref="Task"/> that on completion returns the rendered child content.</returns>
/// <remarks>
/// Child content is only executed once. Successive calls to this method or successive executions of the
/// Child content is only executed once. Successive calls to this method or successive executions of the
/// returned <see cref="Task{TagHelperContent}"/> return a cached result.
/// </remarks>
public async Task<TagHelperContent> GetChildContentAsync()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ private void RenderBoundHTMLAttributes(IList<KeyValuePair<string, Chunk>> chunkA
var firstAttribute = matchingAttributes.First();
var attributeValueChunk = firstAttribute.Value;

// Minimized attributes are not valid for bound attributes. There will be an error for the bound
// attribute logged by TagHelperBlockRewriter already so we can skip.
if (attributeValueChunk == null)
{
continue;
}

var attributeValueRecorded = htmlAttributeValues.ContainsKey(attributeDescriptor.Name);

// Bufferable attributes are attributes that can have Razor code inside of them.
Expand Down Expand Up @@ -323,15 +330,21 @@ private void RenderUnboundHTMLAttributes(IEnumerable<KeyValuePair<string, Chunk>
// Build out the unbound HTML attributes for the tag builder
foreach (var htmlAttribute in unboundHTMLAttributes)
{
string textValue;
string textValue = null;
var isPlainTextValue = false;
var attributeValue = htmlAttribute.Value;
var isPlainTextValue = TryGetPlainTextValue(attributeValue, out textValue);

// HTML attributes are always strings. So if this value is not plain text i.e. if the value contains
// C# code, then we need to buffer it.
if (!isPlainTextValue)
// A null attribute value means the HTML attribute is minimized.
if (attributeValue != null)
{
BuildBufferedWritingScope(attributeValue, htmlEncodeValues: true);
isPlainTextValue = TryGetPlainTextValue(attributeValue, out textValue);

// HTML attributes are always strings. So if this value is not plain text i.e. if the value contains
// C# code, then we need to buffer it.
if (!isPlainTextValue)
{
BuildBufferedWritingScope(attributeValue, htmlEncodeValues: true);
}
}

// Execution contexts are a runtime feature, therefore no need to add anything to them.
Expand All @@ -340,27 +353,39 @@ private void RenderUnboundHTMLAttributes(IEnumerable<KeyValuePair<string, Chunk>
continue;
}

_writer
.WriteStartInstanceMethodInvocation(
ExecutionContextVariableName,
_tagHelperContext.ExecutionContextAddHtmlAttributeMethodName)
.WriteStringLiteral(htmlAttribute.Key)
.WriteParameterSeparator()
.WriteStartMethodInvocation(_tagHelperContext.MarkAsHtmlEncodedMethodName);

// If it's a plain text value then we need to surround the value with quotes.
if (isPlainTextValue)
// If we have a minimized attribute there is no value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't repeat the same comment with slightly different words.

if (attributeValue == null)
{
_writer.WriteStringLiteral(textValue);
_writer
.WriteStartInstanceMethodInvocation(
ExecutionContextVariableName,
_tagHelperContext.ExecutionContextAddMinimizedHtmlAttributeMethodName)
.WriteStringLiteral(htmlAttribute.Key)
.WriteEndMethodInvocation();
}
else
{
RenderBufferedAttributeValueAccessor(_writer);
}
_writer
.WriteStartInstanceMethodInvocation(
ExecutionContextVariableName,
_tagHelperContext.ExecutionContextAddHtmlAttributeMethodName)
.WriteStringLiteral(htmlAttribute.Key)
.WriteParameterSeparator()
.WriteStartMethodInvocation(_tagHelperContext.MarkAsHtmlEncodedMethodName);

// If it's a plain text value then we need to surround the value with quotes.
if (isPlainTextValue)
{
_writer.WriteStringLiteral(textValue);
}
else
{
RenderBufferedAttributeValueAccessor(_writer);
}

_writer
.WriteEndMethodInvocation(endLine: false)
.WriteEndMethodInvocation();
_writer.WriteEndMethodInvocation(endLine: false)
.WriteEndMethodInvocation();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public GeneratedTagHelperContext()
ScopeManagerEndMethodName = "End";
ExecutionContextAddMethodName = "Add";
ExecutionContextAddTagHelperAttributeMethodName = "AddTagHelperAttribute";
ExecutionContextAddMinimizedHtmlAttributeMethodName = "AddMinimizedHtmlAttribute";
ExecutionContextAddHtmlAttributeMethodName = "AddHtmlAttribute";
ExecutionContextOutputPropertyName = "Output";
MarkAsHtmlEncodedMethodName = "Html.Raw";
Expand Down Expand Up @@ -57,6 +58,11 @@ public GeneratedTagHelperContext()
/// </summary>
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; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si.


/// <summary>
/// The name of the <see cref="ExecutionContextTypeName"/> method used to add HTML attributes.
/// </summary>
Expand Down
20 changes: 13 additions & 7 deletions src/Microsoft.AspNet.Razor/Generator/TagHelperCodeGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,25 @@ public override void GenerateStartBlockCode(Block target, CodeGeneratorContext c

foreach (var attribute in tagHelperBlock.Attributes)
{
// Populates the code tree with chunks associated with attributes
attribute.Value.Accept(codeGenerator);
ChunkBlock attributeChunkValue = null;

var chunks = codeGenerator.Context.CodeTreeBuilder.CodeTree.Chunks;
var first = chunks.FirstOrDefault();
if (attribute.Value != null)
{
// Populates the code tree with chunks associated with attributes
attribute.Value.Accept(codeGenerator);

attributes.Add(new KeyValuePair<string, Chunk>(attribute.Key,
new ChunkBlock
var chunks = codeGenerator.Context.CodeTreeBuilder.CodeTree.Chunks;
var first = chunks.FirstOrDefault();

attributeChunkValue = new ChunkBlock
{
Association = first?.Association,
Children = chunks,
Start = first == null ? SourceLocation.Zero : first.Start
}));
};
}

attributes.Add(new KeyValuePair<string, Chunk>(attribute.Key, attributeChunkValue));

// Reset the code tree builder so we can build a new one for the next attribute
codeGenerator.Context.CodeTreeBuilder = new CodeTreeBuilder();
Expand Down
19 changes: 16 additions & 3 deletions src/Microsoft.AspNet.Razor/Parser/HtmlMarkupParser.Block.cs
Original file line number Diff line number Diff line change
Expand Up @@ -473,12 +473,25 @@ private void BeforeAttribute()

if (!At(HtmlSymbolType.Equals))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since parser gets confused by whitespace, how will <element attribute = "value" /> or <element attribute ="value" /> be parsed now? Has this change made handling of (perfectly valid) whitespace before the equals worse?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
// Saw a space or newline after the name, so just skip this attribute and continue around the loop
Accept(whitespace);
Accept(name);
// Minimized attribute
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the comment at this spot in previous iterations was useful. It described what the Output() calls would do. My complaint was about the last ("in most cases") bit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, remind the reader what has already been accepted i.e. what the first Output() will do. The second Output() should be obvious to all.


// Output anything prior to the attribute, in most cases this will be the tag name:
// |<input| checked />. If in-between other attributes this will noop or output malformed attribute
// content (if the previous attribute was malformed).
Output(SpanKind.Markup);

using (Context.StartBlock(BlockType.Markup))
{
Accept(whitespace);
Accept(name);
Output(SpanKind.Markup);
}

return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment right below here reminding user that we're in the mainline scenario at this point i.e. recovery and minimized attribute cases were handled above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanx

// Not a minimized attribute, parse as if it were well-formed (if attribute turns out to be malformed we
// will go into recovery).
Output(SpanKind.Markup);

// Start a new markup block for the attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ public TagHelperBlock(TagHelperBlockBuilder source)

foreach (var attributeChildren in Attributes)
{
attributeChildren.Value.Parent = this;
if (attributeChildren.Value != null)
{
attributeChildren.Value.Parent = this;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -71,11 +71,12 @@ private static IList<KeyValuePair<string, SyntaxTreeNode>> GetTagAttributes(
// Only want to track the attribute if we succeeded in parsing its corresponding Block/Span.
if (succeeded)
{
// Check if it's a bound attribute that is not of type string and happens to be null or whitespace.
// Check if it's a bound attribute that is minimized or not of type string and null or whitespace.
string attributeValueType;
if (attributeValueTypes.TryGetValue(attribute.Key, out attributeValueType) &&
(attribute.Value == null ||
!IsStringAttribute(attributeValueType) &&
IsNullOrWhitespaceAttributeValue(attribute.Value))
IsNullOrWhitespaceAttributeValue(attribute.Value)))
{
var errorLocation = GetAttributeNameStartLocation(child);

Expand Down Expand Up @@ -167,7 +168,7 @@ private static bool TryParseSpan(
{
Debug.Assert(
name != null,
"Name should never be null here. The parser should guaruntee an attribute has a name.");
"Name should never be null here. The parser should guarantee an attribute has a name.");

// We've captured all leading whitespace and the attribute name.
// We're now at: " asp-for|='...'" or " asp-for|=..."
Expand Down Expand Up @@ -239,7 +240,9 @@ private static bool TryParseSpan(
return false;
}

attribute = CreateMarkupAttribute(name, builder, attributeValueTypes);
// If we're not after an equal then we should treat the value as if it were a minimized attribute.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't run into an = it's the end of the attribute, therefore no value.

var attributeValueBuilder = afterEquals ? builder : null;
attribute = CreateMarkupAttribute(name, attributeValueBuilder, attributeValueTypes);

return true;
}
Expand Down Expand Up @@ -432,17 +435,24 @@ private static KeyValuePair<string, SyntaxTreeNode> CreateMarkupAttribute(
IReadOnlyDictionary<string, string> attributeValueTypes)
{
string attributeTypeName;
Span value = null;

// If the attribute was requested by the tag helper and doesn't happen to be a string then we need to treat
// its value as code. Any non-string value can be any C# value so we need to ensure the SyntaxTreeNode
// reflects that.
if (attributeValueTypes.TryGetValue(name, out attributeTypeName) &&
!IsStringAttribute(attributeTypeName))
// Builder will be null in the case of minimized attributes
if (builder != null)
{
builder.Kind = SpanKind.Code;
// If the attribute was requested by the tag helper and doesn't happen to be a string then we need to treat
// its value as code. Any non-string value can be any C# value so we need to ensure the SyntaxTreeNode
// reflects that.
if (attributeValueTypes.TryGetValue(name, out attributeTypeName) &&
!IsStringAttribute(attributeTypeName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why is this line wrapped?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line otherwise.

{
builder.Kind = SpanKind.Code;
}

value = builder.Build();
}

return new KeyValuePair<string, SyntaxTreeNode>(name, builder.Build());
return new KeyValuePair<string, SyntaxTreeNode>(name, value);
}

private static bool IsNullOrWhitespaceAttributeValue(SyntaxTreeNode attributeValue)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System;
using System.Collections.Generic;
using Microsoft.Internal.Web.Utils;

namespace Microsoft.AspNet.Razor.Runtime.TagHelpers
{
Expand All @@ -26,16 +25,13 @@ public bool Equals(IReadOnlyTagHelperAttribute attributeX, IReadOnlyTagHelperAtt
// Normal comparer (TagHelperAttribute.Equals()) doesn't care about the Name case, in tests we do.
return attributeX != null &&
string.Equals(attributeX.Name, attributeY.Name, StringComparison.Ordinal) &&
Equals(attributeX.Value, attributeY.Value);
attributeX.Minimized == attributeY.Minimized &&
(attributeX.Minimized || Equals(attributeX.Value, attributeY.Value));
}

public int GetHashCode(IReadOnlyTagHelperAttribute attribute)
{
return HashCodeCombiner
.Start()
.Add(attribute.Name, StringComparer.Ordinal)
.Add(attribute.Value)
.CombinedHash;
return attribute.GetHashCode();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 though I don't see how this relates to the rest of this PR. should at least be separately mentioned in the PR and commit descriptions.

}
}
}
Loading