angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
96.08k stars 25.43k forks source link

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

Closed naveedahmed1 closed 7 months ago

naveedahmed1 commented 7 months ago

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

JeanMeche commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

naveedahmed1 commented 7 months ago

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 commented 7 months ago

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

dylhunn commented 7 months ago

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.

angular-automatic-lock-bot[bot] commented 6 months ago

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.