Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(location-strategy): crash on going back with router-outlet after closing modal #1748

Merged
merged 3 commits into from
Feb 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/single-page/app/app.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.title {
font-size: 30;
font-size: 15;
margin: 16;
}

Expand Down
5 changes: 5 additions & 0 deletions e2e/single-page/app/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
4 changes: 4 additions & 0 deletions e2e/single-page/app/second/modal/modal.component.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<StackLayout>
<Label text="Welcome to modal"></Label>
<Button text="Close Modal" (tap)="close()"></Button>
</StackLayout>
19 changes: 19 additions & 0 deletions e2e/single-page/app/second/modal/modal.component.ts
Original file line number Diff line number Diff line change
@@ -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();
}

}
22 changes: 20 additions & 2 deletions e2e/single-page/app/second/second.component.ts
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -19,12 +21,16 @@ import { RouterExtensions } from "nativescript-angular/router";

<StackLayout>
<Label [text]="'Second Component: ' + (id$ | async)" class="title"></Label>
<Button text="Show Modal" (tap)="onShowModal()"></Button>
<Button text="Back" (tap)="back()"></Button>
</StackLayout>`
})
export class SecondComponent implements OnInit, OnDestroy {
public id$: Observable<number>;
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"]));
}

Expand All @@ -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) => {
});
}
}
32 changes: 24 additions & 8 deletions e2e/single-page/e2e/tests.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
4 changes: 1 addition & 3 deletions nativescript-angular/directives/dialogs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down
2 changes: 1 addition & 1 deletion nativescript-angular/package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
49 changes: 32 additions & 17 deletions nativescript-angular/router/ns-location-strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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++;
Expand All @@ -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);
}
Expand Down Expand Up @@ -481,7 +489,8 @@ export class NSLocationStrategy extends LocationStrategy {
this.callPopState(null, true, currentOutlet);
}

if (!currentOutlet.isNSEmptyOutlet) {
// Skip frames filtering since currentOutlet is <router-outlet> when no frames available.
if (currentOutlet.frames.length && !currentOutlet.isNSEmptyOutlet) {
currentOutlet.frames = currentOutlet.frames.filter(currentFrame => currentFrame !== frame);
return currentOutlet.frames.length;
}
Expand Down Expand Up @@ -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;

Expand Down
9 changes: 1 addition & 8 deletions nativescript-angular/router/page-router-outlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down