Skip to content

fix(angular): transition animation plays when using browser back and forward buttons #28188

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.

Sign up for GitHub

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

Closed
wants to merge 12 commits into from
Closed

fix(angular): transition animation plays when using browser back and forward buttons #28188

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9b00e20
fix(angular): fix navigation stack when using browser back and forwar…
philipp-stsm Sep 16, 2023
64e0b44
fix(angular): remove legacy code for Angular < 7.2.0
philipp-stsm Sep 16, 2023
9846503
fix(angular): update comment
philipp-stsm Sep 16, 2023
b7d25ea
revert: revert changes regarding navigation stack
philipp-stsm Oct 6, 2023
887bad1
fix: apply fix for missing nav animation
philipp-stsm Oct 6, 2023
c36494b
revert: revert changes in stack controller
philipp-stsm Oct 6, 2023
bb91eb8
revert: revert changes for setting lastNavId
philipp-stsm Oct 6, 2023
e36e541
revert: revert unnecessary change
philipp-stsm Oct 11, 2023
45ae7d3
Merge remote-tracking branch 'upstream/main' into fix-navigation
liamdebeasi Oct 13, 2023
0f51294
Merge branch 'ionic-team:main' into fix-navigation
hoi4 Oct 31, 2023
b5f487c
fix: use root page transition when using root routerlink and browser …
philipp-stsm Oct 31, 2023
9f2ea5a
chore: better semantic colocation of code
philipp-stsm Oct 31, 2023
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
75 changes: 48 additions & 27 deletions packages/angular/src/directives/navigation/stack-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Location } from '@angular/common';
import { ComponentRef, NgZone } from '@angular/core';
import { ActivatedRoute, Router } from '@angular/router';
import { AnimationBuilder, RouterDirection } from '@ionic/core';
import { AnimationBuilder, NavDirection, RouterDirection } from '@ionic/core';

import { bindLifecycleEvents } from '../../providers/angular-delegate';
import { NavController } from '../../providers/nav-controller';
Expand Down Expand Up @@ -62,50 +62,71 @@ export class StackController {
}

setActive(enteringView: RouteView): Promise<StackDidChangeEvent> {
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();

let currentNavigation;
const currentNavigation = this.router.getCurrentNavigation();

const router = this.router as any;
/**
* 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 && currentNavigation?.trigger !== 'popstate') {
if (this.views.length > 0) {
this.views.splice(-1, 1);
}
}

// Angular >= 7.2.0
if (router.getCurrentNavigation) {
currentNavigation = router.getCurrentNavigation();
// 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';

// Angular < 7.2.0
} else if (router.navigations?.value) {
currentNavigation = router.navigations.value;
/**
* 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 e.g. when 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);
}
}

/**
* If the navigation action
* sets `replaceUrl: true`
* then we need to make sure
* we remove the last item
* from our views stack
* 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 (currentNavigation?.extras?.replaceUrl) {
if (this.views.length > 0) {
this.views.splice(-1, 1);
}
if (isDirectionBasedOnNavigationIds && suggestedDirectionBasedOnStackOrder) {
direction = suggestedDirectionBasedOnStackOrder;
animation = suggestedDirectionBasedOnStackOrder;
}

const tabSwitch = isTabSwitch(enteringView, leavingView);
if (tabSwitch) {
direction = 'back';
animation = undefined;
}

const reused = this.views.includes(enteringView);
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();
}

Expand Down
2 changes: 2 additions & 0 deletions packages/angular/src/directives/navigation/stack-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down Expand Up @@ -110,4 +111,5 @@ export interface RouteView {
savedExtras?: NavigationExtras;
unlistenEvents: () => void;
animationBuilder?: AnimationBuilder;
root?: boolean;
}
9 changes: 7 additions & 2 deletions packages/angular/src/providers/nav-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
});
}
Expand Down Expand Up @@ -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;
Expand All @@ -200,6 +204,7 @@ export class NavController {
direction,
animation,
animationBuilder,
isDirectionBasedOnNavigationIds,
};
}

Expand Down
6 changes: 4 additions & 2 deletions packages/angular/test/base/e2e/src/router-link.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the test change here. The fix modifies whether or not an animation is used, but testBack does not verify this.

Copy link
Contributor Author

@hoi4 hoi4 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just an issue I fixed where the test executed the back navigation, but did not call testBack like the other test cases. I assumed it was missed originally when writing the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. Is this change necessary to validate the bug you fixed? If not, could we remove it from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure :)

});

it('should go back with a[routerLink][routerDirection=back]', () => {
Expand All @@ -138,6 +139,7 @@ function testForward() {
cy.get('app-router-link-page #canGoBack').should('have.text', 'true');

cy.go('back');
cy.wait(500);
Copy link
Contributor

@liamdebeasi liamdebeasi Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the timeout changes here necessary? If not, could we revert them? (Sorry, I meant to comment on this with my last round of reviews, but it slipped my mind)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if possible, could you allow me to push changes to your branch? I can resolve the merge conflicts for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, it is necessary. Before, the back transition was instant and now it takes the time for the animation which is why we need the additional wait
I invited you to my fork. Thanks for taking care of the merge conflicts.

cy.testStack('ion-router-outlet', ['app-router-link']);
cy.testLifeCycle('app-router-link', {
ionViewWillEnter: 2,
Expand All @@ -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,
Expand All @@ -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,
Expand Down