NativeScript / nativescript-angular

Integrating NativeScript with Angular
http://docs.nativescript.org/angular/tutorial/ng-chapter-0
Apache License 2.0
1.21k stars 240 forks source link

feat(modals): easier ViewContainerRef handling #1546

Open NathanWalker opened 5 years ago

NathanWalker commented 5 years ago

Is your feature request related to a problem? Please describe.

Not yet but may create a problem for it (likely related to several other reported issues in all sorts of subtle ways).

Describe the solution you'd like

Right now in order to open a modal you must pass the proper ViewContainerRef. This is simply error prone, not versatile and frustrating for sophisticated app development at scale.

Describe alternatives you've considered

Keeping track of the current ViewContainerRef via a service. This works in a majority of cases but due to race conditions and various asynchronous contexts this has been found to be not reliable. Simply put: requiring developers to use a ViewContainerRef to simply open a modal is highly frustrating.

Additional context

I haven't investigated refactoring modal handling just yet but plan to. There must be a way to handle modals without dealing with ViewContainerRef at all or rather the framework could open modals in the right context all the time without leaving it up for the developer to keep track of it since it has been found to be so error prone.

/cc @ADjenkov @vakrilov @sis0k0

NathanWalker commented 5 years ago

This is the dreaded error that occurs on iOS:

ViewHierarchy: Parent page is not part of the window hierarchy. Close the current modal page before showing another one!

Which means your app is now dead and you can no longer open modals at all and in most cases you can't use the app anymore.

NathanWalker commented 5 years ago

These are the "white screens of death" that apps can result in:

screen shot 2018-10-08 at 1 06 33 pm
NathanWalker commented 5 years ago

Fundamentally I think part of the issue is in core modules and could be an iOS race condition: https://stackoverflow.com/a/7194182 Likely need more fine grain control of modal presenting and dismissing to ensure none overlap in here: https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/ui/core/view/view.ios.ts#L429 and when dealing with this logic: https://github.com/NativeScript/NativeScript/blob/master/tns-core-modules/ui/core/view/view.ios.ts#L366-L377 which is ultimately buggy due to some race condition here.

MartoYankov commented 5 years ago

@NathanWalker The ability to pass different ViewContainerRef instances when opening a modal is very important when you want to have navigation inside the modal view. This navigation must be declared in your route config at a certain level. This must match the level of the ViewContainerRef, so that these paths are correctly matched by the router.

We all agree that opening a modal right now brings with itself a lot of boilerplate code that is not really needed. At the same time we want to provide openness to the API, so that a lot of modal and navigation patterns are supported. Perhaps we should rethink this approach and provide different APIs for the different scenarios.

That being said, the error you get "ViewHierarchy: Parent page is not part of the window hierarchy. Close the current modal page before showing another one!" means that you are passing a ViewContainerRef that is not loaded in your app at this time. It looks like something connected to opening/closing modal views, but you haven't shared the case. Can you share a scenario where you achieved this?

NathanWalker commented 5 years ago

See comments in the PR @MartoYankov thanks for investigating with me 👍

tiagoblackcode commented 5 years ago

@MartoYankov

This navigation must be declared in your route config at a certain level. This must match the level of the ViewContainerRef, so that these paths are correctly matched by the router.

Does this mean that if I have the following configuration:

const routes: Routes = [
  {
    path: 'modal',
    component: ModalComponent,
    children: [
      { path: 'child', component: ModalChildComponent }
    ]
  },
  {
    path: 'modal-initiator',
    component: ModalInitiatorComponent
  }
];

@Injectable()
export class AppViewContainerRef {
  viewContainerRef: ViewContainerRef
}

@Component({
  template: '<page-router-outlet></page-router-outlet>',
})
export class AppComponent {
  constructor(
    private vcr: ViewContainerRef, 
    private appVcr: AppViewContainerRef) {
    this.appVcr = this.vcr;
  }
}

@Component({
  template: '<page-router-outlet></page-router-outlet>',
})
export class ModalComponent {
}

@Component({
  template: '<Label text="Modal Child Component"></Label>',
})
export class ModalChildComponent {
}

export class ModalInitiatorComponent implements OnInit {
  constructor(
    private modalService: ModalDialogService, 
    private appVcr: AppViewContainerRef) { }

  ngOnInit() {
    this.modalService.showModal(ModalComponent, {
      viewContainerRef: this.appVcr
    });
  }
}

the modal presents correctly and the ModalInitiatorComponent does not get destroyed despite the ModalComponent being outside of the ModalInitiatorComponent tree? I've been using Angular's named router outlets to overcome this - and avoid patching the routes at runtime - but there's no page animation between modal pages, which the regular routed modals have - when their routes are children of the component that shows the modal.

The most common use case I see for this is a login modal which has registration and password recovery as accessory pages. You'd want for the modal to be able to display all of these and hence setup routing.

MartoYankov commented 5 years ago

@tiagoblackcode The component that you pass to the showModal() function is the root of the modal view. It gets created dynamically and shown as a modal. There is no need to declare it in the Angular routes config. If you want to have navigation inside this modal view, you have to set this modal component`s root as a page-router-outlet (like you did), declare the routes that will be navigated there, and navigate to them. See the example in the docs - https://docs.nativescript.org/angular/ui/modal-view-ng#modal-view-navigation.

tiagoblackcode commented 5 years ago

Yes, I understand that. The Routes configuration above has an extra route for the ModalComponent which hosts the page-router-outlet, my bad.

However, what I was trying to ask was if the modal routes (ModalChildComponent in the example above) can be outside of the route tree of the caller component (`ModalInitiatorComponent in the example above).

IIRC the issue I encountered if the modal routes were not under the caller tree was that when the modal was opened, the underlying page was destroyed and when the modal closed there was a blank page.

MartoYankov commented 5 years ago

@tiagoblackcode To achieve this you must use angular's named/aux router outlet concept. See this playground example - https://play.nativescript.org/?template=play-ng&id=E6WyBS&v=2.

tiagoblackcode commented 5 years ago

And that's what I've been doing. However, you lose two features: 1) animations between modal page transitions; 2) the ability to pass query parameters around. The 2nd one is not that important but the first one has some impact in the user experience IMHO.

MartoYankov commented 5 years ago

@tiagoblackcode You can navigate between pages in the modal view and there are transitions. I've updated the playground with a navigation inside the modal. See https://play.nativescript.org/?template=play-ng&id=E6WyBS&v=2 .

tiagoblackcode commented 5 years ago

@MartoYankov can confirm the animations are working in the playground. I've updated the playground with the navigation within modals: https://play.nativescript.org/?template=play-ng&id=E6WyBS&v=9 . I'm going to double-check what's happening in the project I'm working.

Thank you very much for taking the time.

tiagoblackcode commented 5 years ago

@MartoYankov

So, in my previous configuration I had modal pages lazy-loaded with the following configuration:

// app.module.ts
{
  path: 'login',
  outlet: 'loginModal',
  loadChildren: './login/login.module#LoginModule
}
// login.module.ts

@NgModule({
  imports: [
    NativeScriptRouterModule.forChild([
      { path: '', component: LoginComponent },
      { path: 'register', component: RegisterComponent },
      { path: 'register-success', component: RegisterSuccessComponent },
      { path: 'reset-password', component: ResetPasswordComponent }
    ]),
    // ...
  ],
  // ...
})
export class LoginModule {

}

With this configuration, the "root" route has the outlet declaration while the child routes don't. While the modals work just fine, the animations don't.

Later, I tried eager loading the routes still nested under a "root" route similar to the above, but with the children entry instead of the loadChildren entry, and still without declaring the outlet in the child routes. The modals work but there are no animations neither RouterExtensions#backToPreviousPage does work (link: https://play.nativescript.org/?template=play-ng&id=E6WyBS&v=28).

e.g.

{
  path: 'login',
  outlet: 'loginModal',
  children: [
    { path: '', component: LoginComponent },
    { path: 'register', component: RegisterComponent },
    { path: 'register-success', component: RegisterSuccessComponent },
    { path: 'reset-password', component: ResetPasswordComponent }
  ] 
}

Lastly, I've added the outlet to the child routes and this time the modals do not work.

So, in conclusion:

Edit: updated a use case

Kind Regards, Tiago Melo

MartoYankov commented 5 years ago

@tiagoblackcode The issues you reported are known. They all boil down to nesting named/aux outlets. There is a PR that should fix them - https://github.com/NativeScript/nativescript-angular/pull/1556.

The issue with lazy loading aux/named outlets is a curios one. The reason why you don't have animations is because Angular is injecting a basic router-outlet in this case. This outlet doesn't have a page, action bar or transitions. The PR will allow a workaround for that.

tiagoblackcode commented 5 years ago

Makes sense it doesn't work like that. I didn't know Angular added a router-outlet in that case. It must do that to handle both children and loadChildren since the behaviour is consistent.

Nice one! Gonna see if I can test it :)

flipperlite commented 6 months ago

I have a fix for error "ViewHierarchy: Parent page is not part of the window hierarchy." with no previous modal open - just warp your showModal line with a setTimeout. Here's a working example.

setTimeout(() => {
  view.showModal('path-to-your-modal', optionsShow) // WATCHOUT - no error if page doesn't exist and no modal will open
}, 0)