Change detection not triggering for template event handler

[daelmaak]

🐞 bug report

Description

I am working on an Angular gallery library (repository) and I have a change detection problem with the navigation arrows. The first navigation arrow looks like this in the template:

<div *ngIf="showPrevArrow" class="prev arrow" (click)="prev()">
  <doe-chevron-icon
    *ngIf="!arrowTemplate; else arrowTemplate"
  ></doe-chevron-icon>
</div>

Important part is the click event handler and ngIf on the div. Now everytime it is clicked, the prev method should be called AND change detection should run. This works most of the time:

But in a certain scenario (see reproduction steps), the change detection is not triggered after click which then looks like this:

When I wrap contents of prev method in ngZone.run(() => {...}, the change detection always fires. That's good, but still, I shouldn't be supposed to do that in such a rudimentary case.

🔬 Minimal Reproduction

1. go to https://stackblitz.com/edit/ngx-doe-gallery-x1mbws
2. in the first gallery, swipe to the 2nd picture
3. now click prev arrow to go back to 1st picture
4. have a look at the image counter, it indicates the second picture is still selected

🌍 Your Environment

Angular Version:

Angular CLI: 8.3.20
Node: 12.16.3
OS: linux x64
Angular: 8.2.14
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package Version

@angular-devkit/architect 0.803.20
@angular-devkit/build-angular 0.803.25
@angular-devkit/build-ng-packagr 0.803.25
@angular-devkit/build-optimizer 0.803.25
@angular-devkit/build-webpack 0.803.25
@angular-devkit/core 8.3.20
@angular-devkit/schematics 8.3.20
@angular/cdk 8.2.3
@angular/cli 8.3.20
@angular/material 8.2.3
@ngtools/webpack 8.3.25
@schematics/angular 8.3.20
@schematics/update 0.803.20
ng-packagr 5.7.1
rxjs 6.4.0
typescript 3.5.3
webpack 4.39.2

Anything else relevant? The issue is not platform or browser specific. I tried with angular 9 with the same results.

[pkozlowski-opensource]

@daelmaak I've noticed that your stackblitz with the reproduce scenario is using Angular v8. A lot changed in v9 / v10 in respect to change detection so could you please try upgrading and see if the error persists?

Also, there is quite a bit going on in this reproduction - it would be super-helpful if you could narrow it down to just one (non-working) case and describe expected vs. actual result.

Finally, could you please copy & paste description from SO here? It would help us to have all the info in one place.

Just to set expectations: this issue is not trivial to investigate as reported. We would very much appreciate your help in getting a smaller / focused repro with the latest scenario.

[daelmaak]

I tried ng 9 again and the problem persists. I also copied the SO Q over. About narrowing the problem down, that won't be easy as this repro is about a library which is an integral whole, but I'll see what I can do.

[daelmaak]

Here is the much smaller reproduction https://stackblitz.com/edit/angular-kea4wi

Steps to reproduce it there:

1. Click at the beige area
2. Button appears, click it too
3. Nothing happens, although message should have appeared below

Description:

1. The beige area has a click event handler registered via addEventListener. Inside it a component's showButton property is set to from false to true and I trigger change detection there manually.
2. Now button appears, which thanks to *ngIf="showButton" wasn't rendered initially, and it has a click even handler declared in the template. This handler again changes component's property, this time showMessage to true.
3. When I click it, the handler obviously runs and changes component's showMessage to true, but it doesn't trigger change detection and message below doesn't appear.

To make the example work, just set showButton to true from the very beginning, and the scenario above works.

[pkozlowski-opensource]

@daelmaak thnx so much for providing a smaller reproduce scenario. This one is much better and easier to understand indeed! I've simplified it a bit further and updated to the latest Angular running ivy: https://stackblitz.com/edit/angular-ivy-pw3xzt?file=package.json

I can now reproduce and see the problem you are reporting, although I think that it works as expected. The crux of the problem, as I understand it, is that you are adding an event handler and running change detection outside of Angular zone. As such any changes in the view created outside of Angular zone won't be picked up. At least this would be suggested by the fact that commenting out runOutsideAngular makes the scenario work: https://stackblitz.com/edit/angular-ivy-splvfy?file=src/app/app.component.ts

I must say that I'm not well versed in zone.js + change detection interactions but @JiaLiPassion should be able to provide more guidance here. So far it looks like "works as designed" for me - any particular reason for doing those operations outside of Angular zone?

[daelmaak]

Glad it helps!

I thought that it is a zone problem also, but I got confused after I checked the zone in the click callback. The inner zone indicated angular zone, and root zone was the outer one, which I guess is how it should be(?) in order for the change detection to work.

I understand your description of what probably happened. it makes sense when you think about it. However, it is a gotcha for me. I had problems with zones before (code executed in non-angular zone causing CD issues), but I never thought the path to such problems can also lead through template-declared event handlers.

The reason I handle events outside of the ng zone is performance. Most of the time, I don't want the events to trigger CD. But sometimes, I still need to perform changes on the component nevertheless, and that's when I call detectChanges in scope of runOutsideAngular to make them detected.

[JiaLiPassion]

@daelmaak, just like @pkozlowski-opensource said, the current behavior is correct, but yeah it is confusing.

The reason is,

1. when the page is rendered, the button is not there, so the event handler is not being bounded.
2. in ngOninit, you add an event listener of the div area outside of ngZone, and when click the area, the event handler will also run outside of ngZone which will not trigger change detection, and you call cdr.detectChanges() which did trigger change detection manually, but still, it is outside of ngZone. And the button is rendered and the button event handler is bounded inside this cycle of change detection, so the event handler is also being bounded outside of ngZone. that is why the button click will not update the page.

So instead of calling this.cdr.detectChanges you can call an empty function inside ngZone.run like this https://stackblitz.com/edit/angular-bndeyd?file=src/app/app.component.ts

But still this is a confusing problem, I will try to add it to the document first, thank you for posting the issue.

[daelmaak]

@JiaLiPassion @pkozlowski-opensource thanks for the explanation!

About calling empty cb from ngZone.run, how come this works? I would understand if it worked while changing component properties INSIDE the callback, but just running empty function?

[JiaLiPassion]

@daelmaak, the reason run empty function inside ngZone.run() works because ngZone.run() does two things.

1. it will run the callback inside angular zone, so the event handler bound to the button will also happen in angular zone.
2. it will trigger change detection automatically after the callback is executed, so you don't need to call detectChanges() yourself.

[daelmaak]

So running the callback in ngZone.run somehow switches the context to angular zone, so that changing component properties in this "new" context makes the button render in the right zone. Interesting.

I tried your suggestion and it works fine 👍

[mhevery]

Here is my understanding: In ngOnInit you escape ngZone and than call this.cd.detectChanges() outside ngZone Since this.cd.detectChanges() causes *ngIf to instantiate a template that template's click handler will be registered outside of the ngZoneas a result clicking on the registered event handler will execute handler function outside of the ngZone hence your observation.

Yes this works as expected (correct behavior) and Yes it is surprising.

So the question is what can we do to make it less surprising.

One mental model which we could adopt is to say that all event handlers registered from the (event) syntax always run with NgZone currently installed no matter how the this.cd.detectChanges() was actually called. This would actually align it nicely with #38236. There would be few perf advantages to this so I think it would be worth a try.

1. Change EventManager so that addEventListener is always done with element.__zone_symbol__addEventListener. This will effectively make sure that all registration will happen outside of the Zone, which should improve performance.
2. Change the function wrapListener so that it will always execute the listenerFn in the NgZone provided.

The result is that no matter how the template was created, we can be sure that he template behaves consistently (We also save on the registration as we don't have to pay for the zone.js registration overhead)

I don't know if such change to the mental model would pass google3 but the only way to know is to implement and try.

NOTE: One place where I could see an issue is that in DebugNode we use zone.js to retrieve callbacks for testing, which we may have to solve in a different way.

[JiaLiPassion]

@mhevery , got it! I agree that all event binding in the template should trigger CD automatically.

Change EventManager so that addEventListener is always done with element.__zone_symbol__addEventListener. This will effectively make sure that all registration will happen outside of the Zone, which should improve performance. Change the function wrapListener so that it will always execute the listenerFn in the NgZone provided.

This should work, but I think we discussed this before, that implementing like this will make all ZoneSpec callbacks not being triggered(such as onScheduleTask or onInvokeTask) which may cause some breaking.

[mhevery]

This should work, but I think we discussed this before, that implementing like this will make all ZoneSpec callbacks not being triggered(such as onScheduleTask or onInvokeTask) which may cause some breaking.

Yes I agree. Maybe we could break this into two separate PRs and try them independently? PR1: would make all (event) registrations run through NgZone PR2: would build up on PR1 and try to use element.__zone_symbol__addEventListener to see how much stuff would break.

[angular-automatic-lock-bot[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.}