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.

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

Jump to bottom

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

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 all 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
52 changes: 29 additions & 23 deletions packages/angular/common/src/directives/navigation/stack-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,40 +65,46 @@ export class StackController {
const 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 leavingViewIndex = leavingView ? this.views.indexOf(leavingView) : -1;

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
* 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);
}
}

/**
* 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
*/
const isPopStateTransitionFromRootPage =
direction === 'back' && leavingView?.root && currentNavigation?.trigger === 'popstate';

if (isPopStateTransitionFromRootPage) {
direction = 'root';
animation = undefined;

if (leavingViewIndex >= 0) {
this.views.splice(leavingViewIndex, 1);
}
}

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

const reused = this.views.includes(enteringView);
const views = this.insertView(enteringView, direction);

Expand Down
2 changes: 2 additions & 0 deletions packages/angular/common/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;
}
4 changes: 2 additions & 2 deletions packages/angular/common/src/providers/nav-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ 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.guessDirection = this.guessAnimation = id < this.lastNavId ? 'back' : 'forward';
Copy link
Contributor

Choose a reason for hiding this comment

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

One bug I noticed is animations now play when using the back/forward buttons in the sidemenu app (clicking items in the menu don't play an animation). I'll see if there's a solution for this.

Copy link
Contributor

@liamdebeasi liamdebeasi Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Screen.Recording.2023-10-13.at.12.01.33.PM.mov

Copy link
Contributor Author

@hoi4 hoi4 Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Any news regarding this, @liamdebeasi ?
If it helps, I can also take another look at the bug :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We have work for this scheduled in our next sprint, but that doesn't start until next week. We'd love it if you could take a look!

For this particular action, the routerDirection is root, so maybe we need to consider that when determining if an animation should play? https://github.com/ionic-team/starters/blob/main/angular/official/sidemenu/src/app/app.component.html#L10C33-L10C33

Copy link
Contributor Author

@hoi4 hoi4 Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@liamdebeasi I had a look at the issue. In order to resolve it I needed some of the changes from my other open PR #28297.
I applied the fix to both branches and adapted the test as we don't need the 500ms wait in the root case anymore now. Hope that helps!

I tested the change with the routerlink test page as I didn't know how to get the starter app running with my local changes:
Screenshot 2023-10-31 at 15 38 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liamdebeasi Any chance you have some time to take a look at the fix? 😇

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not. The ticket has been pulled into our current sprint which just started, so I should be able to look at it soon.

this.lastNavId = this.guessDirection === 'forward' ? ev.id : id;
}
});
Expand Down
3 changes: 2 additions & 1 deletion packages/angular/test/base/e2e/src/lazy/router-link.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,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,
Expand Down Expand Up @@ -181,7 +182,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