From 0ed7de6c975c444e62788c017e27a1310e57f982 Mon Sep 17 00:00:00 2001 From: Alexander Djenkov Date: Tue, 26 Feb 2019 13:31:03 +0200 Subject: [PATCH] fix(location-strategy): crash on going back with router-outlet after closing modal (#1748) * fix(location-strategy): crash on going back with router-outlet after closing modal * tests: add e2e tests in single-page app * chore: bump version to 7.2.2 --- e2e/single-page/app/app.css | 2 +- e2e/single-page/app/app.module.ts | 5 ++ .../app/second/modal/modal.component.html | 4 ++ .../app/second/modal/modal.component.ts | 19 +++++++ .../app/second/second.component.ts | 22 ++++++++- e2e/single-page/e2e/tests.e2e-spec.ts | 32 +++++++++--- nativescript-angular/directives/dialogs.ts | 4 +- nativescript-angular/package.json | 2 +- .../router/ns-location-strategy.ts | 49 ++++++++++++------- .../router/page-router-outlet.ts | 9 +--- 10 files changed, 108 insertions(+), 40 deletions(-) create mode 100644 e2e/single-page/app/second/modal/modal.component.html create mode 100644 e2e/single-page/app/second/modal/modal.component.ts diff --git a/e2e/single-page/app/app.css b/e2e/single-page/app/app.css index c9e18996d..6e4e9cee1 100644 --- a/e2e/single-page/app/app.css +++ b/e2e/single-page/app/app.css @@ -1,5 +1,5 @@ .title { - font-size: 30; + font-size: 15; margin: 16; } diff --git a/e2e/single-page/app/app.module.ts b/e2e/single-page/app/app.module.ts index 30b4f6cdf..d2187c691 100644 --- a/e2e/single-page/app/app.module.ts +++ b/e2e/single-page/app/app.module.ts @@ -11,14 +11,19 @@ import { AppComponent } from "./app.component"; import { rendererTraceCategory, viewUtilCategory, routeReuseStrategyTraceCategory, routerTraceCategory } from "nativescript-angular/trace"; import { setCategories, enable } from "tns-core-modules/trace"; +import { ModalComponent } from "./second/modal/modal.component"; setCategories(routerTraceCategory + "," + routeReuseStrategyTraceCategory); enable(); @NgModule({ declarations: [ AppComponent, + ModalComponent, ...navigatableComponents, ], + entryComponents:[ + ModalComponent + ], bootstrap: [AppComponent], providers: [ { provide: NgModuleFactoryLoader, useClass: NSModuleFactoryLoader } diff --git a/e2e/single-page/app/second/modal/modal.component.html b/e2e/single-page/app/second/modal/modal.component.html new file mode 100644 index 000000000..99e8c4358 --- /dev/null +++ b/e2e/single-page/app/second/modal/modal.component.html @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/e2e/single-page/app/second/modal/modal.component.ts b/e2e/single-page/app/second/modal/modal.component.ts new file mode 100644 index 000000000..6a954147a --- /dev/null +++ b/e2e/single-page/app/second/modal/modal.component.ts @@ -0,0 +1,19 @@ +import { Component } from '@angular/core'; +import { ModalDialogParams } from "nativescript-angular/modal-dialog"; + +@Component({ + moduleId: module.id, + selector: 'modal', + templateUrl: './modal.component.html' +}) + +export class ModalComponent { + + constructor(private params: ModalDialogParams) { + } + + public close() { + this.params.closeCallback(); + } + +} \ No newline at end of file diff --git a/e2e/single-page/app/second/second.component.ts b/e2e/single-page/app/second/second.component.ts index aa372c895..f06bb1299 100644 --- a/e2e/single-page/app/second/second.component.ts +++ b/e2e/single-page/app/second/second.component.ts @@ -1,10 +1,12 @@ -import { Component, OnInit, OnDestroy } from "@angular/core"; +import { Component, OnInit, OnDestroy, ViewContainerRef } from "@angular/core"; import { ActivatedRoute, Router, Route } from "@angular/router"; +import { ModalDialogService, ModalDialogOptions } from "nativescript-angular"; import { Page } from "tns-core-modules/ui/page"; import { Observable } from "rxjs"; import { map } from "rxjs/operators"; import { RouterExtensions } from "nativescript-angular/router"; +import { ModalComponent } from "./modal/modal.component"; @Component({ selector: "second", @@ -19,12 +21,16 @@ import { RouterExtensions } from "nativescript-angular/router"; + ` }) export class SecondComponent implements OnInit, OnDestroy { public id$: Observable; - constructor(route: ActivatedRoute, private routerExtensions: RouterExtensions) { + constructor(route: ActivatedRoute, + private routerExtensions: RouterExtensions, + private viewContainerRef: ViewContainerRef, + private modalService: ModalDialogService) { this.id$ = route.params.pipe(map(r => +r["id"])); } @@ -39,4 +45,16 @@ export class SecondComponent implements OnInit, OnDestroy { back() { this.routerExtensions.back(); } + + onShowModal() { + let options: ModalDialogOptions = { + viewContainerRef: this.viewContainerRef, + context: { + }, + fullscreen: true + }; + + this.modalService.showModal(ModalComponent, options).then((dialogResult: string) => { + }); + } } \ No newline at end of file diff --git a/e2e/single-page/e2e/tests.e2e-spec.ts b/e2e/single-page/e2e/tests.e2e-spec.ts index 54dc65bc1..4a9857538 100644 --- a/e2e/single-page/e2e/tests.e2e-spec.ts +++ b/e2e/single-page/e2e/tests.e2e-spec.ts @@ -19,29 +19,45 @@ describe("Single page app", () => { }); it("should load second(1) page", async () => { - await findAndClick(driver, "SECOND(1)") + await findAndClick(driver, "SECOND(1)"); await driver.findElementByAutomationText("Second Component: 1"); - + // ActionBar Title and item await driver.findElementByAutomationText("Second Title"); await driver.findElementByAutomationText("ACTION2"); }); it("should load second(2) page", async () => { - await findAndClick(driver, "SECOND(2)") + await findAndClick(driver, "SECOND(2)"); + + await driver.findElementByAutomationText("Second Component: 2"); - await driver.findElementByAutomationText("Second Component: 1"); - // ActionBar Title and items await driver.findElementByAutomationText("Second Title"); await driver.findElementByAutomationText("ACTION2"); await driver.findElementByAutomationText("ADD"); }); + + it("should open and close modal view", async () => { + await findAndClick(driver, "Show Modal"); + + await driver.findElementByAutomationText("Welcome to modal"); + await findAndClick(driver, "Close Modal"); + + await driver.findElementByAutomationText("Second Component: 2"); + }); + + it("should go back to second(1) and first", async () => { + await findAndClick(driver, "Back"); + await driver.findElementByAutomationText("Second Component: 1"); + await findAndClick(driver, "Back"); + await driver.findElementByAutomationText("First Title"); + await driver.findElementByAutomationText("ACTION1"); + }); }); async function findAndClick(driver: AppiumDriver, text: string) { - const navigationButton = - await driver.findElementByAutomationText(text); - navigationButton.click(); + const navigationButton = await driver.findElementByAutomationText(text); + await navigationButton.click(); } \ No newline at end of file diff --git a/nativescript-angular/directives/dialogs.ts b/nativescript-angular/directives/dialogs.ts index 899c54097..7c8b45739 100644 --- a/nativescript-angular/directives/dialogs.ts +++ b/nativescript-angular/directives/dialogs.ts @@ -87,9 +87,7 @@ export class ModalDialogService { frame = (parentView.page && parentView.page.frame) || topmost(); } - if (frame) { - this.location._beginModalNavigation(frame); - } + this.location._beginModalNavigation(frame); return new Promise((resolve, reject) => { setTimeout(() => { diff --git a/nativescript-angular/package.json b/nativescript-angular/package.json index dc9c748db..f63220905 100644 --- a/nativescript-angular/package.json +++ b/nativescript-angular/package.json @@ -1,6 +1,6 @@ { "name": "nativescript-angular", - "version": "7.2.2", + "version": "7.2.3", "description": "An Angular renderer that lets you build mobile apps with NativeScript.", "homepage": "https://www.nativescript.org/", "bugs": "https://github.com/NativeScript/nativescript-angular/issues", diff --git a/nativescript-angular/router/ns-location-strategy.ts b/nativescript-angular/router/ns-location-strategy.ts index 7ac6331ba..6eab73c06 100644 --- a/nativescript-angular/router/ns-location-strategy.ts +++ b/nativescript-angular/router/ns-location-strategy.ts @@ -275,9 +275,13 @@ export class NSLocationStrategy extends LocationStrategy { if (!outlet) { const topmostFrame = this.frameService.getFrame(); - this.currentOutlet = this.getOutletByFrame(topmostFrame); + this.currentOutlet = this.getOutletByFrame(topmostFrame) || this.currentOutlet; + } + + const frameToBack: Frame = this.currentOutlet.getFrameToBack(); + if (frameToBack) { + frameToBack.goBack(); } - this.currentOutlet.getFrameToBack().goBack(); } else { // Nested navigation - just pop the state if (isLogEnabled()) { @@ -385,10 +389,11 @@ export class NSLocationStrategy extends LocationStrategy { routerLog("NSLocationStrategy._beginModalNavigation()"); } - this.currentOutlet = this.getOutletByFrame(frame); + this.currentOutlet = this.getOutletByFrame(frame) || this.currentOutlet; // It is possible to have frame, but not corresponding Outlet, if - // showing modal dialog on app.component.ts ngOnInit() e.g. + // showing modal dialog on app.component.ts ngOnInit() e.g. In that case + // the modal is treated as none modal navigation. if (this.currentOutlet) { this.currentOutlet.showingModal = true; this._modalNavigationDepth++; @@ -400,15 +405,18 @@ export class NSLocationStrategy extends LocationStrategy { routerLog("NSLocationStrategy.closeModalNavigation()"); } - if (this._modalNavigationDepth > 0) { + const isShowingModal = this._modalNavigationDepth > 0; + + if (isShowingModal) { this._modalNavigationDepth--; } // currentOutlet should be the one that corresponds to the topmost() frame const topmostOutlet = this.getOutletByFrame(this.frameService.getFrame()); - this.currentOutlet = this.findOutletByModal(this._modalNavigationDepth, true) || topmostOutlet; + const outlet = this.findOutletByModal(this._modalNavigationDepth, isShowingModal) || topmostOutlet; - if (this.currentOutlet) { + if (outlet) { + this.currentOutlet = outlet; this.currentOutlet.showingModal = false; this.callPopState(this.currentOutlet.peekState(), false); } @@ -481,7 +489,8 @@ export class NSLocationStrategy extends LocationStrategy { this.callPopState(null, true, currentOutlet); } - if (!currentOutlet.isNSEmptyOutlet) { + // Skip frames filtering since currentOutlet is when no frames available. + if (currentOutlet.frames.length && !currentOutlet.isNSEmptyOutlet) { currentOutlet.frames = currentOutlet.frames.filter(currentFrame => currentFrame !== frame); return currentOutlet.frames.length; } @@ -554,26 +563,32 @@ export class NSLocationStrategy extends LocationStrategy { return pathToOutlet || lastPath; } - findOutletByModal(modalNavigation: number, isShowingModal?: boolean): Outlet { - return this.outlets.find((outlet) => { - let isEqual = outlet.modalNavigationDepth === modalNavigation; - return isShowingModal ? isEqual && outlet.showingModal : isEqual; - }); - } - findOutlet(outletKey: string, activatedRouteSnapshot?: ActivatedRouteSnapshot): Outlet { - let outlet: Outlet = this.outlets.find((currentOutlet) => currentOutlet.outletKeys.indexOf(outletKey) > -1); + let outlet: Outlet = this.outlets.find((currentOutlet) => { + let equalModalDepth = currentOutlet.modalNavigationDepth === this._modalNavigationDepth; + return equalModalDepth && currentOutlet.outletKeys.indexOf(outletKey) > -1; + }); // No Outlet with the given outletKey could happen when using nested unnamed p-r-o // primary -> primary -> prymary if (!outlet && activatedRouteSnapshot) { const pathByOutlets = this.getPathByOutlets(activatedRouteSnapshot); - outlet = this.outlets.find((currentOutlet) => currentOutlet.pathByOutlets === pathByOutlets); + outlet = this.outlets.find((currentOutlet) => { + let equalModalDepth = currentOutlet.modalNavigationDepth === this._modalNavigationDepth; + return equalModalDepth && currentOutlet.pathByOutlets === pathByOutlets; + }); } return outlet; } + private findOutletByModal(modalNavigation: number, isShowingModal?: boolean): Outlet { + return this.outlets.find((outlet) => { + let equalModalDepth = outlet.modalNavigationDepth === modalNavigation; + return isShowingModal ? equalModalDepth && outlet.showingModal : equalModalDepth; + }); + } + private getOutletByFrame(frame: Frame): Outlet { let outlet; diff --git a/nativescript-angular/router/page-router-outlet.ts b/nativescript-angular/router/page-router-outlet.ts index 381ff52bd..fdcd5e2b1 100644 --- a/nativescript-angular/router/page-router-outlet.ts +++ b/nativescript-angular/router/page-router-outlet.ts @@ -422,15 +422,8 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire private getOutlet(activatedRouteSnapshot: ActivatedRouteSnapshot): Outlet { const topActivatedRoute = findTopActivatedRouteNodeForOutlet(activatedRouteSnapshot); - const modalNavigation = this.locationStrategy._modalNavigationDepth; const outletKey = this.locationStrategy.getRouteFullPath(topActivatedRoute); - let outlet; - - if (modalNavigation > 0) { // Modal with 'primary' p-r-o - outlet = this.locationStrategy.findOutletByModal(modalNavigation); - } else { - outlet = this.locationStrategy.findOutlet(outletKey, topActivatedRoute); - } + let outlet = this.locationStrategy.findOutlet(outletKey, topActivatedRoute); // Named lazy loaded outlet. if (!outlet && this.isEmptyOutlet) {