Directive Execution Issue: Upgrading from Angular v17.2.0 to v17.3.0-rc.0 - Executing on Non-existent DOM Elements #54798

naveedahmed1 · 2024-03-09T21:23:01Z

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

Yes

Description

I just updated my app from Angular Version 17.2.0 to Version 17.3.0-rc.0 and noticed this strange behaviour

Here's my app component:

export class AppComponent  {

  public showInstallButton = false;
  public isBrowser;
  constructor(public _gs: GlobalService) {

  }
  ngOnInit() {
    this.isBrowser = this._gs.isPlatformBrowser();
  }
}

App Component HTML:

@if (isBrowser) {

@if (showInstallButton) {
<button id="installButton" title="Install Now!" class="material-icon-button" style="padding-top:4px;">
  <i class="icon icon-file_download white"></i>
</button>
}
}

And a Ripple Directive:

import { Directive, ElementRef, HostListener } from '@angular/core';

@Directive({
  selector: 'button,[ripple],.material-icon-button',
  standalone: true
})
export class RippleDirective {

  @HostListener('mousedown', ['$event'])
  createRipple(event) {

    const element = event.currentTarget;

    const circle = document.createElement("span");
    const diameter = Math.max(element.clientWidth, element.clientHeight);
    const radius = diameter / 2;

    // Setup
    const elemPosX = element.getBoundingClientRect().left,
          elemPosY = element.getBoundingClientRect().top;


    // Get the center of the element
    const x = event.pageX - elemPosX - radius;
    const y = event.pageY - elemPosY - radius;


    circle.style.width = circle.style.height = `${diameter}px`;
    circle.style.left = `${x}px`;
    circle.style.top = `${y}px`;


    circle.classList.add("ripple");

    circle.addEventListener('animationend', () => {
      element.removeChild(circle);
    });

    element.appendChild(circle);

  }

  constructor(
    private elementRef: ElementRef
  ) {
    elementRef.nativeElement.style.position = 'relative';
    elementRef.nativeElement.style.overflow = 'hidden';
  }
}

My assumption is the directive code sholdn't execute in this case since the value of showInstallButton is false.

But it seems that its still executing the directive code and throwing below error:

main.ts:5 ERROR TypeError: Cannot set properties of undefined (setting 'position')
    at new _RippleDirective (ripple.directive.ts:46:36)
    at NodeInjectorFactory.RippleDirective_Factory [as factory] (ripple.directive.ts:48:3)
    at getNodeInjectable (core.mjs:5991:44)
    at instantiateAllDirectives (core.mjs:11353:27)
    at createDirectivesInstances (core.mjs:10752:5)
    at ɵɵtemplate (core.mjs:17888:9)
    at AppComponent_Conditional_0_Template (app.component.html:3:1)
    at executeTemplate (core.mjs:10714:9)
    at renderView (core.mjs:11916:13)
    at createAndRenderEmbeddedLView (core.mjs:11986:9)

The error points to these lines in directive:

elementRef.nativeElement.style.position = 'relative';
    elementRef.nativeElement.style.overflow = 'hidden';

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

main.ts:5 ERROR TypeError: Cannot set properties of undefined (setting 'position')
    at new _RippleDirective (ripple.directive.ts:46:36)
    at NodeInjectorFactory.RippleDirective_Factory [as factory] (ripple.directive.ts:48:3)
    at getNodeInjectable (core.mjs:5991:44)
    at instantiateAllDirectives (core.mjs:11353:27)
    at createDirectivesInstances (core.mjs:10752:5)
    at ɵɵtemplate (core.mjs:17888:9)
    at AppComponent_Conditional_0_Template (app.component.html:3:1)
    at executeTemplate (core.mjs:10714:9)
    at renderView (core.mjs:11916:13)
    at createAndRenderEmbeddedLView (core.mjs:11986:9)

Please provide the environment you discovered this bug in (run `ng version`)

Angular CLI: 17.3.0-rc.0
Node: 20.11.0
Package Manager: npm 10.5.0
OS: win32 x64

Angular: 17.3.0-rc.0
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, platform-server
... pwa, router, service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1703.0-rc.0
@angular-devkit/build-angular   17.3.0-rc.0
@angular-devkit/core            17.2.0
@angular-devkit/schematics      17.2.0
@angular/cdk                    17.2.0-rc.0
@angular/fire                   17.0.0
@angular/google-maps            17.2.0-rc.0
@angular/material               17.2.0-rc.0
@schematics/angular             17.2.0
rxjs                            7.8.1
typescript                      5.3.3
zone.js                         0.14.4

Anything else?

No response

The text was updated successfully, but these errors were encountered:

JeanMeche · 2024-03-09T21:35:03Z

Hi, could you provide a stackblitz reproduction of this issue ?
The error you are reporting indicates that elementRef.nativeElement exists but doesn't have a style property.

JeanMeche · 2024-03-09T21:45:15Z

Ok, I have a repro: https://stackblitz.com/edit/angular-issue-54798?file=package.json,src%2Fmain.ts&template=node

The directive is constructed twice when the selector is a class. The first time the element ref is a comment node. It only happens when it's wrapped by an @if block.

naveedahmed1 · 2024-03-09T22:05:26Z

Thank you @JeanMeche for looking into this. But since the if condition is false, it shouldn't be constructed at all, right?

It was working fine in v17.2.0.

JeanMeche · 2024-03-09T22:07:11Z

You're absolutely right, there is a regression a play here.

naveedahmed1 · 2024-03-09T22:13:18Z

I removed everything from my app component and left the exact same code that I have mentioned above and its still throwing error.

I also tried printing the values of both variables in if conditions:

isBrowser: true
showInstallButton: false

So button indeed doesnt exist in dom. Therefore the directive shouldn't execute.

alxhub · 2024-03-10T02:05:27Z

Thanks for testing the RC ❤️ I believe this is the first template pipeline bug found "in the wild" - congrats!

…rom directive matching 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 class 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 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 angular#54798

…rom directive matching 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 angular#54798

dylhunn · 2024-03-11T16:06:01Z

More discussion in #54800 -- Joost has fixed this in the runtime, but I'm wondering whether we should also change Template Pipeline to match the TDB const array.

…rom directive matching 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 angular#54798

…rom directive matching (#54800) 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

angular-automatic-lock-bot · 2024-04-12T00:09:12Z

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.}

JeanMeche added regression Indicates than the issue relates to something that worked in a previous version compiler: template pipeline labels Mar 9, 2024

alxhub added this to the v17.3 milestone Mar 10, 2024

ngbot bot removed this from the v17.3 milestone Mar 10, 2024

JoostK mentioned this issue Mar 10, 2024

fix(core): exclude class attribute intended for projection matching f… #54800

Closed

JoostK added state: has PR P2 The issue is important to a large percentage of users, with a workaround labels Mar 10, 2024

JoostK added this to the v17.3 milestone Mar 10, 2024

ngbot bot removed this from the v17.3 milestone Mar 10, 2024

atscott closed this as completed in e6ee6d2 Mar 12, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directive Execution Issue: Upgrading from Angular v17.2.0 to v17.3.0-rc.0 - Executing on Non-existent DOM Elements #54798

Directive Execution Issue: Upgrading from Angular v17.2.0 to v17.3.0-rc.0 - Executing on Non-existent DOM Elements #54798

naveedahmed1 commented Mar 9, 2024

JeanMeche commented Mar 9, 2024

JeanMeche commented Mar 9, 2024 •

edited

naveedahmed1 commented Mar 9, 2024

JeanMeche commented Mar 9, 2024 •

edited

naveedahmed1 commented Mar 9, 2024

alxhub commented Mar 10, 2024

dylhunn commented Mar 11, 2024

angular-automatic-lock-bot bot commented Apr 12, 2024

Directive Execution Issue: Upgrading from Angular v17.2.0 to v17.3.0-rc.0 - Executing on Non-existent DOM Elements #54798

Directive Execution Issue: Upgrading from Angular v17.2.0 to v17.3.0-rc.0 - Executing on Non-existent DOM Elements #54798

Comments

naveedahmed1 commented Mar 9, 2024

Which @angular/* package(s) are the source of the bug?

Is this a regression?

Description

Please provide a link to a minimal reproduction of the bug

Please provide the exception or error you saw

Please provide the environment you discovered this bug in (run ng version)

Anything else?

JeanMeche commented Mar 9, 2024

JeanMeche commented Mar 9, 2024 • edited

naveedahmed1 commented Mar 9, 2024

JeanMeche commented Mar 9, 2024 • edited

naveedahmed1 commented Mar 9, 2024

alxhub commented Mar 10, 2024

dylhunn commented Mar 11, 2024

angular-automatic-lock-bot bot commented Apr 12, 2024

Please provide the environment you discovered this bug in (run `ng version`)

JeanMeche commented Mar 9, 2024 •

edited

JeanMeche commented Mar 9, 2024 •

edited