search

ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.92k stars 13.52k forks source link

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

Closed BerkeAras closed 2 years ago

BerkeAras commented 3 years ago

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

liamdebeasi commented 3 years ago

Thanks for the issue. Looks like this is the culprit: https://github.com/ionic-team/ionic-framework/blob/74af3cb50b089a6bd60d515158e03b18b86455b8/packages/react-router/src/ReactRouter/StackManager.tsx#L135. When the gesture starts, Ionic React just navigates back instead of using the onMove callback.

BerkeAras commented 3 years ago

@liamdebeasi Is there already a pull request?

liamdebeasi commented 3 years ago

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

BerkeAras commented 3 years ago

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

liamdebeasi commented 3 years ago

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: https://github.com/ionic-team/ionic-framework/blob/74af3cb50b089a6bd60d515158e03b18b86455b8/packages/react-router/src/ReactRouter/StackManager.tsx#L137

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

ionitron-bot[bot] commented 3 years ago

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 commented 3 years ago

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:

https://github.com/ionic-team/ionic-framework/blob/74af3cb50b089a6bd60d515158e03b18b86455b8/packages/react-router/src/ReactRouter/StackManager.tsx#L137

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

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

milanharia commented 2 years ago

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

fr3fou commented 2 years ago

will this be fixed for ionic 6?

BerkeAras commented 2 years ago

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

@milanharia Thanks for your response. Of course I already tried the onStart/onEnd Handlers as already mentioned above. It is not needed to develop a whole new transition because this is already being used in Ionic Core, Ionic Angular and Ionic Vue.

Because I am working 40 Hours a Week I don't have much time to focus on this issue currently.

@ all: Help is wanted 😁

BerkeAras commented 2 years ago

will this be fixed for ionic 6?

Currently there is no fix by now. I am pretty sure that this issue will be merged as soon as there is a working PR.

BerkeAras commented 2 years ago

The user @apodogov found a workaround: https://github.com/ionic-team/ionic-framework/issues/20904#issuecomment-950227078

Thanks @apodogov 🎉

I am currently working to implement this workaround as a fix on a higher level, so that it could be shipped in an upcoming release. Of course it is not the best idea to implement a workaround on core-level. But because this is a fundamental issue, it could be a fast fix for now and could be redesigned later. What do you think, @liamdebeasi?

liamdebeasi commented 2 years ago

Thanks for the feedback. We are bumping this up in priority internally. No ETA on a release date, but we will update this thread when we have more to share.

yetkinaykan commented 2 years ago

Any progress on this issue?

gschier commented 2 years ago

I noticed this too after trying out the new EAP 21 App (which is Angular) and noticing that it used the native back gesture. I assumed this was just a limitation of Ionic itself, then assumed it was something with my code, now realize it's with Ionic React. I may have some time to take a look at implementing this this week.

EDIT: Just took a quick look at it and it's definitely over my head, having no familiarity to the codebase. I'd be up for giving it another shot with some high-level guidance.

SamuelScheit commented 2 years ago

Any progress on this issue?

masonicboom commented 2 years ago

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:

https://github.com/ionic-team/ionic-framework/blob/74af3cb50b089a6bd60d515158e03b18b86455b8/packages/react-router/src/ReactRouter/StackManager.tsx#L137

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 I'd like to get this working. I've got a draft PR at https://github.com/ionic-team/ionic-framework/pull/25469 with my attempt so far. Questions:

  1. What's the correct way to identify the enteringViewItem's outletId (here)? I have a hack right now (take the current routeInfo.id - 1) which seems to break when there's more than one item in the back stack.

  2. Can you tell why the previous page is not visible during swipe back (see video attached to PR at https://github.com/ionic-team/ionic-framework/pull/25469)? It looks like the previous page is absent from the DOM. Maybe my test case is just not keeping both in the DOM simultaneously? Here I've been testing by going to http://localhost:3000/routing/tabs/home/details/1, pressing "Go to Details 2", then swiping back.

  3. If I start from http://localhost:3000/routing/tabs/home, then tap "Details 1", I've got two pages in the DOM as expected, and my swipe-back will sort-of work. But again, the previous page is not visible during the swipe. Also, it looks like there's a second transition that happens when I let go. Does this give any clues?

https://user-images.githubusercontent.com/86958/173706609-2e4da652-68a2-4129-a6ac-b6420b5d57ff.mov

Could you provide any guidance on where to go from here? I'm not familiar with Ionic's internals, so even a high-level description of the steps that need to happen would be helpful.

My rough understanding is that the router outlet's swipe handler needs to trigger a transition animation from the current screen (the leavingViewItem) to the previous item on the back stack (the enteringViewItem), and then when the swipe completes (and has gone past the threshold), I need to navigate back. But that back navigation seems to trigger its own animation. Do I need to cross-fade the leaving and entering view's opacity, or does the iosTransitionAnimation do that for me? Do I need to disable the animation that's triggered when I do this.context.goBack() or is that not the right method to use (the Vue implementation does something different)?

masonicboom commented 2 years ago

Update: almost got this working. The outletId for both the entering and leaving view items is the same. Makes sense. With the test case starting from http://localhost:3000/routing/tabs/home, I now get a nice swipe back transition. The only problem remaining is a double-transition at the end, after the navigation triggers.

https://user-images.githubusercontent.com/86958/173732877-2a738ac5-600e-4b89-a1a4-ce3da440d735.mov

masonicboom commented 2 years ago

Almost there. Now there's just a flicker (re-render) when the transition back completes.

https://user-images.githubusercontent.com/86958/173748395-65a1e557-141a-41c9-84c3-aab046bef882.mov

masonicboom commented 2 years ago

Remaining:

Got any pointers for those @liamdebeasi? Suggestions on tests (is there something I can easily copy-paste from the Vue implementation)? I can convert https://github.com/ionic-team/ionic-framework/pull/25469 to ready to review and continue discussion there if you want.

liamdebeasi commented 2 years ago

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

npm install @ionic/react@6.1.13-dev.11656722810.12f349fc @ionic/react-router@6.1.13-dev.11656722810.12f349fc

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

masonicboom commented 2 years ago

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

npm install @ionic/react@6.1.10-dev.11656617530.13c91cd8 @ionic/react-router@6.1.10-dev.11656617530.13c91cd8

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/react@6.1.10-dev.11656617530.13c91cd8. Is the build published to NPM? It looks like there's an @ionic/react-router with that version but not an @ionic/react.

liamdebeasi commented 2 years ago

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 commented 2 years ago

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

rgoldiez commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

~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 commented 2 years ago

Thanks for testing! Here's a dev build with the page animation fix. I also update the comment in https://github.com/ionic-team/ionic-framework/issues/22342#issuecomment-1171602754.

6.1.13-dev.11656681741.1973f14d
rgoldiez commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

https://user-images.githubusercontent.com/76167627/176958192-0068584a-9ed4-4926-88f8-789691a36088.mov

Find the repo here to reproduce.

liamdebeasi commented 2 years ago

Thanks! This dev build should fix that issue:

6.1.13-dev.11656722810.12f349fc
milanharia commented 2 years ago

@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

https://user-images.githubusercontent.com/76167627/177118083-5ef510f3-54c5-4d88-8807-66fc10cb44c1.mov

Flashing IonPage

https://user-images.githubusercontent.com/76167627/177118447-1c56c357-f4c1-43d0-b9a2-296a31854f91.mov

liamdebeasi commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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).

https://user-images.githubusercontent.com/76167627/177820522-e9420b23-2553-4877-821f-1f2233d16c9a.MP4

liamdebeasi commented 2 years ago

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

milanharia commented 2 years ago

Please find the repo here.

liamdebeasi commented 2 years ago

Thanks! This dev build should fix the issue:

6.1.13-dev.11657232231.14fdde4d
liamdebeasi commented 2 years ago

Thanks for the issue. This has been resolved via https://github.com/ionic-team/ionic-framework/pull/25563, and a fix will be available in an upcoming release of Ionic Framework.

Thank you to everyone who helped test!

ionitron-bot[bot] commented 2 years ago

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.