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

hoi4 · 2023-09-16T16:45:00Z

Issue number: resolves #16569

This PR resolves a few issues around navigation transition animations and the navigation stack when using the browser back and forward buttons.

The respective transition animation is now applied automatically (without e.g. the workaround described here)
Sometimes when navigating forward with the browser button, pages in the middle of the current tab's stack were incorrectly removed. This caused an issue when toggling between pages via the browser back and forward button (especially visible when using custom transition animations).

I added some logic to use the current navigation stack as basis for the direction of the animation that will be applied.

Short demonstration video of the changes (using the ng16 test app):
https://github.com/ionic-team/ionic-framework/assets/23243256/40aba3ec-0c56-44f1-8b82-ba5a2903b414

Does this introduce a breaking change?

Yes
No

At least to my limited knowledge. The tests are still passing. I just had to increase the timeouts slightly to properly wait for the page transitions to finish.

…d button fix(angular): respect direction set via the NavController fix(angular): adapt tests fix(angular): fixes

hoi4 · 2023-09-19T06:27:05Z

@liamdebeasi FYI :)

hoi4 · 2023-10-04T17:04:34Z

Any chance to get feedback on these changes? 🙏🏻

liamdebeasi · 2023-10-04T20:39:52Z

Hey there! Sorry for the delay. Someone from the team will do a code review soon. Thanks for contributing!

liamdebeasi

Hey there! Thanks so much for making this PR. I spoke with the team, and we've identified a simpler solution that would address #16569.

The main issue is we explicitly disable animations when using the browser back/forward buttons:

ionic-framework/packages/angular/src/providers/nav-controller.ts

Line 42 in 3259da0

this.guessAnimation = !ev.restoredState ? this.guessDirection : undefined;

ev.restoredState is set when the user presses the back/forward buttons: https://angular.io/api/router/NavigationStart#restoredState

Something like the following should resolve the issue:

if (router) {
      router.events.subscribe((ev) => {
        if (ev instanceof NavigationStart) {
          const id = ev.restoredState ? ev.restoredState.navigationId : ev.id;
-         this.guessDirection = id < this.lastNavId ? 'back' : 'forward';
-         this.guessAnimation = !ev.restoredState ? this.guessDirection : undefined;
+         this.guessAnimation = this.guessDirection = id < this.lastNavId ? 'back' : 'forward';
           this.lastNavId = this.guessDirection === 'forward' ? ev.id : id;
        }
      });
    }

I know your PR fixes other issues, but we typically don't accept PRs that fix multiple issues at the same time (it makes PRs hard to review). We're happy to review separate PRs for the other issues you worked to resolve in this PR.

Let me know if you have questions!

hoi4 · 2023-10-06T05:45:23Z

@liamdebeasi Thank you for your feedback. I understand the issue regarding fixing multiple things simultaneously. I will limit the changes in this PR to the fix for the missing nav animation when using the browser back button.
I will furthermore create another PR with the rest of my changes.

liamdebeasi

The change itself looks good, but I have a question on the test change.

liamdebeasi · 2023-10-09T12:51:03Z

packages/angular/test/base/e2e/src/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();


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

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.

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

liamdebeasi

The fix itself looks good. Thank you very much! One question below on the test changes.

liamdebeasi · 2023-10-11T17:13:01Z

packages/angular/test/base/e2e/src/router-link.spec.ts

@@ -138,6 +138,7 @@ function testForward() {
  cy.get('app-router-link-page #canGoBack').should('have.text', 'true');

  cy.go('back');
+  cy.wait(500);


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)

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

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.

liamdebeasi · 2023-10-13T16:00:49Z

packages/angular/common/src/providers/nav-controller.ts

          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';


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.

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

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

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

@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:

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

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.

liamdebeasi

Found one bug, but I'll look into a fix

…back button

Issue number: N/A ---------   ## What is the current behavior?  Our Angular E2E tests are brittle because they rely on arbitrary `cy.wait` calls to account for asynchronous routing. This leads to flaky tests on CI and seemingly random test failures when we make adjustments to the Ionic Anguar routing integration (see: #28188) Additionally, our test execution for the navigation tests is quite slow because transitions are enabled. As a result, we need to wait hundreds of ms per test just for the transitions to finish. ## What is the new behavior?  - Updated the `testStack` command to use a new `getStack` [Cypress query](https://docs.cypress.io/api/cypress-api/custom-queries). These queries come with automatic retrying built-in. By leveraging this query in the `testStack` command, we can avoid the arbitrary waits. - Added `ionic:_testing=true` query strings to the navigation tests. This causes Ionic to disable any transitions so the tests execute faster. - Removed most of the arbitrary `cy.wait` calls. I kept the swipe to go back `cy.wait` -- I wasn't quite sure how to reduce flakiness on that one. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information

liamdebeasi · 2023-11-15T17:30:43Z

Hey there!

I spoke with the team, and we would like to proceed with the fix proposed in #28188 (review) to resolve the reported issue. I was also able to avoid the need for increased timeouts in the tests through 093c671.

We looked at the behavior I noted in #28188 (comment) and realized that this was only working by chance. Ionic Angular was never respecting routerDirection when using the browser back/forward buttons. Since there was no page transition when using those buttons, the resulting behavior happened to align with the behavior specified in routerDirection="none" for the side menu starter app.

To mitigate any potential impact that this change has, we'd like to release this fix in Ionic 8. I don't have a timeline for Ionic 8, but we are actively working on it. I opened a new PR in #28530 with this fix based off the Ionic 8 development branch. I have also given you co-author credit on the commit for this fix. There is a dev build on the PR so you can try the fix in your application. Note that this dev build is subject to any Ionic 8 Breaking Changes (there aren't many at the moment).

I am going to close this in favor of the linked PR, but let me know if you have any questions. Thank you again for contributing this fix!

hoi4 · 2023-11-15T17:48:43Z

We have a big release coming up where this change would be very valuable. But I do understand the decision. Looking forward to Ionic 8! 👍🏻

Issue number: resolves #16569 ---------   ## What is the current behavior?  Ionic Angular's routing integration disables page transitions when using the browser back/forward buttons. ## What is the new behavior?  - Transitions now play when using the back/forward buttons ## Does this introduce a breaking change? - [ ] Yes - [x] No We're not aware of any breaking changes here, though it's possible some developers were relying on this behavior. As a result, we are targeting Ionic 8 to minimize any potential negative impact this fix may have on developers.  ## Other information  Supersedes #28188 Dev build: `7.5.6-dev.11700068172.15ce9b35` Co-authored-by: hoi4 <hoi4@users.noreply.github.com>

philipp-stsm added 3 commits September 16, 2023 18:05

fix(angular): fix navigation stack when using browser back and forwar…

9b00e20

…d button fix(angular): respect direction set via the NavController fix(angular): adapt tests fix(angular): fixes

fix(angular): remove legacy code for Angular < 7.2.0

64e0b44

fix(angular): update comment

9846503

hoi4 requested review from thetaPC and sean-perkins as code owners September 16, 2023 16:45

github-actions bot added the package: angular @ionic/angular package label Sep 16, 2023

hoi4 changed the title ~~fix(angular): fix navigation stack when using browser back and forward button~~ fix(angular): fix navigation transition animation when using browser back and forward button Sep 16, 2023

liamdebeasi reviewed Oct 5, 2023

View reviewed changes

philipp-stsm added 3 commits October 6, 2023 07:39

revert: revert changes regarding navigation stack

b7d25ea

fix: apply fix for missing nav animation

887bad1

revert: revert changes in stack controller

c36494b

revert: revert changes for setting lastNavId

bb91eb8

hoi4 mentioned this pull request Oct 6, 2023

fix: fix incorrect nav animations #28297

Open

2 tasks

liamdebeasi reviewed Oct 9, 2023

View reviewed changes

revert: revert unnecessary change

e36e541

liamdebeasi approved these changes Oct 11, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-navigation

45ae7d3

liamdebeasi changed the title ~~fix(angular): fix navigation transition animation when using browser back and forward button~~ fix(angular): transition animation plays when using browser back and forward button Oct 13, 2023

liamdebeasi changed the title ~~fix(angular): transition animation plays when using browser back and forward button~~ fix(angular): transition animation plays when using browser back and forward buttons Oct 13, 2023

liamdebeasi reviewed Oct 13, 2023

View reviewed changes

liamdebeasi suggested changes Oct 13, 2023

View reviewed changes

hoi4 and others added 3 commits October 31, 2023 13:31

Merge branch 'ionic-team:main' into fix-navigation

0f51294

fix: use root page transition when using root routerlink and browser …

b5f487c

…back button

chore: better semantic colocation of code

9f2ea5a

This was referenced Nov 14, 2023

fix(angular): transition plays when using browser buttons #28530

Merged

test(angular): remove routing waits in tests #28532

Merged

liamdebeasi closed this Nov 15, 2023

hoi4 mentioned this pull request Dec 9, 2023

bug: incorrect nav animation when using browser back and forward button to navigate through multiple pages #28307

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hoi4 commented Sep 16, 2023 •

edited by liamdebeasi

hoi4 commented Sep 19, 2023

hoi4 commented Oct 4, 2023

liamdebeasi commented Oct 4, 2023

liamdebeasi left a comment

hoi4 commented Oct 6, 2023

liamdebeasi left a comment

liamdebeasi Oct 9, 2023

hoi4 Oct 9, 2023 •

edited

liamdebeasi Oct 10, 2023

hoi4 Oct 11, 2023

liamdebeasi left a comment

liamdebeasi Oct 11, 2023 •

edited

liamdebeasi Oct 11, 2023

hoi4 Oct 11, 2023

liamdebeasi Oct 13, 2023

liamdebeasi Oct 13, 2023 •

edited

hoi4 Oct 30, 2023 •

edited

liamdebeasi Oct 30, 2023

hoi4 Oct 31, 2023 •

edited

hoi4 Nov 9, 2023

liamdebeasi Nov 9, 2023

liamdebeasi left a comment

liamdebeasi commented Nov 15, 2023

hoi4 commented Nov 15, 2023

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

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

Conversation

hoi4 commented Sep 16, 2023 • edited by liamdebeasi

Does this introduce a breaking change?

hoi4 commented Sep 19, 2023

hoi4 commented Oct 4, 2023

liamdebeasi commented Oct 4, 2023

liamdebeasi left a comment

Choose a reason for hiding this comment

hoi4 commented Oct 6, 2023

liamdebeasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoi4 Oct 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi left a comment

Choose a reason for hiding this comment

liamdebeasi Oct 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi Oct 13, 2023 • edited

Choose a reason for hiding this comment

hoi4 Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoi4 Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi left a comment

Choose a reason for hiding this comment

liamdebeasi commented Nov 15, 2023

hoi4 commented Nov 15, 2023

hoi4 commented Sep 16, 2023 •

edited by liamdebeasi

hoi4 Oct 9, 2023 •

edited

liamdebeasi Oct 11, 2023 •

edited

liamdebeasi Oct 13, 2023 •

edited

hoi4 Oct 30, 2023 •

edited

hoi4 Oct 31, 2023 •

edited