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', {
