fix(core): exclude class attribute intended for projection matching from directive matching (#54800)

JoostK · atscott · commit 243ccce62475 · 2024-03-12T14:05:18.000-07:00
This commit resolves a regression that was introduced when the compiler switched from `TemplateDefinitionBuilder` (TDB) to the template pipeline (TP) compiler. The TP compiler has changed the output of ```html if (false) { <div class="test"></div> } ``` from ```ts defineComponent({ consts: [['class', 'test'], [AttributeMarker.Classes, 'test']], template: function(rf) { if (rf & 1) { ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0) } } }); ``` to ```ts defineComponent({ consts: [[AttributeMarker.Classes, 'test']], template: function(rf) { if (rf & 1) { ɵɵtemplate(0, App_Conditional_0_Template, 2, 0, "div", 0) } } }); ``` The last argument to the `ɵɵtemplate` instruction (0 in both compilation outputs) corresponds with the index in `consts` of the element's attribute's, and we observe how TP has allocated only a single attribute array for the `div`, where there used to be two `consts` entries with TDB. Consequently, the `ɵɵtemplate` instruction is now effectively referencing a different attributes array, where the distinction between the `"class"` attribute vs. the `AttributeMarker.Classes` distinction affects the behavior: TP's emit causes the runtime to incorrectly match a directive with `selector: '.foo'` to be instantiated on the `ɵɵtemplate` instruction as if it corresponds with a structural directive! Instead of changing TP to align with TDB's emit, this commit updates the runtime instead. This uncovered an inconsistency in selector matching for class names, where there used to be two paths dealing with class matching: 1. The first check was commented to be a special-case for class matching, implemented in `isCssClassMatching`. 2. The second path was part of the main selector matching algorithm, where `findAttrIndexInNode` was being used to find the start position in `tNode.attrs` to match the selector's value against. The second path only considers `AttributeMarker.Classes` values if matching for content projection, OR of the `TNode` is not an inline template. The special-case in path 1 however does not make that distinction, so it would consider the `AttributeMarker.Classes` binding as a selector match, incorrectly causing a directive to match on the `ɵɵtemplate` itself. The second path was also buggy for class bindings, as the return value of `classIndexOf` was incorrectly negated: it considered a matching class attribute as non-matching and vice-versa. This bug was not observable because of another issue, where the class-handling in part 2 was never relevant because of the special-case in part 1. This commit separates path 1 entirely from path 2 and removes the buggy class-matching logic in part 2, as that is entirely handled by path 1 anyway. `isCssClassMatching` is updated to exclude class bindings from being matched for inline templates. Fixes #54798 PR Close #54800
diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts
@@ -10,28 +10,25 @@ import '../util/ng_dev_mode';
 
 import {assertDefined, assertEqual, assertNotEqual} from '../util/assert';
 
+import {AttributeMarker} from './interfaces/attribute_marker';
 import {TAttributes, TNode, TNodeType} from './interfaces/node';
 import {CssSelector, CssSelectorList, SelectorFlags} from './interfaces/projection';
 import {classIndexOf} from './styling/class_differ';
 import {isNameOnlyAttributeMarker} from './util/attrs_utils';
-import { AttributeMarker } from './interfaces/attribute_marker';
 
 const NG_TEMPLATE_SELECTOR = 'ng-template';
 
 /**
  * Search the `TAttributes` to see if it contains `cssClassToMatch` (case insensitive)
  *
+ * @param tNode static data of the node to match
  * @param attrs `TAttributes` to search through.
  * @param cssClassToMatch class to match (lowercase)
  * @param isProjectionMode Whether or not class matching should look into the attribute `class` in
  *    addition to the `AttributeMarker.Classes`.
  */
 function isCssClassMatching(
-    attrs: TAttributes, cssClassToMatch: string, isProjectionMode: boolean): boolean {
-  // TODO(misko): The fact that this function needs to know about `isProjectionMode` seems suspect.
-  // It is strange to me that sometimes the class information comes in form of `class` attribute
-  // and sometimes in form of `AttributeMarker.Classes`. Some investigation is needed to determine
-  // if that is the right behavior.
+    tNode: TNode, attrs: TAttributes, cssClassToMatch: string, isProjectionMode: boolean): boolean {
   ngDevMode &&
       assertEqual(
           cssClassToMatch, cssClassToMatch.toLowerCase(), 'Class name expected to be lowercase.');
@@ -51,14 +48,20 @@ function isCssClassMatching(
         }
       }
     } else if (item === AttributeMarker.Classes) {
+      if (!isProjectionMode && isInlineTemplate(tNode)) {
+        // Matching directives (i.e. when not matching for projection mode) should not consider the
+        // class bindings that are present on inline templates, as those class bindings only target
+        // the root node of the template, not the template itself.
+        return false;
+      }
       // We found the classes section. Start searching for the class.
       while (i < attrs.length && typeof (item = attrs[i++]) == 'string') {
         // while we have strings
         if (item.toLowerCase() === cssClassToMatch) return true;
       }
       return false;
     } else if (typeof item === 'number') {
-      // We've came across a first marker, which indicates
+      // We've come across a first marker, which indicates
       // that the implicit attribute section is over.
       isImplicitAttrsSection = false;
     }
@@ -96,7 +99,7 @@ function hasTagAndTypeMatch(
 /**
  * A utility function to match an Ivy node static data against a simple CSS selector
  *
- * @param node static data of the node to match
+ * @param tNode static data of the node to match
  * @param selector The selector to try matching against the node.
  * @param isProjectionMode if `true` we are matching for content projection, otherwise we are doing
  * directive matching.
@@ -106,10 +109,10 @@ export function isNodeMatchingSelector(
     tNode: TNode, selector: CssSelector, isProjectionMode: boolean): boolean {
   ngDevMode && assertDefined(selector[0], 'Selector should have a tag name');
   let mode: SelectorFlags = SelectorFlags.ELEMENT;
-  const nodeAttrs = tNode.attrs || [];
+  const nodeAttrs = tNode.attrs;
 
   // Find the index of first attribute that has no value, only a name.
-  const nameOnlyMarkerIdx = getNameOnlyMarkerIndex(nodeAttrs);
+  const nameOnlyMarkerIdx = nodeAttrs !== null ? getNameOnlyMarkerIndex(nodeAttrs) : 0;
 
   // When processing ":not" selectors, we skip to the next ":not" if the
   // current one doesn't match
@@ -139,22 +142,15 @@ export function isNodeMatchingSelector(
         if (isPositive(mode)) return false;
         skipToNextSelector = true;
       }
-    } else {
-      const selectorAttrValue = mode & SelectorFlags.CLASS ? current : selector[++i];
-
-      // special case for matching against classes when a tNode has been instantiated with
-      // class and style values as separate attribute values (e.g. ['title', CLASS, 'foo'])
-      if ((mode & SelectorFlags.CLASS) && tNode.attrs !== null) {
-        if (!isCssClassMatching(tNode.attrs, selectorAttrValue as string, isProjectionMode)) {
-          if (isPositive(mode)) return false;
-          skipToNextSelector = true;
-        }
-        continue;
+    } else if (mode & SelectorFlags.CLASS) {
+      if (nodeAttrs === null || !isCssClassMatching(tNode, nodeAttrs, current, isProjectionMode)) {
+        if (isPositive(mode)) return false;
+        skipToNextSelector = true;
       }
-
-      const attrName = (mode & SelectorFlags.CLASS) ? 'class' : current;
+    } else {
+      const selectorAttrValue = selector[++i];
       const attrIndexInNode =
-          findAttrIndexInNode(attrName, nodeAttrs, isInlineTemplate(tNode), isProjectionMode);
+          findAttrIndexInNode(current, nodeAttrs, isInlineTemplate(tNode), isProjectionMode);
 
       if (attrIndexInNode === -1) {
         if (isPositive(mode)) return false;
@@ -169,18 +165,15 @@ export function isNodeMatchingSelector(
         } else {
           ngDevMode &&
               assertNotEqual(
-                  nodeAttrs[attrIndexInNode], AttributeMarker.NamespaceURI,
+                  nodeAttrs![attrIndexInNode], AttributeMarker.NamespaceURI,
                   'We do not match directives on namespaced attributes');
           // we lowercase the attribute value to be able to match
           // selectors without case-sensitivity
           // (selectors are already in lowercase when generated)
-          nodeAttrValue = (nodeAttrs[attrIndexInNode + 1] as string).toLowerCase();
+          nodeAttrValue = (nodeAttrs![attrIndexInNode + 1] as string).toLowerCase();
         }
 
-        const compareAgainstClassName = mode & SelectorFlags.CLASS ? nodeAttrValue : null;
-        if (compareAgainstClassName &&
-                classIndexOf(compareAgainstClassName, selectorAttrValue as string, 0) !== -1 ||
-            mode & SelectorFlags.ATTRIBUTE && selectorAttrValue !== nodeAttrValue) {
+        if (mode & SelectorFlags.ATTRIBUTE && selectorAttrValue !== nodeAttrValue) {
           if (isPositive(mode)) return false;
           skipToNextSelector = true;
         }
diff --git a/packages/core/test/acceptance/control_flow_if_spec.ts b/packages/core/test/acceptance/control_flow_if_spec.ts
@@ -659,5 +659,52 @@ describe('control flow - if', () => {
       expect(directiveCount).toBe(1);
       expect(fixture.nativeElement.textContent).toBe('Main: Before  After Slot: foo');
     });
+
+    it('should not match a directive with a class-based selector only meant for content projection',
+       () => {
+         let directiveCount = 0;
+
+         @Component({
+           standalone: true,
+           selector: 'test',
+           template: 'Main: <ng-content/> Slot: <ng-content select=".foo"/>',
+         })
+         class TestComponent {
+         }
+
+         @Directive({
+           selector: '.foo',
+           standalone: true,
+         })
+         class TemplateDirective {
+           constructor() {
+             directiveCount++;
+           }
+         }
+
+         @Component({
+           standalone: true,
+           imports: [TestComponent, TemplateDirective],
+           template: `<test>Before @if (condition) {
+          <div class="foo">foo</div>
+      } After</test>
+      `
+         })
+         class App {
+           condition = false;
+         }
+
+         const fixture = TestBed.createComponent(App);
+         fixture.detectChanges();
+
+         expect(directiveCount).toBe(0);
+         expect(fixture.nativeElement.textContent).toBe('Main: Before  After Slot: ');
+
+         fixture.componentInstance.condition = true;
+         fixture.detectChanges();
+
+         expect(directiveCount).toBe(1);
+         expect(fixture.nativeElement.textContent).toBe('Main: Before  After Slot: foo');
+       });
   });
 });
diff --git a/packages/core/test/acceptance/directive_spec.ts b/packages/core/test/acceptance/directive_spec.ts
@@ -186,6 +186,49 @@ describe('directives', () => {
       expect(nodesWithDirective.length).toBe(1);
     });
 
+    it('should match class selectors on ng-template', () => {
+      @Directive({selector: '.titleDir'})
+      class TitleClassDirective {
+      }
+
+      TestBed.configureTestingModule({declarations: [TestComponent, TitleClassDirective]});
+      TestBed.overrideTemplate(TestComponent, `
+        <ng-template class="titleDir"></ng-template>
+      `);
+
+      const fixture = TestBed.createComponent(TestComponent);
+      const nodesWithDirective =
+          fixture.debugElement.queryAllNodes(By.directive(TitleClassDirective));
+
+      expect(nodesWithDirective.length).toBe(1);
+    });
+
+    it('should NOT match class selectors on ng-template created by * syntax', () => {
+      @Directive({selector: '.titleDir'})
+      class TitleClassDirective {
+      }
+
+      @Component({selector: 'test-cmp', template: `<div *ngIf="condition" class="titleDir"></div>`})
+      class TestCmp {
+        condition = false;
+      }
+
+      TestBed.configureTestingModule({declarations: [TestCmp, TitleClassDirective]});
+
+      const fixture = TestBed.createComponent(TestCmp);
+
+      const initialNodesWithDirective =
+          fixture.debugElement.queryAllNodes(By.directive(TitleClassDirective));
+      expect(initialNodesWithDirective.length).toBe(0);
+
+      fixture.componentInstance.condition = true;
+      fixture.detectChanges();
+
+      const changedNodesWithDirective =
+          fixture.debugElement.queryAllNodes(By.directive(TitleClassDirective));
+      expect(changedNodesWithDirective.length).toBe(1);
+    });
+
     it('should NOT match classes to directive selectors', () => {
       TestBed.configureTestingModule({declarations: [TestComponent, TitleDirective]});
       TestBed.overrideTemplate(TestComponent, `
