Add FallbackTestPseudoElement property to LinkTagHelper

[dasblinkenlight]

Background and Motivation

LinkTagHelper generates a fallback test using properties passed to it via a sequence of asp-fallback-test-<...> attributes, namely asp-fallback-test-class, asp-fallback-test-property, and asp-fallback-test-value. All these attributes are optional.

Performing fallback testing with pseudo-elements requires passing an additional value via an optional asp-fallback-test-pseudo-element attribute. Following the naming convention of the other properties, the additional value should go into the property called FallbackTestPseudoElement.

Proposed API

public class LinkTagHelper : UrlResolutionTagHelper
{
...
+    private const string FallbackTestPseudoElementAttributeName = "asp-fallback-test-pseudo-element";
...
+    [HtmlAttributeName(FallbackTestPseudoElementAttributeName)]
+    public string FallbackTestPseudoElement { get; set; }
...
}

Usage Examples

<link rel="stylesheet"
    href="https://cdnjs.cloudflare.com/ajax/libs/bootstrap-icons/1.9.1/font/bootstrap-icons.min.css"
    integrity="sha512-5PV92qsds/16vyYIJo3T/As4m2d8b6oWYfoqV+vtizRB6KhF1F9kYzWzQmsO6T3z3QG2Xdhrx7FQ+5R1LiQdUA=="
    crossorigin="anonymous" referrerpolicy="no-referrer"
    asp-fallback-href="~/lib/bootstrap-icons/dist/bootstrap-icons.css"
    asp-fallback-test-class="bi"
    asp-fallback-test-pseudo-element=":before"
    asp-fallback-test-value="bootstrap-icons"
    asp-fallback-test-property="font-family"
    asp-suppress-fallback-integrity="true"
    />

Alternative Designs

N/A

Risks

Since the new attribute is optional, this change is virtually risk-free. Removing its use from the <link> element restores the present functionality.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[dasblinkenlight]

@captainsafia I don't understand the last bullet point from msftbot, about "someone being assigned to champion this change in the meeting". This change is small and self-explanatory, does it need a "champion" for the meeting as well?

[captainsafia]

@dasblinkenlight This typically means that someone in our API review meetings is available and understands the ramifications of the change. I don't believe it's a hard requirement but @halter73 who heads API review can confirm.

[halter73]

Exactly. Personally, I'm not super familiar with the LinkTagHelper or tag helpers in general, so I don't know much about asp-fallback-* properties. I definitely don't know why we need a asp-fallback-test-pseudoelement but not a asp-fallback-test-element. I'd figure CSS elements would be more common than pseudo-element, but I'm probably missing something because I'm not a subject area expert.

A champion is someone we know and trust who is knowledgeable in the area, who can answer these kinds of questions and help us gain confidence that this is an API we want to commit to adding and maintaining going forward.

[dasblinkenlight]

@halter73 Long story short, we don't need asp-fallback-test-element because it is always the same -- namely, it's the <meta> tag that LinkTagHelper inserted for the fallback testing.

A brief overview of the current implementation

Here is a brief explanation of what is going on here: LinkTagHelper works with the <link> tag, which we use to reference a resource from an external location. When the resource is a CDN-hosted library, ASP.NET lets us test if the resource failed to load, and specify a backup location for it (i.e. a copy of the library on your own host).
Fallback test is implemented as follows:

1. LinkTagHelper renders two additional tags following the "plain" <link> tag - <meta> and <SCRIPT>
2. The <meta> tag is rendered with the css class specified in asp-fallback-test-class attribute, which is backed by the FallbackTestClass property of LinkTagHelper
3. The <SCRIPT> tag includes a small JS function that finds the <meta> tag, and examines its computed style using getComputedStyle function
4. The function compares the style to the value the library sets for the given class (asp-fallback-test-value/FallbackTestValue)
5. If the comparison fails, the function inserts <link> tags for the backup location of the resource (asp-fallback-href/FallbackHref)

Enhancement

Note that getComputedStyle has two overloads - with and without the pseudoElt parameter. Most css libraries supply a style that can be tested using the first overload. However, some popular libraries (e.g. bootstrap icons) consist entirely of pseudo-element styles.

This is where this enhancement comes in: it lets the author of the ASP.NET page specify the value of the second parameter for the call to getComputedStyle, letting us automate the fallback test for css libraries that we are forced to test manually today.

How do we solve this problem today?

Currently, ASP.NET authors use workarounds, such as this one for bootstrap icons, when they hard-code the entire fallback test function with a single modification -- passing "::before" for the second parameter of getComputedStyle. This enhancement will do it properly, i.e. by adding asp-fallback-test-pseudo-element="::before" attribute to the <link> tag.

[halter73]

Thanks for the detailed explanation!

I was about to suggest supporting asp-fallback-test-class="hidden::before" rather than adding a new property, but I think I prefer your proposal. My suggestion probably stretches the notion of "class" too far, so it wouldn't be as discoverable. People might not be as likely to realize we added the feature without something like asp-fallback-test-pseudo-element in the autocomplete. We also wouldn't want to give the impression that any CSS selector would work since we're not adding arbitrary HTML elements.

For our future reference, the original issue is #38146.

[halter73]

API Review Notes:

• There's no known workaround. This API does make asp-fallback-href usable in a somewhat rare scenario. Other than doing it completely manually yourself.
• We think that the API is reasonable, but not sure the implementation is worth the additional complexity.

API is approved, but we still need to decide whether the PR should be approved.

public class LinkTagHelper : UrlResolutionTagHelper
{
+    [HtmlAttributeName(FallbackTestPseudoElementAttributeName)]
+    public string FallbackTestPseudoElement { get; set; }
}

[dasblinkenlight]

@halter73 Thank you very much for the update. At this point, is there anything I need to do with regard to moving the PR forward? Should this issue remain open, or does it need to be closed?

As far as the additional complexity goes, the change itself is relatively straightforward, and spans only a few dozen lines. The overwhelming majority of the PR changes are in the additional unit tests. It is fueled by the combinatorial explosion of unit test cases that we need to cover all combinations of asp-fallback-test-* properties - basically, I needed to copy the setup for all the existing test cases, add the new attribute to them, and verify that the new output is correct.

[mkArtakMSFT]

Thanks for suggesting this feature and also for putting out a PR for it, @dasblinkenlight.
Unfortunately, after further discussion within the team, we've decided not to take this change. It feels bad to say this, given that you've spent a lot of time on the PR. To avoid this in the future, please wait for the issues to be triaged (assigned to a milestone) before starting the work on it. Once again thank you for your effort to make the framework even better.

[dasblinkenlight]

@mkArtakMSFT I am sorry, "after further discussion within the team" does not work as an explanation. Could you please provide detailed reasoning behind your decision? It makes zero sense to me that you would close the PR with a clean implementation and an approved API change with no explanation, not even a code review, after six month of back and forth with me. The problem I identified and solved is real, and there is literally no other solution to it in ASP.NET.

[adrielairaldo]

I've come this far since I'm encountering exactly the same use case as @dasblinkenlight reports.

I have been following the issue since its inception 3 years ago (#38146) and I wonder what has happened; why has his collaboration been rejected?