fix(core): more accurate matching of classes during content projection

[masaoki]

Small bug fix. Showing a minimum app to reproduce the bug.

1. Create the app and add angular material.

ng new project
cd project
ng add @angular/material

1. Overwrite the src/app/app.component.html with minimal content.

<button mat-button *ngIf="true"><span *ngIf="true" class="{{'a'}}"></span></button>

1. Run the app. The button is not shown because of an exception.

main.ts:6
ERROR TypeError: item.toLowerCase is not a function
    at isCssClassMatching (core.mjs:8726:35)
    at isNodeMatchingSelector (core.mjs:8814:22)
    at isNodeMatchingSelectorList (core.mjs:8931:13)
    at matchingProjectionSlotIndex (core.mjs:14179:13)
    at Module.ɵɵprojectionDef (core.mjs:14222:49)
    at MatButton_Template (button.mjs:113:99)
    at executeTemplate (core.mjs:10534:9)
    at renderView (core.mjs:10356:13)
    at renderComponent (core.mjs:11529:5)
    at renderChildComponents (core.mjs:10216:9)

Because isCssClassMatching() function does not take care if the value is not string, while attrs[] may contain AttributeMarker which is actually numbers, item.toLowerCase() throws the exception. Just inserted a check if the item is string.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[AndrewKushnir]

@masaoki thanks for creating this PR. This fix would also require a test in the packages/core/test/acceptance/content_spec.ts spec. Could you please try to reproduce this problem by putting together a test (see this doc on how to run tests locally)? I think it'd be possible to reproduce this issue without Material components for tests, you can mimic the behavior of the MatButton (the content projection aspect of it). Please let us know if you have any questions. Thank you.

[masaoki]

@AndrewKushnir Ok. I'm going to find a way to reproduce the exception without angular material, and try to reflect the result to the test.

[masaoki]

@AndrewKushnir Pushed content_spec.ts with a test added. The exception could be reproduced when the component has <ng-content> looking at the inner children with both *ngIf and class='{{interpolation}}'.

[truskowc]

Getting the same issue. What's going on with this?

[truskowc]

For those looking for a fix you can rewrite the class interpolation:

class="icon {{ parentMenu.iconClass }}"

to

[ngClass]="[ parentMenu.iconClass ? parentMenu.iconClass : '' ]"

this worked for me

[masaoki]

@AndrewKushnir
Are there any obstacles for merging? What can I do?

[AndrewKushnir]

@masaoki thanks for the PR and sorry for the delay.

While the fix avoids the JS exception, I'm not 100% sure it resolves the root cause. The TAttributes array consists of strings (used as keys and values) and numbers (used as markers to indicate a start of a section). If we come across a situation where a a string is class and the next element is a number, it means that we are in not in a "static attribute" section, but instead, we are in bindings section, thus we should not try to match. However in a situation like this:

<span *ngIf="true" id="5" class="{{'a'}}" [title]="'abc'"></span>

the TAttributes would probably look like this:

['id', '5', 3 /* AttributeMarker.Bindings */, 'class', 'title']

So the elements after a marker would be just binding names, but the logic will treat them as key-value pair and likely match (if you have select=".title" in a projector component).

Could you pleaser try to add a test case based on this example? Also it'd be interesting to see what the attrs array look like in this case.

You can also find additional information about the TAttributes array in the code here.

Thank you.

[masaoki]

@AndrewKushnir
In the my test case with minimum attributes on it, the html is:

<child-comp><span *ngIf="true" class="{{'a'}}"></span></child-comp>

In this case, the attrs inside isCssClassMatching() is

[3, 'class', 4, 'ngIf']

which makes item.toLowerCase() throw the exception. When I change the html to the one you suggested:

<child-comp><span *ngIf="true" id="5" class="{{'a'}}" [title]="'abc'"></span></child-comp>

the attr is

['id', '5', 3, 'class', 'title', 4, 'ngIf']

The exception is gone in this case because of the additional "title" binding, but the isCssClassMatching() confusingly returns true since 'title' is after 'class' as you expected.

The sort order is:

1. attribute name (string), value (string) pair
2. AttributeMarker.* (number) with following attribute names (string)

So what we actually have to do here is to find the cssClassToMatch value followed by 'class' before any number value, or the same value between AttributeMarker.Classes and another AttributeMarker.* where the 'class' found is not what we look for.

I rewrote the isCssClassMatching() method to effectively work as expected. Also added both of test cases to packages/core/test/acceptance/content_spec.ts.

[AndrewKushnir]

So what we actually have to do here is to find the cssClassToMatch value followed by 'class' before any number value, or the same value between AttributeMarker.Classes and another AttributeMarker.* where the 'class' found is not what we look for.

Yes, that's correct 👍

Ideally, it'd be great to do it in a single pass over the attrs array, so we can probably do something like this? (note: I haven't tested it, just wanted to illustrate the idea)

diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts
index 77beac2b7d..5599805554 100644
--- a/packages/core/src/render3/node_selector_matcher.ts
+++ b/packages/core/src/render3/node_selector_matcher.ts
@@ -35,9 +35,14 @@ function isCssClassMatching(
       assertEqual(
           cssClassToMatch, cssClassToMatch.toLowerCase(), 'Class name expected to be lowercase.');
   let i = 0;
+  // Indicates whether we are processing value from the implicit
+  // attribute section (i.e. before the first marker in the array).
+  let isImplicitAttrsSection = true;
   while (i < attrs.length) {
     let item = attrs[i++];
-    if (isProjectionMode && item === 'class') {
+    if (isProjectionMode && item === 'class' && isImplicitAttrsSection) {
+      // We found a `class` attribute in the implicit attribute section,
+      // check if it matches the value of the `cssClassToMatch` argument.
       item = attrs[i] as string;
       if (classIndexOf(item.toLowerCase(), cssClassToMatch, 0) !== -1) {
         return true;
@@ -49,6 +54,10 @@ function isCssClassMatching(
         if (item.toLowerCase() === cssClassToMatch) return true;
       }
       return false;
+    } else if (typeof item === 'number') {
+      // We've came across a first marker, which indicates
+      // that the implicit attribute section is over.
+      isImplicitAttrsSection = false;
     }
   }
   return false;

[masaoki]

@AndrewKushnir
I see but this code will make another defect with something like:

<child-comp><span *ngIf="true" id="class" class="{{'a'}}"></span></child-comp>

I pushed changes a little modification based on your code again with a test covering the above case.
Check it pls.

[AndrewKushnir]

I see but this code will make another defect with something like:
...
I pushed changes a little modification based on your code again with a test covering the above case.
Check it pls.

Yes, it makes sense. When we are in the "implicit attrs" section, we should handle elements in pair ([key, value, ...]) as implemented in your change 👍

[masaoki]

@AndrewKushnir Did it. Thanks for your assistance!

[AndrewKushnir]

@masaoki thanks for addressing the feedback!

It looks like CI is failing for a couple reasons:

• The spec file is not formatted correctly, please run a command mentioned in the CI job output (see the CI job output here)
• There is a failing test (see CI job output here) - it looks like the test is failing due to the override of the item var and instead it should look something like this:

--- a/packages/core/src/render3/node_selector_matcher.ts
+++ b/packages/core/src/render3/node_selector_matcher.ts
@@ -41,11 +41,11 @@ function isCssClassMatching(
   while (i < attrs.length) {
     let item = attrs[i++];
     if (typeof item === 'string' && isImplicitAttrsSection) {
-      item = attrs[i++] as string;
+      const value = attrs[i++] as string;
       if (isProjectionMode && item === 'class') {
         // We found a `class` attribute in the implicit attribute section,
         // check if it matches the value of the `cssClassToMatch` argument.
-        if (classIndexOf(item.toLowerCase(), cssClassToMatch, 0) !== -1) {
+        if (classIndexOf(value.toLowerCase(), cssClassToMatch, 0) !== -1) {
           return true;
         }
       }

Please try to apply the changes and let us know if CI is still "red".

Could you please also merge all commits into one and may be rephrase the commit message a bit to reflect the changes (something like fix(core): more accurate matching of classes during content projection)?

Thank you.

[masaoki]

@AndrewKushnir
Thanks again for review and suggestion. Hope this is last time.

[AndrewKushnir]

@masaoki the change looks good, thanks for addressing the feedback 👍

We'll run some additional internal tests in Google's codebase and if everything is ok, we'll add the PR to the merge queue.

Thank you.

[AndrewKushnir]

Global Presubmit (internal-only link).

[AndrewKushnir]

Caretaker note: TGP is "green", this PR is ready for merge.

[pkozlowski-opensource]

This PR was merged into the repository by commit e655e8a.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}