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.

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

bug: react swipe to go back does not implement gesture properly #22342

Closed
BerkeAras opened this issue Oct 17, 2020 · 50 comments · Fixed by #25563
Closed

bug: react swipe to go back does not implement gesture properly #22342

BerkeAras opened this issue Oct 17, 2020 · 50 comments · Fixed by #25563
Labels
help wanted a good issue for the community package: react @ionic/react package type: bug a confirmed bug report

Comments

@BerkeAras
Copy link

Bug Report

Ionic version:

[ ] 4.x
[x] 5.x

Current behavior:
iOS Swipe back not working properly on React

Expected behavior:
App should go back after swiping, not while swiping.

Steps to reproduce:
Sample App: https://azbs-app-dev.azurewebsites.net/schedule

@ionitron-bot ionitron-bot bot added the triage label Oct 17, 2020
@liamdebeasi liamdebeasi changed the title bug: iOS Swipe Back not working properly on React bug: react swipe to go back does not implement gesture properly Oct 19, 2020
@liamdebeasi liamdebeasi added package: react @ionic/react package type: bug a confirmed bug report labels Oct 19, 2020
@ionitron-bot ionitron-bot bot removed the triage label Oct 19, 2020
@liamdebeasi
Copy link
Contributor

Thanks for the issue. Looks like this is the culprit:

. When the gesture starts, Ionic React just navigates back instead of using the onMove callback.

@BerkeAras
Copy link
Author

@liamdebeasi Is there already a pull request?

@liamdebeasi
Copy link
Contributor

There is no PR yet. Are you interested in working on this?

@BerkeAras
Copy link
Author

@liamdebeasi I could try, I do not work with ts often ^^

@liamdebeasi
Copy link
Contributor

Thanks! We have a guide on contributing here: https://github.com/ionic-team/ionic-framework/blob/master/.github/CONTRIBUTING.md#creating-a-pull-request.

This fix needs to be implemented in the onStart and onEnd handlers for routerOutlet:

The easiest thing may be to adapt the solution we have for Ionic Vue: https://github.com/ionic-team/ionic-framework/blob/master/packages/vue/src/components/IonRouterOutlet.ts#L143

@liamdebeasi liamdebeasi added the help wanted a good issue for the community label Oct 23, 2020
@ionitron-bot
Copy link

ionitron-bot bot commented Oct 23, 2020

This issue has been labeled as help wanted. This label is added to issues that we believe would be good for contributors.

If you'd like to work on this issue, please comment here letting us know that you would like to submit a pull request for it. This helps us to keep track of the pull request and make sure there isn't duplicated effort.

For a guide on how to create a pull request and test this project locally to see your changes, see our contributing documentation.

Thank you!

@BerkeAras
Copy link
Author

Thanks! We have a guide on contributing here: https://github.com/ionic-team/ionic-framework/blob/master/.github/CONTRIBUTING.md#creating-a-pull-request.

This fix needs to be implemented in the onStart and onEnd handlers for routerOutlet:

The easiest thing may be to adapt the solution we have for Ionic Vue: https://github.com/ionic-team/ionic-framework/blob/master/packages/vue/src/components/IonRouterOutlet.ts#L143

Thank you @liamdebeasi.

I will start contributing as soon as possible :)

@BerkeAras
Copy link
Author

@liamdebeasi I could not find anything about this in the contributing guideline: How can I preview my changes for react? Running npm start just runs a preview for ionic core.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 6, 2020

Good question. We have a test Ionic React application that you can use to preview your changes or write Cypress tests against: https://github.com/ionic-team/ionic-framework/tree/master/packages/react/test/test-app.

To copy over your changes, you should first run npm run build in the packages/react and packages/react-router directory. Then, in the packages/react/test-app directory, run npm run sync to copy over the built changes. After that, you can use npm run start to start a live reload server for the test app.

edit: use packages/react-router/test-app instead of packages/react/test-app

@ghost
Copy link

ghost commented Jan 12, 2021

Hello @BerkeAras I kindly wanted to ask about your progress. Are you already working on a PR or is this topic still open? Thanks already and best regards.

@BerkeAras
Copy link
Author

Hello @BerkeAras I kindly wanted to ask about your progress. Are you already working on a PR or is this topic still open? Thanks already and best regards.

Hey @patrickap-bkklinde :)

Unfortunately I have to work very much in the last few months, I could not work on a PR :( I will work on it as soon as possible 👍

@Cyral
Copy link

Cyral commented May 26, 2021

FYI I noticed that the ion-nav component already has the native transition: https://ionicframework.com/docs/api/nav, so developing the transition is not needed, it just needs to be ported to the router component

@milanharia
Copy link

Hello @BerkeAras I kindly wanted to ask about your progress. Are you already working on a PR or is this topic still open? Thanks already and best regards.

Hey @patrickap-bkklinde :)

Unfortunately I have to work very much in the last few months, I could not work on a PR :( I will work on it as soon as possible 👍

Hi @BerkeAras have you had the chance to look into this yet?

@BerkeAras
Copy link
Author

I have still difficulties using Gestures to swipe back and to cancel going back @milanharia.
I need help 😂

@milanharia
Copy link

@BerkeAras What have you done so far? Have you tried to edit the onStart and onEnd handlers as mentioned above?

@fr3fou
Copy link

fr3fou commented Oct 22, 2021

will this be fixed for ionic 6?

@masonicboom
Copy link
Contributor

masonicboom commented Jun 30, 2022

Thanks for the issue. Can everyone try the following dev build and let me know if it resolves the issue?

npm install @ionic/[email protected] @ionic/[email protected]

This dev build contains a combination of work from myself and @masonicboom.

Thanks @liamdebeasi. I tried the command you gave but get No matching version found for @ionic/[email protected]. Is the build published to NPM? It looks like there's an @ionic/react-router with that version but not an @ionic/react.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jun 30, 2022

Ah right, I forgot that NPM is having some issues: https://status.npmjs.org/incidents/6wr25yb0b2dd. I'll publish another dev build once the issue is resolved.

@liamdebeasi
Copy link
Contributor

Try 6.1.13-dev.11656627909.1efa1aeb. (Also updated in my comment above)

@rgoldiez
Copy link

@liamdebeasi swipe back gesture seems to be working with the build you shared. However, I noticed that page animations (when navigating forward, as an example, on iOS) don't seem to work with this build.

@milanharia
Copy link

@liamdebeasi I have also noticed what @rgoldiez mentioned. I have also managed to break the navigation stack multiple times when navigating back and forth between pages (the URL will update but the page will not change), and some pages will just flash when trying to swipe back.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jul 1, 2022

Can you provide sample applications? I can't reproduce these behaviors on my end. I can reproduce the page animation issue. I cannot reproduce the navigation stack change issue.

@liamdebeasi
Copy link
Contributor

Thanks for testing! Here's a dev build with the page animation fix. I also update the comment in #22342 (comment).

6.1.13-dev.11656681741.1973f14d

@rgoldiez
Copy link

rgoldiez commented Jul 1, 2022

Thanks for testing! Here's a dev build with the page animation fix. I also update the comment in #22342 (comment).

6.1.13-dev.11656681741.1973f14d

@liamdebeasi - this build fixes the animation issue.

One question on swipe back functionality - should it take you to back to the same page that the back button would? The scenario I have is that a nest page could be reloaded onto that nested page (ie, no pages in the stack). The back button has a default route such that pressing the back button takes the user back to where they would expect to go. However, in this scenario (without any pages in the stack) the swipe gesture opens the side menu. Is this expected?

@liamdebeasi
Copy link
Contributor

I am assuming you are talking about ion-back-button and not the browser back button, though please correct me if my assumption is incorrect.

In your scenario, yes that is to be expected. The reason for this is the component for the default route has not been created and therefore does not exist in the DOM/any navigation stack. The swipe to go back behavior is designed to work when there is an existing page to go back to in the navigation stack.

@rgoldiez
Copy link

rgoldiez commented Jul 1, 2022

@liamdebeasi - yes, your assumption is correct. And thanks for clarifying the behavior. I wasn't sure if it was suppose to get created (like pressing the ion-back-button) when the touch for the gesture starts.

@rgoldiez
Copy link

rgoldiez commented Jul 1, 2022

@liamdebeasi - yes, your assumption is correct. And thanks for clarifying the behavior. I wasn't sure if it was suppose to get created (like pressing the ion-back-button) when the touch for the gesture starts.

To clarify my comment, ion-back-button seems to have to create the (default back route) page when it does exist in the stack.

@milanharia
Copy link

@liamdebeasi I have noticed when you go from a parent page (/tab1) to a nested page (/nested1) and then to another nested page (/nested2), then swipe back twice to go back to the parent page, then go back to the first nested page, you are unable to swipe back to the parent. This is then resolved when you tap the ion-back-button. The journey would be /tab1 > /nested1 > /nested2 > /nested1 > /tab1 > /nested1 > /tab1. Here is a video to make it clearer:

Screen.Recording.2022-07-01.at.20.28.29.mov

Find the repo here to reproduce.

@liamdebeasi
Copy link
Contributor

Thanks! This dev build should fix that issue:

6.1.13-dev.11656722810.12f349fc

@milanharia
Copy link

@liamdebeasi I have also noticed that if you swipe back quickly across the entire screen, it causes the animation to trigger twice. I have also noticed in some instances the entire IonPage flashing when you swipe back to the page.

Double animation

Screen.Recording.2022-07-04.at.09.41.39.mov

Flashing IonPage

Screen.Recording.2022-07-04.at.09.46.55.mov

@liamdebeasi
Copy link
Contributor

Thanks, I was able to fix both of those in my PR. Here is another dev build if you are interested in testing:

6.1.13-dev.11657055102.1ed275da

@rgoldiez
Copy link

rgoldiez commented Jul 5, 2022

@liamdebeasi - this version works great! I had experienced one of the two issues that @milanharia had reported but no issues any more. Thanks for everyone's work on this!

@milanharia
Copy link

@liamdebeasi I am experiencing really strange issues if you attempt to swipe back from the root level tab page. In the video below I swiped back from /tab1 and then it seemed to affect the navigational stack as you can see the flashes of the tab1 page when I routed to /tab2 and /tab3. I was then unable to access the nested pages in /tab1 (ie. /tab1/nested1).

RPReplay_Final1657209759.MP4

@liamdebeasi
Copy link
Contributor

Do you have a sample app I can test this in? I am unable to reproduce this locally.

@milanharia
Copy link

Please find the repo here.

@liamdebeasi
Copy link
Contributor

Thanks! This dev build should fix the issue:

6.1.13-dev.11657232231.14fdde4d

liamdebeasi added a commit that referenced this issue Jul 19, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. This has been resolved via #25563, and a fix will be available in an upcoming release of Ionic Framework.

Thank you to everyone who helped test!

@ionitron-bot
Copy link

ionitron-bot bot commented Aug 18, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted a good issue for the community package: react @ionic/react package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants