Skip to content

[Mvc] Add Razor compiler support for CSS isolation for Views and Razor pages #29899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 16, 2021

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Feb 4, 2021

Fixes #25713

Applies the given cssscope to the HtmlElements on a given file. TagHelpers don't get the scope applied since they are not considered "part of the file" for the purpose of scoped CSS (equivalent to what we do for components).

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 4, 2021
@javiercn javiercn requested a review from a team February 4, 2021 16:55
@@ -525,6 +525,49 @@ public void RazorPageWithNoLeadingPageDirective_Runtime()
var diagnotics = compiled.CodeDocument.GetCSharpDocument().Diagnostics;
Assert.Equal("RZ3906", Assert.Single(diagnotics).Id);
}

[Fact]
public void View_WithCssScope()
Copy link
Member Author

Choose a reason for hiding this comment

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

The test is within the razor extensions project because otherwise is painful to write.

@javiercn javiercn requested a review from captainsafia February 4, 2021 17:07

namespace Microsoft.AspNetCore.Razor.Language.Extensions
{
internal class ViewCssScopePass : IntermediateNodePassBase, IRazorOptimizationPass
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of code-organization, MVC specific types have typically existed in the .Extensions assembly. In addition, we probably want to put this behind a language version check (we don't want this running on older versions of MVC).

(cc @NTaylorMullen)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point, but I don't think this is MVC specific, this is specifc to cshtml. Also, if that's the case, why is the components version of this (as well as A LOT of other stuff in this assembly instead of on its own Components assembly?)

if (child is IntermediateToken token && token.IsHtml)
{
var content = token.Content;
if (content.StartsWith("<") && !content.StartsWith("</"))
Copy link
Contributor

Choose a reason for hiding this comment

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

These should use StringComparison.Ordinal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it matters, does it?

Copy link
Member

Choose a reason for hiding this comment

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

Might be beneficial to avoid any present or future linter warnings. But agreed it shouldn't affect behavior.

I've been trying to think of any other cases where we might find a < which isn't immediately preceding a tag name, and haven't thought of any. For example cases like < p > or <a href=< >...</a> but I think these are too invalid to get through the parser anyway. So as far as I can tell, this seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I had the same concern and experimented a bit with different syntaxes. Worst case scenario (unlikely) we get a "smalish" bug here, that has an easy workaround and is easy to fix.


private void ProcessElement(HtmlContentIntermediateNode node, string cssScope)
{
cssScope = " " + cssScope;
Copy link
Member

@SteveSandersonMS SteveSandersonMS Feb 15, 2021

Choose a reason for hiding this comment

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

Nitpicky, but instead of allocating a new string for every node, we could construct the space-prefixed version just once and pass it into this method.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looks great to me. Presumably there's also some MSBuild work needed to pass in the scope identifiers for .cshtml files, right?

@SteveSandersonMS
Copy link
Member

TagHelpers don't get the scope applied since they are not considered "part of the file" for the purpose of scoped CSS (equivalent to what we do for components).

Just to check: this is only in the case where the whole tag is itself a tag helper, and not just one of its attributes, right?

That is, I recognize that <my-tag-helper> isn't going to emit the scope attribute (because we don't control what it emits), but if you have <a asp-for="...">, then this will emit the scope attribute, right?

@javiercn
Copy link
Member Author

That is, I recognize that <my-tag-helper> isn't going to emit the scope attribute (because we don't control what it emits), but if you have <a asp-for="...">, then this will emit the scope attribute, right?

Nope.

That's also a tag helper (an attribute tag helper) and we don't have any control over the HTML emitted when you apply a tag helper.

@SteveSandersonMS
Copy link
Member

That is a bit of a usability concern. People won’t think that adding an attribute to an element may break its scoped styles.

What stops us from rewriting it earlier in the process before the tag helper attribute gets involved?

@javiercn
Copy link
Member Author

@SteveSandersonMS A few things:

  • When you apply an attribute tag helper for a tag, it gets colorized like any other tag helper.
  • Tag helpers have control over the entire output of the tag, independent of whether they are applied as elements or attributes. If we enable the scope for "attribute tag helpers", there is no guarantee that it will get applied in the final output and the experience can be confusing.
  • Applying the scope might break the isolation that an existing tag helper might have relied on.

I think the current experience is simpler in that the scope only applies to HTML elements in the file, and the moment an element has a tag helper on it, it stops being a regular element.

FYI, if in the future we want to enable something like this, I would be ok with an explicit @scope directive that allows the scope to be "materialized" at that point and passed down explicitly to some components.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Feb 15, 2021

It's extremely rare that I create .cshtml files and use any tag helpers, so I'm not a good person to make any pronouncements about what users would or wouldn't expect. All I can say is this one case does seem surprising to me.

My mental model is that attribute tag helpers normally play a role of adding or changing attributes, rather than replacing the entire element. As such I would not expect that changing <a href="/MyPage"> to <a asp-page="/MyPage"> would disable the scoped CSS for that element - it seems completely orthogonal. I would expect that the asp-page attribute is just a magic alternative to href (and isn't changing the containing tag). I know that strictly speaking an attribute tag helper can do whatever it wants.

Like I said, this area of the product isn't one I really use a lot, so my opinion is of limited relevance, and I'd defer to your judgement! The choice we make here is something we need to be happy with forever, as we couldn't later make scoped styles start appearing here by default without it being a breaking change.

@javiercn
Copy link
Member Author

It's extremely rare that I create .cshtml files and use any tag helpers, so I'm not a good person to make any pronouncements about what users would or wouldn't expect. All I can say is this one case does seem surprising to me.

My mental model is that attribute tag helpers normally play a role of adding or changing attributes, rather than replacing the entire element. As such I would not expect that changing <a href="/MyPage"> to <a asp-page="/MyPage"> would disable the scoped CSS for that element - it seems completely orthogonal. I would expect that the asp-page attribute is just a magic alternative to href (and isn't changing the containing tag). I know that strictly speaking an attribute tag helper can do whatever it wants.

Like I said, this area of the product isn't one I really use a lot, so my opinion is of limited relevance, and I'd defer to your judgement! The choice we make here is something we need to be happy with forever, as we couldn't later make scoped styles start appearing here by default without it being a breaking change.

I understand your concerns. In the end is a balance between simplicity and least surprise. On the one side, folks might get surprised by this (the same thing happens with our NavLink or Input* components) if we don't apply it to tag helpers.

On the opposite end, we don't have a way to enforce a tag helper actually applies the attribute and that makes the experience highly variable. We might make it work for our own tag helpers, but it's not guaranteed that it will work for other tag helpers and that creates a somewhat inconsistent experience.

I think for the time being, I prefer we keep it simple and see what feedback we get (I'm anticipating we'll get a feedback similar to the one we receive in components). At that point, it's relatively easy for us to do something like what is described in dotnet/razor#7606 which would work for both regular tag helpers and components.

It's a bit more nuanced for TH since several can be applied to an element and an element can itself be a tag helper, but it's not that common to apply more than one outside of our own TH for which we can easily produce the right behavior.

@javiercn javiercn force-pushed the javiercn/mvc-css-isolation branch from ec02b96 to 79e1f8e Compare February 16, 2021 18:48
@javiercn javiercn merged commit bab9671 into main Feb 16, 2021
@javiercn javiercn deleted the javiercn/mvc-css-isolation branch February 16, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Css Isolation for Asp.Net Core Web Application for .cshtml files
3 participants