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

masaoki · pkozlowski-opensource · commit 087f4412afe9 · 2023-03-20T16:07:14.000+01:00
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. Created a testcase for the original fix. It causes an exception without the fix. fix(core): add a check code to avoid an exception inside isCssClassMatching 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. Add `import { MatButtonModule } from '@angular/material/button'`, and also MatButtonModule inside @NgModule imports in src/app/app.module.ts to use MatButtonModule. 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 Close #48888
diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts
@@ -35,12 +35,19 @@ 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') {
-      item = attrs[i] as string;
-      if (classIndexOf(item.toLowerCase(), cssClassToMatch, 0) !== -1) {
-        return true;
+    if (typeof item === 'string' && isImplicitAttrsSection) {
+      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(value.toLowerCase(), cssClassToMatch, 0) !== -1) {
+          return true;
+        }
       }
     } else if (item === AttributeMarker.Classes) {
       // We found the classes section. Start searching for the class.
@@ -49,6 +56,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;
diff --git a/packages/core/test/acceptance/content_spec.ts b/packages/core/test/acceptance/content_spec.ts
@@ -1321,6 +1321,51 @@ describe('projection', () => {
         expect(fixture.nativeElement).toHaveText('Hello world!');
         expect(xDirectives).toEqual(1);
       });
+
+      it('should work without exception when subelement has both ngIf and class as interpolation',
+         () => {
+           @Component(
+               {selector: 'child-comp', template: '<ng-content select=".nomatch"></ng-content>'})
+           class ChildComp {
+           }
+
+           @Component({
+             selector: 'parent-comp',
+             template: `<child-comp><span *ngIf="true" class="{{'a'}}"></span></child-comp>`
+           })
+           class ParentComp {
+           }
+
+           TestBed.configureTestingModule({declarations: [ParentComp, ChildComp]});
+           const fixture = TestBed.createComponent<ParentComp>(ParentComp);
+
+           fixture.detectChanges();
+           expect(fixture.nativeElement.innerHTML).toBe('<child-comp></child-comp>');
+         });
+    });
+
+    it('selection of child element should properly work even with confusing attribute names', () => {
+      @Component({selector: 'child-comp', template: '<ng-content select=".title"></ng-content>'})
+      class ChildComp {
+      }
+
+      @Component({
+        selector: 'parent-comp',
+        template:
+            `<child-comp><span *ngIf="true" id="5" jjj="class" class="{{'a'}}" [title]="'abc'"></span></child-comp>`
+      })
+      class ParentComp {
+      }
+
+      TestBed.configureTestingModule({declarations: [ParentComp, ChildComp]});
+      const fixture = TestBed.createComponent<ParentComp>(ParentComp);
+
+      fixture.detectChanges();
+      // tNode.attrs will be ['id', '5', 'jjj', 'class', 3 /* AttributeMarker.Bindings */, 'class',
+      // 'title', 4 /* AttributeMarker.Template */, 'ngIf'] isNodeMatchingSelector() must not
+      // confuse it as 'class=title' attribute. <ng-content select=".title"> should not match the
+      // child.
+      expect(fixture.nativeElement.innerHTML).toBe('<child-comp></child-comp>');
     });
   });
 });
