TagHelper attributes with no value cannot be used in TagHelpers

[NTaylorMullen]

Razor does not correctly parse HTML attributes that have no value.

After playing around with the parse tree a bit I noticed that if you were to create HTML elements such as:

<input checked>
<input checked value>

The Razor parser does not generate an accurate syntax tree. This in turn results in the generated code not calling WriteAttribute and furthermore with TagHelpers does not allow us to pull the attributes from an HTML element easily.

Once this has been added we should also support this attribute syntax in Tag Helpers.

[dougbu]

supporting this would go beyond the XHTML restrictions elsewhere in the system. the only special case I'm aware of is <input @checked/> and that seems sufficient.

[NTaylorMullen]

@dougbu this is supported in HTML 5: http://www.w3.org/TR/html-markup/syntax.html#syntax-attr-empty

[dougbu]

Agreed. However we restrict ourselves to generating and accepting only the XHTML subset of HTML 5.

[NTaylorMullen]

Hmm? Where do we restrict ourselves to generating and accepting only the XHTML subset of HTML5?

[dougbu]

This bug is just one example where Razor doesn't support non-XHTML Another is tag helpers not handling an empty element unless it's written as a self-closing tag.

On the generation side, RazorPage.WriteAttributeTo() special-cases bool values and generates long-form attributes -- either nothing (for false) or attribute-name='attribute-name'. TagBuilder.AppendAttributes() and TagHelperOutput.GenerateStartTag() also generate only long-form attributes and their containing classes never leave off end tags.

Side question: Why did you add the "bug" tag for this issue? Seems more like an enhancement to support additional syntax for already-supported HTML features. Even more like a part of a larger "HTML short-form" feature that would touch a lot of the Razor parser as well as the HTML and tag helper infrastructure.

[NTaylorMullen]

It's marked as a bug because we're making the move to support HTML5. That was also why we modified TagBuiler/TagHelperOutput to have case-insensitive attributes and marked those similarly as bugs.

[dougbu]

Low importance: The analogy doesn't hold because a .cshtml file containing attributes using various cases can still generate a valid-looking XHTML document. But perhaps you're right and we should normalize the case of all attributes we write, regardless of the user's input.

[yishaigalatzer]

The key issue here is this:

In razor 2 or 3 when the user wrote

<input required />

We rendered

<input required />

But with tag helpers (input tag helper for example)

<input asp-for="FieldName" required />

We will trigger a parsing error and you will have to type

<input asp-for="FieldName" required="true" />

[dougbu]

when this bug is fixed, will need to modify the expectations in https://github.com/aspnet/Mvc/tree/dev/test/Microsoft.AspNet.Mvc.FunctionalTests/Compiler/Resources files as well as one HTML helper test file (see https://github.com/aspnet/Mvc/blob/dev/test/WebSites/MvcTagHelpersWebSite/Views/MvcTagHelper_Home/OrderUsingHtmlHelpers.cshtml#L72) that mimics current tag helper results

[NTaylorMullen]

With issue #289 all TagHelper bound attributes that are not typed System.String without a value will be invalid and cause a parse error. Do we want to do something special with say bool typed attributes, or any other types?

/cc @DamianEdwards

[DamianEdwards]

I think we just shouldn't support bound properties that don't have a value specified, i.e. parser error.

[NTaylorMullen]

@ChrisKlug Can completely understand the frustration. Razor actually as whole today doesn't parse attributes correctly even when not using TagHelpers. For instance: #123. Hoping to get these issues all resolved to have less frustration 😄

[DamianEdwards]

Just to be clear, the Tag Helper will be able to see the attribute once we fix other things we're currently working on. You'll be able to see the attribute from TagHelperContext.AllAttributes. This bug is specifically about having Tag Helper bound properties (i.e. properties on your Tag Helper class) be populated from attributes with no value.

Also, you'll be able to target a Tag Helper based on the existence of such a property using the TargetElement attribute.

[ChrisKlug]

Ok, great! It just caused me a heap of headscratching when I couldn't get my TagHelper to run when the "required" attribute didn't have a value. After a bit of looking, it seemed as it could be solved by changing some not too complicated things in the attribute parsing, but I don't know if that would affect something else... But fixing that seemed like a small thing that could be fixed by a quick pull request, but if it is bigger I will stay out of your way...

[NTaylorMullen]

@ChrisKlug The parsing logic change isn't much. Writing the tests for it will be the time sink, prob 20% feature 80% test time for this one 😄

[NTaylorMullen]

6fa3e40
0882ff4