diff --git a/packages/vue-router/src/locationHistory.ts b/packages/vue-router/src/locationHistory.ts index 879994eb02..ab838751e1 100644 --- a/packages/vue-router/src/locationHistory.ts +++ b/packages/vue-router/src/locationHistory.ts @@ -85,11 +85,25 @@ export const createLocationHistory = () => { locationHistory.push(routeInfo); } - const clearHistory = () => { - locationHistory.length = 0; + /** + * Wipes the location history arrays. + * You can optionally provide a routeInfo + * object which will wipe that entry + * and every entry that appears after it. + */ + const clearHistory = (routeInfo?: RouteInfo) => { Object.keys(tabsHistory).forEach(key => { tabsHistory[key] = []; }); + + if (routeInfo) { + const existingRouteIndex = locationHistory.findIndex(r => r.position === routeInfo.position); + if (existingRouteIndex === -1) return; + + locationHistory.splice(existingRouteIndex); + } else { + locationHistory.length = 0; + } } const getTabsHistory = (tab: string): RouteInfo[] => { let history; @@ -105,17 +119,6 @@ export const createLocationHistory = () => { const size = () => locationHistory.length; - const updateByHistoryPosition = (routeInfo: RouteInfo, updateEntries: boolean) => { - const existingRouteIndex = locationHistory.findIndex(r => r.position === routeInfo.position); - if (existingRouteIndex === -1) return; - - locationHistory[existingRouteIndex].pathname = routeInfo.pathname; - - if (updateEntries) { - locationHistory[existingRouteIndex].pushedByRoute = routeInfo.pushedByRoute; - } - } - /** * Finds and returns the location history item * given the state of browser's history API. @@ -205,7 +208,6 @@ export const createLocationHistory = () => { return { current, - updateByHistoryPosition, size, last, previous, @@ -214,6 +216,7 @@ export const createLocationHistory = () => { update, getFirstRouteInfoForTab, getCurrentRouteInfoForTab, - findLastLocation + findLastLocation, + clearHistory } } diff --git a/packages/vue-router/src/router.ts b/packages/vue-router/src/router.ts index 079f2861a8..f70ee3a3f9 100644 --- a/packages/vue-router/src/router.ts +++ b/packages/vue-router/src/router.ts @@ -60,7 +60,7 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => * new pages or updating history via the forward * and back browser buttons. */ - const initialHistoryPosition = opts.history.state.position as number; + let initialHistoryPosition = opts.history.state.position as number; let currentHistoryPosition = opts.history.state.position as number; let currentRouteInfo: RouteInfo; @@ -159,8 +159,60 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => leavingLocationInfo = locationHistory.previous(); } else if (incomingRouteParams.routerAction === 'pop') { leavingLocationInfo = locationHistory.current(initialHistoryPosition, currentHistoryPosition + 1); + + /** + * If the Ionic Router action was "pop" + * and the browser history action was "replace", then + * it is the case that the user clicked an IonBackButton + * that is trying to go back to the route specified + * by the defaultHref property. + * + * The problem is that this route currently does + * not exist in the browser history, and we cannot + * prepend an item in the browser's history stack. + * To work around this, we replace the state of + * the current item instead. + * Given this scenario: + * /page2 --> /page3 --> (back) /page2 --> (defaultHref) /page1 + * We would replace the state of /page2 with the state of /page1. + * + * When doing this, we are essentially re-writing past + * history which makes the future history no longer relevant. + * As a result, we clear out the location history so that users + * can begin pushing new routes to the stack. + * + * This pattern is aligned with how the browser handles + * pushing new routes after going back as well as how + * other stack based operations such as undo/redo work. + * For example, if you do tasks A, B, C, undo B and C, and + * then do task D, you cannot "redo" B and C because you + * rewrote the stack's past history. + * + * With browser history, it is a similar concept. + * Going /page1 --> /page2 --> /page3 and then doing + * router.go(-2) will bring you back to /page1. + * If you then push /page4, you have rewritten + * the past history and you can no longer go + * forward to /page2 or /page3. + */ + if (action === 'replace') { + locationHistory.clearHistory(); + } } else { - leavingLocationInfo = locationHistory.current(initialHistoryPosition, currentHistoryPosition - 1); + /** + * If the routerDirection was specified as "root", then + * we are replacing the initial state of location history + * with this incoming route. As a result, the leaving + * history info is stored at the same location as + * where the incoming history location will be stored. + * + * Otherwise, we can assume this is just another route + * that will be pushed onto the end of location history, + * so we can grab the previous item in history relative + * to where the history state currently is. + */ + const position = (incomingRouteParams.routerDirection === 'root') ? currentHistoryPosition : currentHistoryPosition - 1; + leavingLocationInfo = locationHistory.current(initialHistoryPosition, position); } } else { leavingLocationInfo = currentRouteInfo; @@ -285,26 +337,40 @@ export const createIonRouter = (opts: IonicVueRouterOptions, router: Router) => */ if (historySize > historyDiff && routeInfo.tab === undefined) { /** - * When going from /a --> /a/1 --> /b, then going - * back to /a, then going /a --> /a/2 --> /b, clicking - * the ion-back-button should return us to /a/2, not /a/1. - * However, since the route entry for /b already exists, - * we need to update other information such as the "pushedByRoute" - * so we know which route pushed this new route. + * When navigating back through the history, + * if users then push a new route the future + * history stack is no longer relevant. As + * a result, we need to clear out all entries + * that appear after the current routeInfo + * so that we can then append the new history. * - * However, when using router.go with a stride of >1 or <-1, - * we should not update this additional information because - * we are traversing through the history, not pushing new states. - * Going from /a --> /b --> /c, then doing router.go(-2), then doing - * router.go(2) to go from /a --> /c should not update the route - * listing to say that /c was pushed by /a. + * This does not apply when using router.go + * as that is traversing through the history, + * not altering it. + * + * Previously we had only updated the existing route + * and then left the future history alone. That + * worked for some use cases but was not sufficient + * in other scenarios. */ - const hasDeltaStride = delta !== undefined && Math.abs(delta) !== 1; - locationHistory.updateByHistoryPosition(routeInfo, !hasDeltaStride); + + if (routeInfo.routerAction === 'push' && delta === undefined) { + locationHistory.clearHistory(routeInfo); + locationHistory.add(routeInfo); + } } else { locationHistory.add(routeInfo); } + /** + * If we recently reset the location history + * then we also need to update the initial + * history position. + */ + if (locationHistory.size() === 1) { + initialHistoryPosition = routeInfo.position; + } + currentRouteInfo = routeInfo; } incomingRouteParams = undefined; diff --git a/packages/vue/test-app/src/App.vue b/packages/vue/test-app/src/App.vue index 0f088ec216..173993a7d5 100644 --- a/packages/vue/test-app/src/App.vue +++ b/packages/vue/test-app/src/App.vue @@ -5,7 +5,7 @@ diff --git a/packages/vue/test-app/tests/e2e/specs/routing.js b/packages/vue/test-app/tests/e2e/specs/routing.js index 2e7a3dee51..896a189f80 100644 --- a/packages/vue/test-app/tests/e2e/specs/routing.js +++ b/packages/vue/test-app/tests/e2e/specs/routing.js @@ -400,6 +400,119 @@ describe('Routing', () => { cy.ionPageVisible('home'); cy.ionPageHidden('inputs'); }) + + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/23873 + it('should correctly show pages after going back to defaultHref page', () => { + cy.visit('http://localhost:8080/default-href'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('defaulthref'); + + cy.ionBackClick('routing'); + cy.ionPageDoesNotExist('routing'); + cy.ionPageVisible('defaulthref'); + + cy.ionBackClick('defaulthref'); + cy.ionPageDoesNotExist('defaulthref'); + cy.ionPageVisible('home'); + + cy.routerPush('/default-href'); + cy.ionPageVisible('defaulthref'); + cy.ionPageHidden('home'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('defaulthref'); + + cy.ionBackClick('routing'); + cy.ionPageDoesNotExist('routing'); + cy.ionPageVisible('defaulthref'); + + cy.ionBackClick('defaulthref'); + cy.ionPageDoesNotExist('defaulthref'); + cy.ionPageVisible('home'); + }) + + it('should correctly update location history after rewriting past state', () => { + cy.visit('http://localhost:8080'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('home'); + + cy.routerPush('/inputs'); + cy.ionPageVisible('inputs'); + cy.ionPageHidden('routing'); + + cy.ionBackClick('inputs'); + cy.ionPageVisible('routing'); + cy.ionPageDoesNotExist('inputs'); + + cy.ionBackClick('routing'); + cy.ionPageVisible('home'); + cy.ionPageDoesNotExist('routing'); + + cy.routerPush('default-href'); + cy.ionPageVisible('defaulthref'); + cy.ionPageHidden('home'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('defaulthref'); + + cy.routerPush('/inputs'); + cy.ionPageVisible('inputs'); + cy.ionPageHidden('routing'); + + cy.ionBackClick('inputs'); + cy.ionPageVisible('routing'); + cy.ionPageDoesNotExist('inputs'); + + cy.ionBackClick('routing'); + cy.ionPageVisible('defaulthref'); + cy.ionPageDoesNotExist('routing'); + }) + + it('should correctly update location history after setting root state', () => { + cy.visit('http://localhost:8080'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('home'); + + cy.routerPush('/routing/1'); + cy.ionPageVisible('routingparameter-1'); + cy.ionPageHidden('routing'); + + cy.ionBackClick('routingparameter-1'); + cy.ionPageVisible('routing'); + cy.ionPageDoesNotExist('routingparameter-1'); + + cy.ionBackClick('routing'); + cy.ionPageVisible('home'); + cy.ionPageDoesNotExist('routing'); + + cy.ionRouterNavigate('/inputs', 'root') + cy.ionPageVisible('inputs'); + cy.ionPageDoesNotExist('home'); + + cy.routerPush('/routing'); + cy.ionPageVisible('routing'); + cy.ionPageHidden('inputs'); + + cy.routerPush('/routing/1'); + cy.ionPageVisible('routingparameter-1'); + cy.ionPageHidden('routing'); + + cy.ionBackClick('routingparameter-1'); + cy.ionPageVisible('routing'); + cy.ionPageDoesNotExist('routingparameter-1'); + + cy.ionBackClick('routing'); + cy.ionPageVisible('inputs'); + cy.ionPageDoesNotExist('routing'); + }) }); describe('Routing - Swipe to Go Back', () => { diff --git a/packages/vue/test-app/tests/e2e/support/commands.js b/packages/vue/test-app/tests/e2e/support/commands.js index 03b0a511a6..5592908950 100644 --- a/packages/vue/test-app/tests/e2e/support/commands.js +++ b/packages/vue/test-app/tests/e2e/support/commands.js @@ -82,6 +82,12 @@ Cypress.Commands.add('routerGo', (n) => { }); }); +Cypress.Commands.add('ionRouterNavigate', (...args) => { + cy.window().then(win => { + win.debugIonRouter.navigate(...args); + }); +}); + Cypress.Commands.add('ionBackButtonHidden', (pageId) => { cy.get(`div.ion-page[data-pageid=${pageId}]`) .should('be.visible', true) diff --git a/packages/vue/test-app/tests/unit/routing.spec.ts b/packages/vue/test-app/tests/unit/routing.spec.ts index d793cd1bae..6d169580f6 100644 --- a/packages/vue/test-app/tests/unit/routing.spec.ts +++ b/packages/vue/test-app/tests/unit/routing.spec.ts @@ -312,6 +312,124 @@ describe('Routing', () => { expect(cmpAgain[0].props()).toEqual({ title: 'xyz' }); }); + /** + * TODO(FW-653) Either Ionic Vue or Vue Test Utils + * is leaking routing information between tests. Moving these tests + * to the end cause them to fail. Need to figure out what the root cause is. + */ + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/24109 + it('canGoBack() should return the correct value', async () => { + const Page = { + components: { IonPage }, + template: `` + } + const Page2 = { + components: { IonPage }, + template: `` + } + const AppWithInject = { + components: { IonApp, IonRouterOutlet }, + template: '', + setup() { + const ionRouter = useIonRouter(); + return { ionRouter } + } + } + + const router = createRouter({ + history: createWebHistory(process.env.BASE_URL), + routes: [ + { path: '/', component: Page } + { path: '/page2', component: Page2 } + ] + }); + + router.push('/'); + await router.isReady(); + const wrapper = mount(AppWithInject, { + global: { + plugins: [router, IonicVue] + } + }); + + const ionRouter = wrapper.vm.ionRouter; + expect(ionRouter.canGoBack()).toEqual(false); + + router.push('/page2'); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(true); + + router.back(); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(false); + }); + + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/24109 + it('canGoBack() should return the correct value when using router.go', async () => { + const Page = { + components: { IonPage }, + template: `` + } + const Page2 = { + components: { IonPage }, + template: `` + } + const Page3 = { + components: { IonPage }, + template: `` + } + const AppWithInject = { + components: { IonApp, IonRouterOutlet }, + template: '', + setup() { + const ionRouter = useIonRouter(); + return { ionRouter } + } + } + + const router = createRouter({ + history: createWebHistory(process.env.BASE_URL), + routes: [ + { path: '/', component: Page } + { path: '/page2', component: Page2 }, + { path: '/page3', component: Page3 }, + ] + }); + + router.push('/'); + await router.isReady(); + const wrapper = mount(AppWithInject, { + global: { + plugins: [router, IonicVue] + } + }); + + const ionRouter = wrapper.vm.ionRouter; + expect(ionRouter.canGoBack()).toEqual(false); + + router.push('/page2'); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(true); + + router.push('/page3'); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(true); + + router.go(-2); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(false); + + router.go(2); + await waitForRouter(); + + expect(ionRouter.canGoBack()).toEqual(true); + }); + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/23043 it('should call the props function again when params change', async () => { const Page1 = { @@ -549,117 +667,4 @@ describe('Routing', () => { expect(wrapper.findComponent(Page2).exists()).toBe(false); expect(wrapper.findComponent(Page3).exists()).toBe(false); }); - - // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/24109 - it('canGoBack() should return the correct value', async () => { - const Page = { - components: { IonPage }, - template: `` - } - const Page2 = { - components: { IonPage }, - template: `` - } - const AppWithInject = { - components: { IonApp, IonRouterOutlet }, - template: '', - setup() { - const ionRouter = useIonRouter(); - return { ionRouter } - } - } - - const router = createRouter({ - history: createWebHistory(process.env.BASE_URL), - routes: [ - { path: '/', component: Page } - { path: '/page2', component: Page2 } - ] - }); - - router.push('/'); - await router.isReady(); - const wrapper = mount(AppWithInject, { - global: { - plugins: [router, IonicVue] - } - }); - - const ionRouter = wrapper.vm.ionRouter; - expect(ionRouter.canGoBack()).toEqual(false); - - router.push('/page2'); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(true); - - router.back(); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(false); - }); - - // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/24109 - it('canGoBack() should return the correct value when using router.go', async () => { - const Page = { - components: { IonPage }, - template: `` - } - const Page2 = { - components: { IonPage }, - template: `` - } - const Page3 = { - components: { IonPage }, - template: `` - } - const AppWithInject = { - components: { IonApp, IonRouterOutlet }, - template: '', - setup() { - const ionRouter = useIonRouter(); - return { ionRouter } - } - } - - const router = createRouter({ - history: createWebHistory(process.env.BASE_URL), - routes: [ - { path: '/', component: Page } - { path: '/page2', component: Page2 }, - { path: '/page3', component: Page3 }, - ] - }); - - router.push('/'); - await router.isReady(); - const wrapper = mount(AppWithInject, { - global: { - plugins: [router, IonicVue] - } - }); - - const ionRouter = wrapper.vm.ionRouter; - expect(ionRouter.canGoBack()).toEqual(false); - - router.push('/page2'); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(true); - - router.push('/page3'); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(true); - - router.go(-2); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(false); - - router.go(2); - await waitForRouter(); - - expect(ionRouter.canGoBack()).toEqual(true); - }); });