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 7 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
4 changes: 2 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,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';
this.lastNavId = this.guessDirection === 'forward' ? ev.id : id;
}
});
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