Fix browser BACK button when going to plugin pages We have to make sure that we only use `pushState()` of the browser History API for URLs that gr-router will actually handle. Calling `pushState()` tells the browser that both the previous and the next URL are handled by the same single page application with a popstate event handler. But if we call `pushState()` and then later `window.location.reload()` from the router and a separate page and document are loaded, then the `BACK` button will stop working. Bug: Issue 40015337 Release-Notes: Fix browser BACK button when going to plugin pages Change-Id: I3b2773a1d1080f67ef325b54f92069aced1a1907
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts index 6919982..f8a1f61 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1810,7 +1810,10 @@ if (this.loggedIn) { this.openReplyDialog(FocusTarget.ANY); } else { - this.getNavigation().setUrl(this.loginUrl); + // We are not using `this.getNavigation().setUrl()`, because the login + // page is served directly from the backend and is not part of the web + // app. + window.location.assign(this.loginUrl); } }
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts index 94241ea..3d21e07 100644 --- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts +++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
@@ -10,7 +10,12 @@ export interface NavigationService { /** * This is similar to letting the browser navigate to this URL when the user - * clicks it, or to just setting `window.location.href` directly. + * clicks it, or to just calling `window.location.assign()` directly. + * + * CAUTION: You should actually use `window.location.assign()` directly for + * URLs that are not handled by gr-router. Otherwise we will call + * `pushState()` and then `window.location.reload()` from the router, which + * will break the browser's back button. * * This adds a new entry to the browser location history. Consier using * `replaceUrl()`, if you want to avoid that. @@ -23,6 +28,11 @@ * Navigate to this URL, but replace the current URL in the history instead of * adding a new one (which is what `setUrl()` would do). * + * CAUTION: You should actually use `window.location.replace()` directly for + * URLs that are not handled by gr-router. Otherwise we will call + * `replaceState()` and then `window.location.reload()` from the router, which + * will break the browser's back button. + * * page.redirect() eventually just calls `window.history.replaceState()`. */ replaceUrl(url: string): void;
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-page.ts b/polygerrit-ui/app/elements/core/gr-router/gr-page.ts index 5e7cc3b..cb96d77 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-page.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-page.ts
@@ -53,6 +53,11 @@ path?: string; } +export const UNHANDLED_URL_PATTERNS = [ + /^\/log(in|out)(\/(.+))?$/, + /^\/plugins\/(.+)$/, +]; + const clickEvent = document.ontouchstart ? 'touchstart' : 'click'; export class Page { @@ -239,6 +244,18 @@ if (this.base && orig === path && window.location.protocol !== 'file:') { return; } + + // See issue 40015337: We have to make sure that we only use + // show()/pushState() for URLs that gr-router will actually handle. + // Calling pushState() tells the browser that both the previous and the + // next URL are handled by the same single page application with a + // popstate event handler. But if we call pushState() and then + // later `window.location.reload()` from the router and a separate page + // and document are loaded, then the BACK button will stop working. + if (UNHANDLED_URL_PATTERNS.find(pattern => pattern.test(path))) { + return; + } + e.preventDefault(); this.show(orig); };
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts index d194bf55..729a15b 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts
@@ -20,14 +20,49 @@ page.stop(); }); - test('click handler', async () => { - const spy = sinon.spy(); - page.registerRoute(/\/settings/, spy); - const link = await fixture<HTMLAnchorElement>( - html`<a href="/settings"></a>` - ); - link.click(); - assert.isTrue(spy.calledOnce); + suite('click handler', () => { + const clickListener = (e: Event) => e.preventDefault(); + let spy: sinon.SinonSpy; + let link: HTMLAnchorElement; + + setup(async () => { + spy = sinon.spy(); + link = await fixture<HTMLAnchorElement>(html`<a href="/settings"></a>`); + + document.addEventListener('click', clickListener); + }); + + teardown(() => { + document.removeEventListener('click', clickListener); + }); + + test('click handled by specific route', async () => { + page.registerRoute(/\/settings/, spy); + link.href = '/settings'; + link.click(); + assert.isTrue(spy.calledOnce); + }); + + test('click handled by default route', async () => { + page.registerRoute(/.*/, spy); + link.href = '/something'; + link.click(); + assert.isTrue(spy.called); + }); + + test('click not handled for /plugins/... links', async () => { + page.registerRoute(/.*/, spy); + link.href = '/plugins/gitiles'; + link.click(); + assert.isFalse(spy.called); + }); + + test('click not handled for /login/... links', async () => { + page.registerRoute(/.*/, spy); + link.href = '/login'; + link.click(); + assert.isFalse(spy.called); + }); }); test('register route and exit', () => {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts index a3cb0ee..6dad4cf 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -120,12 +120,6 @@ NEW_AGREEMENTS: /^\/settings\/new-agreement\/?/, REGISTER: /^\/register(\/.*)?$/, - // Pattern for login and logout URLs intended to be passed-through. May - // include a return URL. - // TODO: Maybe this pattern and its handler can just be removed, because - // passing through is what the default router would eventually do anyway. - LOG_IN_OR_OUT: /^\/log(in|out)(\/(.+))?$/, - // Pattern for a catchall route when no other pattern is matched. DEFAULT: /.*/, @@ -171,8 +165,6 @@ // Matches /admin/repos/<repos>,access. REPO_DASHBOARDS: /^\/admin\/repos\/(.+),dashboards$/, - PLUGINS: /^\/plugins\/(.+)$/, - // Matches /admin/plugins with optional filter and offset. PLUGIN_LIST: /^\/admin\/plugins\/?(?:\/q\/filter:(.*?))?(?:,(\d+))?$/, // Matches /admin/groups with optional filter and offset. @@ -460,7 +452,10 @@ */ redirectToLogin(returnUrl: string) { const basePath = getBaseUrl() || ''; - this.setUrl( + // We are not using `this.getNavigation().setUrl()`, because the login + // page is served directly from the backend and is not part of the web + // app. + window.location.assign( '/login/' + encodeURIComponent(returnUrl.substring(basePath.length)) ); } @@ -583,6 +578,8 @@ * page.show() eventually just calls `window.history.pushState()`. */ setUrl(url: string) { + // TODO: Use window.location.assign() instead of page.show(), if the URL is + // external, i.e. not handled by the router. this.page.show(url); } @@ -593,6 +590,8 @@ * this.page.redirect() eventually just calls `window.history.replaceState()`. */ replaceUrl(url: string) { + // TODO: Use window.location.replace() instead of page.redirect(), if the + // URL is external, i.e. not handled by the router. this.redirect(url); } @@ -809,10 +808,6 @@ this.handleRepoRoute(ctx) ); - this.mapRoute(RoutePattern.PLUGINS, 'handlePassThroughRoute', () => - this.handlePassThroughRoute() - ); - this.mapRoute( RoutePattern.PLUGIN_LIST, 'handlePluginListFilterRoute', @@ -921,10 +916,6 @@ this.handleRegisterRoute(ctx) ); - this.mapRoute(RoutePattern.LOG_IN_OR_OUT, 'handlePassThroughRoute', () => - this.handlePassThroughRoute() - ); - this.mapRoute( RoutePattern.IMPROPERLY_ENCODED_PLUS, 'handleImproperlyEncodedPlusRoute', @@ -1589,14 +1580,6 @@ } /** - * Handler for routes that should pass through the router and not be caught - * by the catchall _handleDefaultRoute handler. - */ - handlePassThroughRoute() { - windowLocationReload(); - } - - /** * URL may sometimes have /+/ encoded to / /. * Context: Issue 6888, Issue 7100 */ @@ -1651,10 +1634,15 @@ this.show404(); } else { // Route can be recognized by server, so we pass it to server. - this.handlePassThroughRoute(); + this.windowReload(); } } + // Allows stubbing in tests. + windowReload() { + windowLocationReload(); + } + private show404() { // Note: the app's 404 display is tightly-coupled with catching 404 // network responses, so we simulate a 404 response status to display it.
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts index fbc0338..6f4e528 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -181,7 +181,6 @@ 'handleDocumentationSearchRedirectRoute', 'handleLegacyLinenum', 'handleImproperlyEncodedPlusRoute', - 'handlePassThroughRoute', 'handleProjectDashboardRoute', 'handleLegacyProjectDashboardRoute', 'handleProjectsOldRoute', @@ -334,7 +333,7 @@ suite('route handlers', () => { let redirectStub: sinon.SinonStub; let setStateStub: sinon.SinonStub; - let handlePassThroughRoute: sinon.SinonStub; + let windowReloadStub: sinon.SinonStub; let redirectToLoginStub: sinon.SinonStub; async function checkUrlToState<T extends ViewState>( @@ -367,18 +366,12 @@ assert.equal(redirectToLoginStub.lastCall.firstArg, toUrl); } - async function checkUrlNotMatched(url: string) { - handlePassThroughRoute.reset(); - router.page.show(url); - await waitUntilCalled(handlePassThroughRoute, 'handlePassThroughRoute'); - } - setup(() => { stubRestApi('addRepoNameToCache'); redirectStub = sinon.stub(router, 'redirect'); redirectToLoginStub = sinon.stub(router, 'redirectToLogin'); setStateStub = sinon.stub(router, 'setState'); - handlePassThroughRoute = sinon.stub(router, 'handlePassThroughRoute'); + windowReloadStub = sinon.stub(router, 'windowReload'); router._testOnly_startRouter(); }); @@ -445,7 +438,7 @@ onExit!('', () => {}); // we left page; router.handleDefaultRoute(); - assert.isTrue(handlePassThroughRoute.calledOnce); + assert.isTrue(windowReloadStub.calledOnce); }); test('IMPROPERLY_ENCODED_PLUS', async () => { @@ -1039,12 +1032,6 @@ }); }); - test('LOG_IN_OR_OUT pass through', async () => { - // LOG_IN_OR_OUT: /^\/log(in|out)(\/(.+))?$/, - await checkUrlNotMatched('/login/asdf'); - await checkUrlNotMatched('/logout/asdf'); - }); - test('PLUGIN_SCREEN', async () => { // PLUGIN_SCREEN: /^\/x\/([\w-]+)\/([\w-]+)\/?/, await checkUrlToState('/x/foo/bar', {