[angular 9] child pages initially added to incorrect container element in Ionic Angular apps

[liamdebeasi]

🐞 bug report

Affected Package

The issue is caused by package @angular/routing

Is this a regression?

Yes, the previous version in which this bug was not present was: Angular 8.x

Description

We are running into an issue with navigation on Angular 9. It seems that after the upgrade, Angular is inserting pages into the wrong container component.
We have the following DOM layout:

app-root (the main component for the Angular app)
|
|_____ion-app (a web component that manages Ionic utilities)
|     |
|_____|_____ion-router-outlet (a web component that is a wrapper around Angular's router outlet)
|     |     |
|_____|_____|_____app-home (the main page of our application)

Clicking a button in app-home navigates you to app-child. In Angular <=8, this child page is added as a child inside of ion-router-outlet. With Angular 9, this page is initially added as child of ion-app, and then re-added as a child of ion-router-outlet . This is causing issues with our users’ apps as certain components no longer work on subsequent navigations to the child page.

🔬 Minimal Reproduction

Angular 8 repro: https://github.com/liamdebeasi/ng8-routing-repro
Angular 9 repro: https://github.com/liamdebeasi/ng9-routing-repro

Please see repo README files for steps to reproduce.

🌍 Your Environment

Angular Version:

Angular CLI: 9.0.1
Node: 10.15.0
OS: darwin x64

Angular: 9.0.0
... common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.801.3
@angular-devkit/build-angular     0.900.1
@angular-devkit/build-optimizer   0.900.1
@angular-devkit/build-webpack     0.900.1
@angular-devkit/core              9.0.1
@angular-devkit/schematics        9.0.1
@angular/cli                      9.0.1
@ngtools/webpack                  9.0.1
@schematics/angular               8.1.3
@schematics/update                0.900.1
rxjs                              6.5.4
typescript                        3.7.5
webpack                           4.41.2

Anything else relevant?

This is an Ionic Framework v5 application, but we have not changed anything related to our routing setup with regards to Angular 9.

[atscott]

Hi @liamdebeasi, thanks for reporting this issue. As far as I can see, the dom manipulations are behaving according to what Ionic is doing. activateWith calls createComponent which inserts the created component to the dom (this is the same in v8 and v9). Then the StackController calls appendChild with the app-child. Since it's already inserted, this acts as a move action. This issue seems not to be caused by Angular. Please contact the author(s) of project Ionic or file issue on their issue tracker.

[liamdebeasi]

Thanks for the reply. I am one of the Ionic developers, and we recently released an update that adds Angular 9 support to the framework. If you follow the additional debugging steps in the repos I included you should see that with Angular 9, multiple instances of our components are being created. On Angular 8 this is not happening.

From what I can tell, routing in Angular 9 now thinks that a component's parent is ion-app instead of ion-router-outlet (which is what routing in Angular 8 thinks it is). The StackController is calling appendChild again because of this line: https://github.com/ionic-team/ionic/blob/master/angular/src/directives/navigation/stack-controller.ts#L243-L245.

Ionic knows that the containerEl is ion-router-outlet, and in Angular 8 the component being added (enteringEl) has a parent element of ion-router-outlet which causes the check in the link I included to (correctly) fail. On Angular 9, the parent element for the component being added is now ion-app, which causes the check in the link I included to (incorrectly) pass.

While this could very well be something we need to fix in Ionic Angular, the only change that has caused this issue was upgrading to Angular 9. Does Angular 9 change the way it determines how to insert elements into the DOM? I looked through the breaking changes, but I could not find anything that would suggest this.

Thanks!

[atscott]

Hi Liam, it could take a while to unpack what's going on here since I'm not familiar with Ionic. The short answer is yes, Angular 9 does have a different internal representation of the dom. Does Ionic frequently do native dom manipulation like what StackController does? Moving around nodes that were created by the Angular framework means that the dom will not match Angular's internal tracking and there's no way for us to know about it (aside from querying the dom, which would be slow). If ion-router-outlet or ion-app were moved, then this could be the source of the problem.

[liamdebeasi]

No worries, I'll try to debug more and hopefully I can pinpoint a bit more accurately where the source of this change seems to be coming from (probably tomorrow).

Other than the code I sent, we typically stay away from messing with the native DOM and try to let Angular do the heavy lifting on that. That particular line was added to accommodate a change in Angular 6.1 regarding restoring scroll positions. I believe at the time Angular used the document body to work with scrolling (which does not work if you have nested scroll containers).

Regarding ion-router-outlet and ion-app, we do not move those around either.

[atscott]

Hey Liam, I think the issue is almost certainly that native DOM manipulation. If you notice, in v9 the app-home acts the same way as the app-child for insertion (it's first inserted into ion-app then moved to ion-router-outlet). It's kind of involved how the app-child is finding where to insert, but essentially pre-v9 identifies the parent node for app-child as ion-router-outlet because app-home was moved there whereas v9 inserts where the comment node is (a child of ion-app). This is actually more consistent - all the nodes are inserted to ion-app in v9 and then you move them to the router. Pre-v9, only the first component would do this and subsequent ones would, in a roundabout way (find the last sibling that was inserted, then get its parent), identify the ion-router-outlet as the parent.

[liamdebeasi]

Ahh okay yep that makes sense. I agree the v9 way is much more consistent 👍. Is this new behavior considered a breaking change?

[atscott]

Yes, this is related to the last bullet point in the breaking changes:

Embedded views (such as ones created by *ngFor) are now inserted in front of anchor DOM comment node (e.g. ) rather than behind it as was the case previously. In most cases this does not have any impact on rendered DOM. In some cases (such as animations delaying the removal of an embedded view) any new embedded views will be inserted after the embedded view being animated away. This difference only last while the animation is active, and might alter the visual appearance of the animation. Once the animation is finished the resulting rendered DOM is identical to that rendered with View Engine.

It has to do with the indexes of the ViewContainerRef and how it determines siblings. https://github.com/ionic-team/ionic/blob/master/angular/src/directives/navigation/ion-router-outlet.ts#L201. But essentially if you're moving nodes around yourself, it could coincidentally work with a current implementation and then break if we change how we track nodes.

[liamdebeasi]

Ok thanks for all the help!

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