From 6aaef02f31c857dc6dd04a942fad3146e57a658f Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Sat, 16 Sep 2023 18:05:03 +0200 Subject: [PATCH 1/9] fix(angular): fix navigation stack when using browser back and forward button fix(angular): respect direction set via the NavController fix(angular): adapt tests fix(angular): fixes --- .../directives/navigation/stack-controller.ts | 66 ++++++++++++++----- .../src/directives/navigation/stack-utils.ts | 2 + .../common/src/providers/nav-controller.ts | 9 ++- .../base/e2e/src/lazy/router-link.spec.ts | 6 +- 4 files changed, 63 insertions(+), 20 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 8241afa864f..6caf1dd0e3a 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -1,7 +1,7 @@ import { Location } from '@angular/common'; import { ComponentRef, NgZone } from '@angular/core'; import { ActivatedRoute, Router } from '@angular/router'; -import type { AnimationBuilder, RouterDirection } from '@ionic/core/components'; +import { AnimationBuilder, NavDirection, RouterDirection } from '@ionic/core'; import { bindLifecycleEvents } from '../../providers/angular-delegate'; import { NavController } from '../../providers/nav-controller'; @@ -62,14 +62,8 @@ export class StackController { } setActive(enteringView: RouteView): Promise { - const consumeResult = this.navCtrl.consumeTransition(); + const { isDirectionBasedOnNavigationIds, ...consumeResult } = this.navCtrl.consumeTransition(); let { direction, animation, animationBuilder } = consumeResult; - const leavingView = this.activeView; - const tabSwitch = isTabSwitch(enteringView, leavingView); - if (tabSwitch) { - direction = 'back'; - animation = undefined; - } const viewsSnapshot = this.views.slice(); @@ -87,25 +81,65 @@ export class StackController { } /** - * If the navigation action - * sets `replaceUrl: true` - * then we need to make sure - * we remove the last item - * from our views stack + * If the navigation action sets `replaceUrl: true` then we need to make sure + * we remove the last item from our views stack */ - if (currentNavigation?.extras?.replaceUrl) { + if (currentNavigation?.extras?.replaceUrl && currentNavigation?.trigger !== 'popstate') { if (this.views.length > 0) { this.views.splice(-1, 1); } } - const reused = this.views.includes(enteringView); + // determine direction based on the order of the views in the stack + const leavingView = this.activeView; + const isEnteringViewReused = this.views.includes(enteringView); + const leavingViewIndex = leavingView ? this.views.indexOf(leavingView) : -1; + const enteringViewIndex = isEnteringViewReused ? this.views.indexOf(enteringView) : this.views.length; + const suggestedDirectionBasedOnStackOrder: NavDirection | undefined = + leavingViewIndex === -1 ? undefined : enteringViewIndex < leavingViewIndex ? 'back' : 'forward'; + + /** + * The user triggered a back navigation on a page that was navigated to with root. In this case, the new page + * becomes the root and the leavingView is removed. + * + * This can happen when e.g.: + * - using the NavController's navigateBack on a root page + * - navigating to a page with navigateRoot and then using the browser back button + */ + if ( + direction === 'back' && + isDirectionBasedOnNavigationIds && + leavingView?.root && + currentNavigation?.trigger === 'popstate' + ) { + if (leavingViewIndex >= 0) { + this.views.splice(leavingViewIndex, 1); + } + } + + /** + * direction based on stack order takes precedence over direction based on navigation ids + * + * only applied if the user did not explicitly set the direction + * (e.g. via the NavController, routerLink directive etc.) + */ + if (isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder) { + direction = suggestedDirectionBasedOnStackOrder; + animation = suggestedDirectionBasedOnStackOrder; + } + + const tabSwitch = isTabSwitch(enteringView, leavingView); + if (tabSwitch) { + direction = 'back'; + animation = undefined; + } + const views = this.insertView(enteringView, direction); // Trigger change detection before transition starts // This will call ngOnInit() the first time too, just after the view // was attached to the dom, but BEFORE the transition starts - if (!reused) { + if (!isEnteringViewReused) { enteringView.ref.changeDetectorRef.detectChanges(); } diff --git a/packages/angular/common/src/directives/navigation/stack-utils.ts b/packages/angular/common/src/directives/navigation/stack-utils.ts index 41203407599..44d05e1f2b9 100644 --- a/packages/angular/common/src/directives/navigation/stack-utils.ts +++ b/packages/angular/common/src/directives/navigation/stack-utils.ts @@ -14,6 +14,7 @@ export const insertView = (views: RouteView[], view: RouteView, direction: Route const setRoot = (views: RouteView[], view: RouteView) => { views = views.filter((v) => v.stackId !== view.stackId); + view.root = true; views.push(view); return views; }; @@ -110,4 +111,5 @@ export interface RouteView { savedExtras?: NavigationExtras; unlistenEvents: () => void; animationBuilder?: AnimationBuilder; + root?: boolean; } diff --git a/packages/angular/common/src/providers/nav-controller.ts b/packages/angular/common/src/providers/nav-controller.ts index aff31b090b3..c246c4fee42 100644 --- a/packages/angular/common/src/providers/nav-controller.ts +++ b/packages/angular/common/src/providers/nav-controller.ts @@ -37,10 +37,11 @@ export class NavController { if (router) { router.events.subscribe((ev) => { if (ev instanceof NavigationStart) { + // restoredState is set if the browser back/forward button is used const id = ev.restoredState ? ev.restoredState.navigationId : ev.id; this.guessDirection = id < this.lastNavId ? 'back' : 'forward'; - this.guessAnimation = !ev.restoredState ? this.guessDirection : undefined; - this.lastNavId = this.guessDirection === 'forward' ? ev.id : id; + this.guessAnimation = this.guessDirection; + this.lastNavId = id; } }); } @@ -180,11 +181,14 @@ export class NavController { direction: RouterDirection; animation: NavDirection | undefined; animationBuilder: AnimationBuilder | undefined; + isDirectionBasedOnNavigationIds: boolean; } { let direction: RouterDirection = 'root'; let animation: NavDirection | undefined; const animationBuilder = this.animationBuilder; + const isDirectionBasedOnNavigationIds = this.direction === 'auto'; + if (this.direction === 'auto') { direction = this.guessDirection; animation = this.guessAnimation; @@ -200,6 +204,7 @@ export class NavController { direction, animation, animationBuilder, + isDirectionBasedOnNavigationIds, }; } diff --git a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts index 5ca72825f51..e7fdc62b4c7 100644 --- a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts +++ b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts @@ -113,6 +113,7 @@ describe('Router Link', () => { describe('back', () => { it('should go back with ion-button[routerLink][routerDirection=back]', () => { cy.get('#routerLink-back').click(); + testBack(); }); it('should go back with a[routerLink][routerDirection=back]', () => { @@ -138,6 +139,7 @@ function testForward() { cy.get('app-router-link-page #canGoBack').should('have.text', 'true'); cy.go('back'); + cy.wait(500); cy.testStack('ion-router-outlet', ['app-router-link']); cy.testLifeCycle('app-router-link', { ionViewWillEnter: 2, @@ -159,7 +161,7 @@ function testRoot() { cy.get('app-router-link-page #canGoBack').should('have.text', 'false'); cy.go('back'); - cy.wait(100); + cy.wait(500); cy.testStack('ion-router-outlet', ['app-router-link']); cy.testLifeCycle('app-router-link', { ionViewWillEnter: 1, @@ -181,7 +183,7 @@ function testBack() { cy.get('app-router-link-page #canGoBack').should('have.text', 'false'); cy.go('back'); - cy.wait(100); + cy.wait(500); cy.testStack('ion-router-outlet', ['app-router-link']); cy.testLifeCycle('app-router-link', { ionViewWillEnter: 1, From ff2c9f778e8fbf47ade45edcc06e1b79518901a2 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Sat, 16 Sep 2023 18:15:43 +0200 Subject: [PATCH 2/9] fix(angular): remove legacy code for Angular < 7.2.0 --- .../src/directives/navigation/stack-controller.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 6caf1dd0e3a..77b70c3209c 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -67,18 +67,7 @@ export class StackController { const viewsSnapshot = this.views.slice(); - let currentNavigation; - - const router = this.router as any; - - // Angular >= 7.2.0 - if (router.getCurrentNavigation) { - currentNavigation = router.getCurrentNavigation(); - - // Angular < 7.2.0 - } else if (router.navigations?.value) { - currentNavigation = router.navigations.value; - } + const currentNavigation = this.router.getCurrentNavigation(); /** * If the navigation action sets `replaceUrl: true` then we need to make sure From 7c23f9af9f133f80e9cc204ccc9515fb85059b52 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Sat, 16 Sep 2023 18:41:16 +0200 Subject: [PATCH 3/9] fix(angular): update comment --- .../common/src/directives/navigation/stack-controller.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 77b70c3209c..059f7c545ae 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -91,9 +91,7 @@ export class StackController { * The user triggered a back navigation on a page that was navigated to with root. In this case, the new page * becomes the root and the leavingView is removed. * - * This can happen when e.g.: - * - using the NavController's navigateBack on a root page - * - navigating to a page with navigateRoot and then using the browser back button + * This can happen e.g. when navigating to a page with navigateRoot and then using the browser back button */ if ( direction === 'back' && From c8370d959873b19ab4186ade0340fd3b4bbe32a6 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Fri, 6 Oct 2023 08:22:29 +0200 Subject: [PATCH 4/9] fix: apply changes from other branch --- packages/angular/common/src/providers/nav-controller.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/angular/common/src/providers/nav-controller.ts b/packages/angular/common/src/providers/nav-controller.ts index c246c4fee42..fe1c6817f16 100644 --- a/packages/angular/common/src/providers/nav-controller.ts +++ b/packages/angular/common/src/providers/nav-controller.ts @@ -39,9 +39,8 @@ export class NavController { if (ev instanceof NavigationStart) { // restoredState is set if the browser back/forward button is used const id = ev.restoredState ? ev.restoredState.navigationId : ev.id; - this.guessDirection = id < this.lastNavId ? 'back' : 'forward'; - this.guessAnimation = this.guessDirection; - this.lastNavId = id; + this.guessAnimation = this.guessDirection = id < this.lastNavId ? 'back' : 'forward'; + this.lastNavId = this.guessDirection === 'forward' ? ev.id : id; } }); } From 2c3a5299158a6c9c3a5d8bea8be943034a90b28e Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Tue, 31 Oct 2023 14:58:37 +0100 Subject: [PATCH 5/9] fix: apply root transition when using root routerLink and browser back button --- .../directives/navigation/stack-controller.ts | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 059f7c545ae..3155c8a9521 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -93,24 +93,29 @@ export class StackController { * * This can happen e.g. when navigating to a page with navigateRoot and then using the browser back button */ - if ( + const isPopStateTransitionFromRootPage = direction === 'back' && isDirectionBasedOnNavigationIds && leavingView?.root && - currentNavigation?.trigger === 'popstate' - ) { - if (leavingViewIndex >= 0) { - this.views.splice(leavingViewIndex, 1); - } - } + currentNavigation?.trigger === 'popstate'; /** - * direction based on stack order takes precedence over direction based on navigation ids + * whether direction based on stack order takes precedence over direction based on navigation ids * * only applied if the user did not explicitly set the direction * (e.g. via the NavController, routerLink directive etc.) */ - if (isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder) { + const shouldSuggestedDirectionBasedOnStackOrderTakePrecedence = + isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder; + + if (isPopStateTransitionFromRootPage) { + direction = 'root'; + animation = undefined; + + if (leavingViewIndex >= 0) { + this.views.splice(leavingViewIndex, 1); + } + } else if (shouldSuggestedDirectionBasedOnStackOrderTakePrecedence) { direction = suggestedDirectionBasedOnStackOrder; animation = suggestedDirectionBasedOnStackOrder; } From b07e8d21bf8d00f52111e98330b1ac04e7ca7057 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Tue, 31 Oct 2023 15:08:09 +0100 Subject: [PATCH 6/9] chore: revert unnecessary changes and tidy up --- .../common/src/directives/navigation/stack-controller.ts | 7 +++---- .../angular/test/base/e2e/src/lazy/router-link.spec.ts | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 3155c8a9521..a5b15f27a9e 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -1,7 +1,7 @@ import { Location } from '@angular/common'; import { ComponentRef, NgZone } from '@angular/core'; import { ActivatedRoute, Router } from '@angular/router'; -import { AnimationBuilder, NavDirection, RouterDirection } from '@ionic/core'; +import type { AnimationBuilder, RouterDirection } from '@ionic/core/components'; import { bindLifecycleEvents } from '../../providers/angular-delegate'; import { NavController } from '../../providers/nav-controller'; @@ -105,8 +105,7 @@ export class StackController { * only applied if the user did not explicitly set the direction * (e.g. via the NavController, routerLink directive etc.) */ - const shouldSuggestedDirectionBasedOnStackOrderTakePrecedence = - isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder; + const useDirectionBasedOnStackOrder = isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder; if (isPopStateTransitionFromRootPage) { direction = 'root'; @@ -115,7 +114,7 @@ export class StackController { if (leavingViewIndex >= 0) { this.views.splice(leavingViewIndex, 1); } - } else if (shouldSuggestedDirectionBasedOnStackOrderTakePrecedence) { + } else if (useDirectionBasedOnStackOrder) { direction = suggestedDirectionBasedOnStackOrder; animation = suggestedDirectionBasedOnStackOrder; } diff --git a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts index e7fdc62b4c7..bf105627b1a 100644 --- a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts +++ b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts @@ -161,7 +161,7 @@ function testRoot() { cy.get('app-router-link-page #canGoBack').should('have.text', 'false'); cy.go('back'); - cy.wait(500); + cy.wait(100); cy.testStack('ion-router-outlet', ['app-router-link']); cy.testLifeCycle('app-router-link', { ionViewWillEnter: 1, From 968a50306eff949c7d91dbeca7db313383b16477 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Tue, 31 Oct 2023 15:16:09 +0100 Subject: [PATCH 7/9] fix: fix import --- .../common/src/directives/navigation/stack-controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index a5b15f27a9e..9bc0afba93d 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -1,7 +1,7 @@ import { Location } from '@angular/common'; import { ComponentRef, NgZone } from '@angular/core'; import { ActivatedRoute, Router } from '@angular/router'; -import type { AnimationBuilder, RouterDirection } from '@ionic/core/components'; +import type { AnimationBuilder, NavDirection, RouterDirection } from '@ionic/core/components'; import { bindLifecycleEvents } from '../../providers/angular-delegate'; import { NavController } from '../../providers/nav-controller'; From b852c9b61c94e255b87962fd71b985a1e4a0e0bb Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Tue, 31 Oct 2023 15:39:27 +0100 Subject: [PATCH 8/9] fix: small simplification --- .../common/src/directives/navigation/stack-controller.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 9bc0afba93d..996c8b4e0bf 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -94,10 +94,7 @@ export class StackController { * This can happen e.g. when navigating to a page with navigateRoot and then using the browser back button */ const isPopStateTransitionFromRootPage = - direction === 'back' && - isDirectionBasedOnNavigationIds && - leavingView?.root && - currentNavigation?.trigger === 'popstate'; + direction === 'back' && leavingView?.root && currentNavigation?.trigger === 'popstate'; /** * whether direction based on stack order takes precedence over direction based on navigation ids From 9027d742fae154d780d25c5d1607fdf0f637b9c8 Mon Sep 17 00:00:00 2001 From: Philipp Heuer Date: Wed, 15 Nov 2023 18:53:05 +0100 Subject: [PATCH 9/9] revert: revert changes to test file --- packages/angular/test/base/e2e/src/lazy/router-link.spec.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts index 91c2ed0832d..5550d6326fc 100644 --- a/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts +++ b/packages/angular/test/base/e2e/src/lazy/router-link.spec.ts @@ -111,7 +111,6 @@ describe('Router Link', () => { describe('back', () => { it('should go back with ion-button[routerLink][routerDirection=back]', () => { cy.get('#routerLink-back').click(); - testBack(); }); it('should go back with a[routerLink][routerDirection=back]', () => { @@ -137,7 +136,6 @@ function testForward() { cy.get('app-router-link-page #canGoBack').should('have.text', 'true'); cy.go('back'); - cy.wait(500); cy.testStack('ion-router-outlet', ['app-router-link']); cy.testLifeCycle('app-router-link', { ionViewWillEnter: 2,