Razor compiler emit CSS scope attributes #23703

SteveSandersonMS · 2020-07-06T14:35:35Z

Implements #22776

The actual IR tree update is pretty simple - we just add an extra attribute to every markup element within a component that has an associated CSS scope.

Most of the work here is plumbing the information from MSBuild through various layers within the Razor compiler. @NTaylorMullen @ajaybhargavb are you happy with the way this is being done here? It feels quite special-casey but it doesn't look like there's any other established way to pass through per-document parameters. I did consider passing this information via RazorCodeGenerationOptions but that seems only to be used for project-wide settings so doesn't look right to use it for per-document information.

Blazor team review notes: This PR means that our MSBuild code in the SDK can choose to supply CssScope metadata for any of the RazorComponent itemgroup entries. For example:

    <Content Update="Pages\Counter.razor">
      <!-- TODO: In our SDK, auto-generate this metadata for any component that has a corresponding *.razor.css file on disk -->
      <!-- The exact value doesn't matter, but should be derived from a hash of the OS-independent relative path to the .razor file within the project -->
      <CssScope>b-8d2ksdjwa20</CssScope>
    </Content>

... will end up rendering an extra attribute called b-8d2ksdjwa20 on each of its HTML elements (e.g., <span b-8d2ksdjwa20>Hey</span>). This ties in with the CSS selector rewriting implemented in #23657 so that CSS rules only apply to elements rendered by this component.

SteveSandersonMS · 2020-07-06T15:59:06Z

I've also added a further optimization into the "minimized attribute" code path, because this becomes an extremely-frequently hit path for components that have scoped CSS. On components with scoped CSS, every single HTML element has at least one minimized attribute.

The FastGrid-from-blank benchmark improves by around 5% due to this optimization. Admittedly that's a pretty extreme case, but it is the sort of thing we're trying to optimize.

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentDesignTimeNodeWriter.cs

captainsafia · 2020-07-06T19:52:20Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectItem.cs

        /// <param name="file">The <see cref="FileInfo"/>.</param>
-        public DefaultRazorProjectItem(string basePath, string filePath, string relativePhysicalPath, string fileKind, FileInfo file)
+        /// <param name="cssScope">A scope identifier that will be used on elements in the generated class, or null.</param>
+        public DefaultRazorProjectItem(string basePath, string filePath, string relativePhysicalPath, string fileKind, FileInfo file, string cssScope)


Can we do string cssScope = null here to make this backwards compatible?

We could, but the class is internal so hopefully we don’t need to be concerned with back-compat.

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectEngine.cs

javiercn · 2020-07-08T10:15:52Z

src/Components/Components/ref/Microsoft.AspNetCore.Components.netcoreapp.cs

    {
        public RenderTreeBuilder() { }
        public void AddAttribute(int sequence, in Microsoft.AspNetCore.Components.RenderTree.RenderTreeFrame frame) { }
+        public void AddAttribute(int sequence, string name) { }


Is this a perf optimization?

I think I answered my own question here. You can correct me if I'm wrong.

Yes: #23703 (comment)

javiercn

I'm not an expert, but the changes and the tests look good!

SteveSandersonMS · 2020-07-08T11:09:09Z

Thanks @javiercn. I'd like to get a sign-off from either @NTaylorMullen or @ajaybhargavb before I merge it though!

javiercn · 2020-07-08T11:13:37Z

Thanks @javiercn. I'd like to get a sign-off from either @NTaylorMullen or @ajaybhargavb before I merge it though!

Sounds reasonable :)

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectItem.cs

SteveSandersonMS · 2020-07-10T21:22:25Z

Pinging @NTaylorMullen @ajaybhargavb - do my answers above sound right to you?

ajaybhargavb · 2020-07-10T21:39:53Z

Pinging @NTaylorMullen @ajaybhargavb - do my answers above sound right to you?

I have the same question as @NTaylorMullen's comment above. Otherwise consider me signed off as long as @NTaylorMullen is happy. One other comment,

I didn't see any Razor SDK tests that test the MSBuild portions etc. cc @pranavkm in case he has any thoughts on what to add here.

SteveSandersonMS · 2020-07-10T23:55:25Z

I have the same question as @NTaylorMullen's comment above.

I posted an answer previously. Is there still an open question? If so, could you clarify what?

SteveSandersonMS · 2020-07-10T23:56:10Z

I didn't see any Razor SDK tests that test the MSBuild portions etc.

The MSBuild targets are a separate work item, not implemented here.

ajaybhargavb · 2020-07-11T00:01:17Z

I posted an answer previously. Is there still an open question? If so, could you clarify what?

Woah. I swear GitHub hid your response from me 👀. That resolves my concerns

mrpmorris · 2020-07-11T09:38:32Z

Will this be compatible with MyComponent.razor.css files that are generated from scss files during build?

src/Razor/Microsoft.NET.Sdk.Razor/src/RazorGenerate.cs

NTaylorMullen · 2020-07-11T17:18:27Z

src/Razor/Microsoft.NET.Sdk.Razor/src/RazorGenerate.cs

                    if (!string.IsNullOrEmpty(kind))
                    {
                        builder.AppendLine("-k");
                        builder.AppendLine(kind);


Question not related to this PR but I noticed it while reviewing: @ajaybhargavb / @pranavkm given the IsNullOrEmpty check above if someone clears their DocumentKind for a Razor item wont that corrupt every other document kind because it will no longer be 1-to-1? I'd imagine if the parser version was >= 3 then we'd always output Kind even if it wasn't on the item to ensure the mappings were correct.

NTaylorMullen · 2020-07-11T17:18:50Z

src/Razor/Microsoft.AspNetCore.Razor.Tools/src/GenerateCommand.cs

+            if (CssScopeSources.Values.Count != CssScopeValues.Values.Count)
+            {
+                // CssScopeSources and CssScopeValues arguments must appear as matched pairs
+                Error.WriteLine($"{CssScopeSources.Description} has {CssScopeSources.Values.Count}, but {CssScopeValues.Description} has {CssScopeValues.Values.Count} values.");


src/Razor/Microsoft.AspNetCore.Razor.Tools/src/GenerateCommand.cs

NTaylorMullen · 2020-07-11T17:30:15Z

src/Razor/Microsoft.AspNetCore.Razor.Tools/src/GenerateCommand.cs

            Outputs = Option("-o", "Generated output file path", CommandOptionType.MultipleValue);
            RelativePaths = Option("-r", "Relative path", CommandOptionType.MultipleValue);
            FileKinds = Option("-k", "File kind", CommandOptionType.MultipleValue);
+            CssScopeSources = Option("-cssscopeinput", "CSS scope source file", CommandOptionType.MultipleValue);


cssscopeinput makes it sound like this is the CSS file. Ditto with the description.

Good point. Changed cssscopeinput to cssscopedinput (to clarify that the scoping is something happening to the input, rather than the input being the scope), and updated descriptions. The term input here is still applicable because of its correspondence to the other parameter named input.

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectEngine.cs

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectItem.cs

…r minimized HTML attributes

SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Jul 6, 2020

SteveSandersonMS requested review from a team, NTaylorMullen and ajaybhargavb July 6, 2020 14:35

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 6, 2020

captainsafia reviewed Jul 6, 2020

View reviewed changes

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentDesignTimeNodeWriter.cs Show resolved Hide resolved

captainsafia reviewed Jul 6, 2020

View reviewed changes

pranavkm requested a review from a team July 6, 2020 22:30

NTaylorMullen reviewed Jul 6, 2020

View reviewed changes

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectEngine.cs Show resolved Hide resolved

javiercn reviewed Jul 8, 2020

View reviewed changes

javiercn approved these changes Jul 8, 2020

View reviewed changes

SteveSandersonMS linked an issue Jul 8, 2020 that may be closed by this pull request

CSS isolation: Razor compiler update to emit CSS scope attributes #22776

Closed

NTaylorMullen reviewed Jul 8, 2020

View reviewed changes

src/Razor/Microsoft.AspNetCore.Razor.Language/src/DefaultRazorProjectItem.cs Show resolved Hide resolved

NTaylorMullen approved these changes Jul 11, 2020

View reviewed changes

SteveSandersonMS added 7 commits July 13, 2020 15:18

Pass CSS scope info all the way from MSBuild into CodeDocument

1af7ec1

Add CSS scope attribute to IR tree

86d1f95

Better ordering

40c2c40

Begin defining integration test case for CssScope pass

6997686

Show effects of CssScope

7bcbab1

Fix test

690b1bb

Optimisation: skip parameter passing and checking components cases fo…

3ea8f9c

…r minimized HTML attributes

SteveSandersonMS added 4 commits July 13, 2020 15:18

Update another baseline

94fc124

CR: Change how parameters are written

3c7b56e

CR: Rename cssScopeSourceIndices and do case-insensitive comparisons

b1bbd27

CR: Change param names

accc34a

SteveSandersonMS force-pushed the stevesa/rzc-emit-scope-attributes branch from 8f8f7eb to accc34a Compare July 13, 2020 14:35

Update baseline

86168d6

SteveSandersonMS merged commit 90b697f into master Jul 13, 2020

SteveSandersonMS deleted the stevesa/rzc-emit-scope-attributes branch July 13, 2020 16:27

SteveSandersonMS mentioned this pull request Aug 27, 2020

Fix minimized attributes when using 5.0 SDK with 3.x target #25307

Merged

Razor compiler emit CSS scope attributes #23703

Razor compiler emit CSS scope attributes #23703

Uh oh!

Conversation

SteveSandersonMS commented Jul 6, 2020

Uh oh!

SteveSandersonMS commented Jul 6, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS commented Jul 8, 2020

Uh oh!

javiercn commented Jul 8, 2020

Uh oh!

Uh oh!

SteveSandersonMS commented Jul 10, 2020

Uh oh!

ajaybhargavb commented Jul 10, 2020

Uh oh!

SteveSandersonMS commented Jul 10, 2020

Uh oh!

SteveSandersonMS commented Jul 10, 2020

Uh oh!

ajaybhargavb commented Jul 11, 2020

Uh oh!

mrpmorris commented Jul 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants