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

Add CSS value selectors to TagHelper attributes #684

Closed
NTaylorMullen opened this issue Feb 4, 2016 · 14 comments
Closed

Add CSS value selectors to TagHelper attributes #684

NTaylorMullen opened this issue Feb 4, 2016 · 14 comments
Assignees
Milestone

Comments

@NTaylorMullen
Copy link
Member

Add the ability to do:

HtmlTargetElement(Attributes = "href^=~/") // Value starts with "~/"
HtmlTargetElement(Attributes = "href$=;") // Value ends with ";"
HtmlTargetElement(Attributes = "href=something") // Value equals "something"

This would enable our UrlResolutionTagHelper in MVC to only apply when it sees a ~/.

@NTaylorMullen
Copy link
Member Author

/cc @Eilon @rynowak

Talked with @DamianEdwards and we need to do this in the name of perf for UrlResolutionTagHelper for RC2.

@rynowak
Copy link
Member

rynowak commented Feb 4, 2016

See: aspnet/Mvc#3516 (comment)

@Eilon
Copy link
Member

Eilon commented Feb 4, 2016

Is there something not super complicated that we can do for v1? I'd rather not invent/implement a language for this...

@DamianEdwards
Copy link
Member

Spoke with @davidfowl and for now perhaps we just support a callback model instead. e.g.

[HtmlTargetElement("a", Attributes = "href", PredicateMethodName = "OnEmitAsTagHelper")]
public class MyTagHelper : TagHelper
{
    public static bool OnEmitAsTagHelper(string elementName, TagHelperAttributeList attributes)
    {
        return attributes["href"].StartsWith("~);
    }
}

@DamianEdwards
Copy link
Member

Or maybe we create a new context type for this stage, e.g. TagHelperBuilderContext, which we can grow over time as we add more support for Tag Helpers to participate in view compilation?

@dougbu
Copy link
Member

dougbu commented Feb 5, 2016

There are at least three perf issues related to the UrlResolutionTagHelper:

  1. It targets a number of elements that would not be targeted otherwise, leading to "extra" TagHelperExecutionContext, TagHelperOutput, ... allocations. Pretty much any solution that doesn't require an instance of the tag helper should address this part of the problem.
  2. The tag helper system allocates large-sized TagHelperAttributes and related arrays / lists and so on. Recent changes such as Lazily initialize TagHelperAttributeLists. #683 (still in PR) should help here. But any callback approach would mean these TagHelperAttributes are still allocated.
  3. The support for attribute values containing C# expressions that start with "~/" requires early evaluation of IHtmlContent values. We could reduce this overhead significantly if we removed support for entirely dynamic attributes and IHtmlContent included a string FirstSegment { get; } property for UrlResolutionTagHelper to inspect. The solutions mentioned above share removal of entirely dynamic attributes with this suggestion.

Do we have current numbers showing whether 1, 2, or 3 is the primary issue for significantly reduced performance when UrlResolutionTagHelper is enabled? (2 is the one that I see near the top for many scenarios. I've heard others point toward 1. But I'm not sure of the post-#683 situation.)

@DamianEdwards
Copy link
Member

The attributes in this case would be allocated during view compilation, so that's not an issue. The example was just pseudo code, so it's likely a different type to pass the attributes to the predicate is better.

As for attributes that contain expressions, we need to do the simplest thing for now that preserves the pre-Tag Helpers behavior.

@NTaylorMullen
Copy link
Member Author

The work you're suggesting is near equivalent workload to implementing a CSS selector on the Attributes property. Seeing that we have a whole host of codegen related extension points slated for post v1 I think the OnEmitAsTagHelper type method would make more sense there and a CSS selector here.

@DamianEdwards
Copy link
Member

I think we should do some rapid experiments here to validate some of the ideas.

@NTaylorMullen
Copy link
Member Author

Which ideas are you referring to?

@NTaylorMullen
Copy link
Member Author

@DamianEdwards and I talked offline and we're gonna go with the CSS selector on Attributes =. I assume this feature is approved for RC2?

@DamianEdwards
Copy link
Member

Hack something up so we can assess whether it actually helps the perf issue.

@Eilon
Copy link
Member

Eilon commented Feb 9, 2016

I'm fine with RC2 if it looks like it solves the perf problem we have.

@NTaylorMullen NTaylorMullen added this to the 1.0.0-rc2 milestone Feb 12, 2016
NTaylorMullen added a commit that referenced this issue Feb 29, 2016
- Added the ability for users to opt into CSS `TagHelper` selectors in their required attributes by surrounding the value with `[` and `]`. Added operators `^`, `$` and `=`.
- Added tests to cover code paths used when determining CSS selectors.

#684
NTaylorMullen added a commit that referenced this issue Mar 2, 2016
- Added the ability for users to opt into CSS `TagHelper` selectors in their required attributes by surrounding the value with `[` and `]`. Added operators `^`, `$` and `=`.
- Added tests to cover code paths used when determining CSS selectors.

#684
NTaylorMullen added a commit that referenced this issue Mar 4, 2016
- Added the ability for users to opt into CSS `TagHelper` selectors in their required attributes by surrounding the value with `[` and `]`. Added operators `^`, `$` and `=`.
- Added tests to cover code paths used when determining CSS selectors.

#684
NTaylorMullen added a commit that referenced this issue Mar 7, 2016
- Added the ability for users to opt into CSS `TagHelper` selectors in their required attributes by surrounding the value with `[` and `]`. Added operators `^`, `$` and `=`.
- Added tests to cover code paths used when determining CSS selectors.

#684
NTaylorMullen added a commit to aspnet/Mvc that referenced this issue Mar 8, 2016
- Modified `UrlResolutionTagHelper` to utilize new CSS selector attributes to restrict when it applies. It now only appies to tags that have their values starting with `~/`.
- `UrlResolutionTagHelper` no longer applies to dynamic content such as `href="@SomethingResultingInTildaSlash"`.
- Updated tests to reflect new behavior.
NTaylorMullen added a commit that referenced this issue Mar 8, 2016
- Added the ability for users to opt into CSS `TagHelper` selectors in their required attributes by surrounding the value with `[` and `]`. Added operators `^`, `$` and `=`.
- Added tests to cover code paths used when determining CSS selectors.

#684
NTaylorMullen added a commit to aspnet/Mvc that referenced this issue Mar 8, 2016
- Modified `UrlResolutionTagHelper` to utilize new CSS selector attributes to restrict when it applies. It now only appies to tags that have their values starting with `~/`.
- `UrlResolutionTagHelper` no longer applies to dynamic content such as `href="@SomethingResultingInTildaSlash"`.
- Updated tests to reflect new behavior.
NTaylorMullen added a commit to aspnet/RazorTooling that referenced this issue Mar 8, 2016
- Added the new `TagHelperRequiredAttributeDescriptor` to the VisualStudio validation test.
@NTaylorMullen
Copy link
Member Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants