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: ionic vue, ionic react incorrect view rendered when using router.go(n) and n < -1 #22563

Closed
NunoSav opened this issue Nov 25, 2020 · 32 comments · Fixed by #23773
Closed
Labels
package: react @ionic/react package package: vue @ionic/vue package type: bug a confirmed bug report
Milestone

Comments

@NunoSav
Copy link

NunoSav commented Nov 25, 2020

Bug Report

Ionic version:
[ ] 4.x
[x] 5.x

Current behavior:
Trying to go back 2 or more routes.

Expected behavior:
App navigates back X amount of routes as long as there's that many in the history stack

Steps to reproduce:
Related code:
https://github.com/NunoSav/ionic-vue3-router-go-back

Other information:
I also made a vue3+router sample that mirrors the one above but without Ionic in it to see if the issue was with vue-router itself, it wasn't. You can check it here https://github.com/NunoSav/vue3-router-go-back

Ionic info:

Ionic:

   Ionic CLI       : 6.12.2 (/Users/nur/.config/yarn/global/node_modules/@ionic/cli)
   Ionic Framework : @ionic/vue 5.5.0

Capacitor:

   Capacitor CLI   : 2.4.3
   @capacitor/core : 2.4.3

Utility:

   cordova-res (update available: 0.15.2) : 0.15.1
   native-run (update available: 1.2.2)   : 1.2.1

System:

   NodeJS : v12.16.3 (/usr/local/bin/node)
   npm    : 6.14.4
   OS     : macOS Big Sur
@ionitron-bot ionitron-bot bot added the triage label Nov 25, 2020
@liamdebeasi
Copy link
Member

liamdebeasi commented Nov 25, 2020

Thanks, I can reproduce this. When you do router.go(n) it looks like we ignore the n value and assume 1 page navigation.

@liamdebeasi liamdebeasi added package: vue @ionic/vue package type: bug a confirmed bug report labels Nov 25, 2020
@liamdebeasi liamdebeasi changed the title bug: Ionic-vue-router fails to go back more than one route bug: ionic vue router.go(n) does not work when n < -1 Nov 25, 2020
@liamdebeasi liamdebeasi added this to the 5.5.3 milestone Nov 25, 2020
@NunoSav
Copy link
Author

NunoSav commented Nov 25, 2020

Any chance to get a build with this fix and #22540 ? 😛

@liamdebeasi
Copy link
Member

#22540 was already fixed and should be shipping out soon. I've assigned this one to a milestone so you can track our progress on the milestones page: https://github.com/ionic-team/ionic-framework/milestones. The milestone assignment is subject to change, but it should give you a rough idea of how close fixes are to shipping.

@liamdebeasi liamdebeasi changed the title bug: ionic vue router.go(n) does not work when n < -1 bug: ionic vue, ionic react incorrect view rendered when using router.go(n) and n < -1 Dec 11, 2020
@liamdebeasi liamdebeasi added the package: react @ionic/react package label Dec 11, 2020
@liamdebeasi
Copy link
Member

liamdebeasi commented Dec 11, 2020

Took a closer look at this. Turns out this is the same issue as #22400 in Ionic React, which makes sense since Ionic Vue and Ionic React share similar routing integration code.

The main problem is we do not know how many pages a user has gone back, which makes fixing this tricky. It looks like this is not a problem in Ionic Angular, so I will probably look there for some hints on how to solve this.

edit: Removing this from the 5.5.3 milestone as it is unlikely I will be able to solve, test, and merge this by next week.

@liamdebeasi liamdebeasi removed this from the 5.5.3 milestone Dec 11, 2020
@lijianping1989
Copy link

I encountered the same problem, which was introduced in ionic/react 5.3.0, it still exists in ionic/react 5.5.2. You may be able to refer to version 5.2.3 to solve the problem.
But the previous version has the history.replace() issue.
Hope the go(n) issue can be resolved soon

@ldikmans
Copy link

ldikmans commented Feb 9, 2021

Any suggestions for a workaround? The only thing we have found so far is a window.location.reload. Which is far from acceptable for the user experience of the app.

@rluttikhuizen
Copy link

Is there an update on this issue and/or new milestone planned for the fix?

Many thanks,

Ronald

@carlgik4
Copy link

With a deep diving into the source code, I didn't find an easy way to solve this issue :(

@liamdebeasi
Copy link
Member

Unfortunately, I don't think the routers give us enough information to solve this bug. The main thing we would need is information on how many pages the user went back (I.e. the n in go(n)), so we can update our stack accordingly. Without that information, I do not see of a good way to fix this.

@liamdebeasi
Copy link
Member

liamdebeasi commented Jun 24, 2021

Also this used to work in the older version of Ionic React mainly by chance as it was pretty aggressive with pruning its internal view stack. This behavior caused some other unintended issues which is why the behavior changed starting in Ionic React 5.3.0.

@mattsteve
Copy link

mattsteve commented Jun 30, 2021

This is a game breaker for us (Vue). Another reproduction: https://github.com/mattsteve/ionic-vue-router-go-broken

By the way, I can't even chain router.go(-1) in a loop. It still goes back only 1 page. This is probably a similar, but related issue. Router should remember all the commands that were ordered and not ignore them because a transition animation is playing.

@mattsteve
Copy link

By the way, @modus/ionic-vue doesn't have this bug. Maybe worth studying their code.

https://github.com/ModusCreateOrg/ionic-vue

@liamdebeasi
Copy link
Member

@modus/ionic-vue relies on the <router-view> component provided by Vue Router which is why the bug does not exist there. The tradeoff is @modus/ionic-vue cannot render multiple views in the DOM at the same time, which can lead to performance issues when using swipe to go back on lower end devices.

@mattsteve
Copy link

Well, it was working good for our purposes. Would rather have a small performance hit on low-end devices than have a major bug like this.

@rluttikhuizen
Copy link

Well, it was working good for our purposes. Would rather have a small performance hit on low-end devices than have a major bug like this.

+1

@ldikmans
Copy link

ldikmans commented Jul 2, 2021

Our navigation really breaks for the user, especially on native apps. We are forced to do a history.replace, which gives really unexpected behavior for users when they use the native back button, it brings them back to

Home page
Page 1
Page 2
Page 3

After doing some stuff on Page 3, there is nothing to do anymore for them on Page2, so we want to send them back to Page 1. If they would press back from there, they would go back to Home (which is expected and logical in our navigational structure)
since history.go(-2) does not work, we do history.replace(Page1). However if they press back now, they go back to Page 2. Which confuses the user. They get 'stuck' because pressing back one more time sends them back to Page1....

the alternative history.push(Page1) does not work either, pushing back will send them back to Page3. Our main menu (tabs , profile buttons, etc) is in Home, so they feel stuck...

this is really a very bad bug in terms of user experience.

@NunoSav
Copy link
Author

NunoSav commented Jul 2, 2021

While not perfect you can still workaround it by using a reactive variable to know when to trigger router.back() on a onIonViewWillEnter hook.

After doing some stuff on Page 3, there is nothing to do anymore for them on Page2, so we want to send them back to Page 1.

You can implement what i said with something like this on Page2:


import {  onIonViewWillEnter } from '@ionic/vue';
import { shouldGoBackStateVariable } from '...';

(...)
  setup() {
    onIonViewWillEnter(() => {
      if (shouldGoBackStateVariable === true) router.back();
    });

    return {
      ...
    };
  },

You would need to set shouldGoBackStateVariable = true when applicable.

@ldikmans
Copy link

ldikmans commented Jul 2, 2021

While not perfect you can still workaround it by using a reactive variable to know when to trigger router.back() on a onIonViewWillEnter hook.

After doing some stuff on Page 3, there is nothing to do anymore for them on Page2, so we want to send them back to Page 1.

You can implement what i said with something like this on Page2:


import {  onIonViewWillEnter } from '@ionic/vue';
import { shouldGoBackStateVariable } from '...';

(...)
  setup() {
    onIonViewWillEnter(() => {
      if (shouldGoBackStateVariable === true) router.back();
    });

    return {
      ...
    };
  },

You would need to set shouldGoBackStateVariable = true when applicable.

we are using react, but I guess the same applies there. It is an interesting workaround, thanks!

We have created another workaround and override the backbutton on Page 1, it always goes back to Home, so people don't get stuck. This works because of the hierarchy we have in the app, but it does not work for native back. (swipe on iOS).

@mattsteve
Copy link

@liamdebeasi Can't you use beforeEach in addition to afterEach to examine the change to window.history.length to detect this scenario?

@mattsteve
Copy link

Uh, actually window.history.state has position information, I see length doesn't change but the position can be used.

image

@liamdebeasi
Copy link
Member

I'll take a look and see if that is something we can use. Thanks!

@liamdebeasi
Copy link
Member

We should be able to use position to track the deltas here. I started looking into implementation options and there are a lot of unknowns still in terms of what the expected behavior is, but we have a path forward at least. Thanks for the suggestion!

Nothing to announce in terms of ETA right now as I need to do more research on this feature, but when I have something I will be sure to provide a dev build so you can all test it out and provide feedback.

@liamdebeasi liamdebeasi added this to the 5.6.13 milestone Jul 21, 2021
@liamdebeasi liamdebeasi modified the milestones: 5.6.13, 5.6.14 Aug 2, 2021
@liamdebeasi
Copy link
Member

Hi everyone,

Quick update on things: Turns out Vue Router has an undocumented delta value! Along with window.history.state.position, I was able to use this to implement router.go support in Ionic Vue and have prepared a dev build. It would be really helpful if people could test and provide feedback:

npm install @ionic/vue@5.7.0-dev.202108091813.fd3cdaf @ionic/vue-router@5.7.0-dev.202108091813.fd3cdaf

As this is a development build of Ionic, please do not use this in production.


Note on Ionic React: According to remix-run/history#36 (comment) and remix-run/history#334 (comment), it does not seem like React Router exposes a delta value. The window.history.state.position noted in #22563 (comment) is provided by Vue Router, and it does not look like React Router has something similar. One option is we could build this position tracking into Ionic React ourselves, but I am a bit hesitant to start altering window.history.state especially when React Router already manages the state for us.

@mattsteve
Copy link

mattsteve commented Aug 10, 2021

Routing backwards multiple pages looks to work, though there is some behavior I'm not expecting, if I do this:

router.push({ name: 'page1' });
// Wait for router animation...
// User types in some inputs...
router.push({ name: 'page2' });
// Wait for router animation...
router.go(-2);
// Wait for router animation...
router.push({ name: 'page1' });
// The text that user typed in page1 is still there, so the page wasn't destroyed

It seems page1 is kept in memory and was never unmounted & remounted. I expect every router.push to be accompanied by a new mount() of that page's component.

There's also another problem; I'm trying to figure out which command triggers it.

@mattsteve
Copy link

mattsteve commented Aug 10, 2021

Yeah, I don't know, router.replace() doesn't seem to be working at all? The URL changes but the page does not.

Edit: Ok, it's not that router.replace() never works, there seems to be a series of events where I can get it out of sync.

Edit: My issue seems to be that my page with <ion-tabs> needs an <ion-page> wrapped around it. This was working before... I guess your updated code doesn't handle that in the sort order when switching pages. Reproduction if you think it's something to fix: https://github.com/mattsteve/ionic-vue-router-ion-tabs-no-page

Specifically adding an <ion-page> here resolved the issue I'm talking about in this comment. It was trying to return to another page in the stack but wasn't showing it on top.
https://github.com/mattsteve/ionic-vue-router-ion-tabs-no-page/blob/master/src/views/main.vue#L2

@liamdebeasi
Copy link
Member

Thanks for testing! Here is a new dev build which should fix the mount issue in #22563 (comment):

npm install @ionic/vue@5.7.0-dev.202108102149.e699dfc @ionic/vue-router@5.7.0-dev.202108102149.e699dfc

I will investigate the replace behavior in #22563 (comment) this week.

@mattsteve
Copy link

Thanks for testing! Here is a new dev build which should fix the mount issue in #22563 (comment):

npm install @ionic/vue@5.7.0-dev.202108102149.e699dfc @ionic/vue-router@5.7.0-dev.202108102149.e699dfc

Looks good

@liamdebeasi
Copy link
Member

Regarding the router.replace issue, your tabs component should have an <ion-page> wrapping it. This is what we do in our starters too: https://github.com/ionic-team/starters/blob/main/vue/official/tabs/src/views/Tabs.vue

I can reproduce the issue in #22563 (comment) on Ionic Vue 5.6.13, so it does not seem to be related to the dev build.

I will keep this issue open for a few more days to give others more time to test. If all goes well, we should be able to get this into an upcoming release of v5.

@mattsteve
Copy link

Regarding the router.replace issue, your tabs component should have an <ion-page> wrapping it. This is what we do in our starters too: https://github.com/ionic-team/starters/blob/main/vue/official/tabs/src/views/Tabs.vue

Yeah, that's what I said initially. I was just saying it used to work without it, I suppose it automatically added the ".ion-page" class to the <ion-tabs>. Was just questioning if you intentionally removed that part.

@liamdebeasi
Copy link
Member

I have not changed anything regarding tabs. I need to clean up the code for this patch, so I will keep my eye out for anything that may have caused that behavior change.

@liamdebeasi
Copy link
Member

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

I have created #23775 which is a continuation of this issue for Ionic React developers. This thread is pretty Vue-specific, so I felt it would be useful to give React its own thread in case there are any differences in implementation.

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 16, 2021

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 Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: react @ionic/react package package: vue @ionic/vue package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants