Fix back button render issue

Resolves an issue where pressing back button after clicking on a new route caused previous route not to be cleared from DOM.
markcellus · May 30, 2021 · aa684c9 · aa684c9
1 parent 6ef7670
commit aa684c9
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 44 deletions.
diff --git a/src/router-component.ts b/src/router-component.ts
@@ -73,12 +73,14 @@ export class RouterComponent extends HTMLElement {
                 if (!triggerRouteChange) {
                     delete state.triggerRouteChange;
                 }
-                this.previousLocation = { ...this.location };
                 method.call(history, state, title, url);
-
-                if (triggerRouteChange) {
-                    this.showRoute(url);
+                if (!triggerRouteChange) {
+                    return;
+                }
+                if (this.previousLocation) {
+                    this.hideRoute(this.previousLocation.pathname);
                 }
+                this.showRoute(url);
             };
         }
         this.showRoute(this.getFullPathname(this.location));
@@ -154,7 +156,7 @@ export class RouterComponent extends HTMLElement {
         if (!location) return;
         const [pathname, hashString] = location.split('#');
         const routeElement = this.getRouteElementByPath(pathname);
-
+        this.previousLocation = { ...this.location };
         if (!routeElement) {
             return console.warn(
                 `Navigated to path "${pathname}" but there is no matching element with a path ` +
@@ -316,10 +318,9 @@ export class RouterComponent extends HTMLElement {
 
     private async popStateChanged() {
         const path = this.getFullPathname(this.location);
-        if (this.location.href !== this.previousLocation.href) {
-            this.hideRoute(this.previousLocation.pathname);
-        }
-        this.showRoute(path);
+        // although popstate was called we still need to trigger
+        // replaceState so all stateful operations can be performed
+        window.history.replaceState({}, document.title, path);
     }
 
     private setupElement(routeElement: Element) {

diff --git a/tests/router-component-tests.ts b/tests/router-component-tests.ts
@@ -1,5 +1,5 @@
 import sinon from 'sinon';
-import { expect, fixture, html } from '@open-wc/testing';
+import { expect, fixture, html, nextFrame } from '@open-wc/testing';
 // eslint-disable-next-line no-unused-vars
 import { extractPathParams, RouterComponent } from '../src/router-component';
 
@@ -98,40 +98,6 @@ describe('<router-component>', async () => {
         expect(document.body.querySelector('second-page')).to.be.null;
     });
 
-    it('switches to the child that has the path that matches the current location after popstate has been called', async () => {
-        await fixture(html`
-            <router-component>
-                <first-page path="/page1"></first-page>
-                <second-page path="/page2"></second-page>
-            </router-component>
-        `);
-
-        window.history.pushState({}, document.title, '/page1');
-        window.history.pushState({}, document.title, '/page2');
-        const popstate = new PopStateEvent('popstate', { state: {} });
-        window.dispatchEvent(popstate);
-        expect(document.body.querySelector('first-page')).to.be.null;
-        expect(document.body.querySelector('second-page')).to.not.be.null;
-    });
-
-    it('shows a warning when attempting to go to a route that is not handled after popstate is called', async () => {
-        const component: RouterComponent = await fixture(html`
-            <router-component>
-                <first-page path="/page1"></first-page>
-                <second-page path="/page2"></second-page>
-            </router-component>
-        `);
-
-        window.history.pushState({}, document.title, '/page1');
-        const newPath = 'nope';
-        consoleWarn.resetHistory();
-        component.show(newPath);
-        expect(consoleWarn.args[0]).to.deep.equal([
-            `Navigated to path "${newPath}" but there is no matching ` +
-                `element with a path that matches. Maybe you should implement a catch-all route with the path attribute of ".*"?`,
-        ]);
-    });
-
     it('shows the child whose path matches the catch all url', async () => {
         await fixture(html`
             <router-component>
@@ -382,6 +348,62 @@ describe('<router-component>', async () => {
         expect(disconnectedCallbackSpy.callCount).to.equal(1);
     });
 
+    describe('when popstate is triggered', () => {
+        it('switches to the child that has the path that matches the current location', async () => {
+            await fixture(html`
+                <router-component>
+                    <first-page path="/page1"></first-page>
+                    <second-page path="/page2"></second-page>
+                </router-component>
+            `);
+
+            window.history.pushState({}, document.title, '/page1');
+            window.history.pushState({}, document.title, '/page2');
+            const popstate = new PopStateEvent('popstate', { state: {} });
+            window.dispatchEvent(popstate);
+            expect(document.body.querySelector('first-page')).to.be.null;
+            expect(document.body.querySelector('second-page')).to.not.be.null;
+        });
+
+        it('shows a warning when attempting to go to a route that is not handled', async () => {
+            const component: RouterComponent = await fixture(html`
+                <router-component>
+                    <first-page path="/page1"></first-page>
+                    <second-page path="/page2"></second-page>
+                </router-component>
+            `);
+
+            window.history.pushState({}, document.title, '/page1');
+            const newPath = 'nope';
+            consoleWarn.resetHistory();
+            component.show(newPath);
+            expect(consoleWarn.args[0]).to.deep.equal([
+                `Navigated to path "${newPath}" but there is no matching ` +
+                    `element with a path that matches. Maybe you should implement a catch-all route with the path attribute of ".*"?`,
+            ]);
+        });
+
+        it('removes previous route element after clicking back button', async () => {
+            await fixture(html`
+                <router-component>
+                    <first-page path="/page1">
+                        <a href="/page2">To page 2</a>
+                    </first-page>
+                    <second-page path="/page2"></second-page>
+                </router-component>
+            `);
+
+            window.history.pushState({}, document.title, '/page1');
+            const firstPageLink: HTMLAnchorElement =
+                document.querySelector('first-page a');
+            firstPageLink.click(); // go to second
+            window.history.back();
+            await nextFrame();
+            expect(document.body.querySelector('first-page')).to.not.be.null;
+            expect(document.body.querySelector('second-page')).to.be.null;
+        });
+    });
+
     describe('when pushState or replaceState is overridden', () => {
         const origPushState = window.history.pushState;
         const origReplaceState = window.history.replaceState;