Skip to content

Commit

Permalink
Fix back navigation after POST form submission (#1014)
Browse files Browse the repository at this point in the history
This commit fixes a bug where navigating back after submitting a POST
form would not show the previous page. The bug was introduced in
#949. The new cache API is async
since it needs to access CacheStorage which is async. In that PR,
getCachedSnapshot was changed to be async, but hasCachedSnapshot was not
changed. This meant that getCachedSnapshot would always return a promise,
which is truthy, and so hasCachedSnapshot which just checked that the
return value was not null would always return true.

The bug would show up when you tried to navigate back to a page after
a post form submission. The form submission would clear the cache, but the
restoration visit would think it had a cached snapshot and so would not
issue a request. This meant that the page would not be restored and nothing
would happen.

There's a new test in navigation_tests.js that reproduces this bug.

The fix is to make hasCachedSnapshot async and await it in the places
where it is used.
  • Loading branch information
afcapel committed Sep 28, 2023
1 parent 0ec99ae commit c207f5b
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 13 deletions.
16 changes: 8 additions & 8 deletions src/core/drive/visit.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,10 @@ export class Visit {
}
}

issueRequest() {
async issueRequest() {
if (this.hasPreloadedResponse()) {
this.simulateRequest()
} else if (this.shouldIssueRequest() && !this.request) {
} else if (!this.request && await this.shouldIssueRequest()) {
this.request = new FetchRequest(this, FetchMethod.get, this.location)
this.request.perform()
}
Expand Down Expand Up @@ -231,14 +231,14 @@ export class Visit {
}
}

hasCachedSnapshot() {
return this.getCachedSnapshot() != null
async hasCachedSnapshot() {
return (await this.getCachedSnapshot()) != null
}

async loadCachedSnapshot() {
const snapshot = await this.getCachedSnapshot()
if (snapshot) {
const isPreview = this.shouldIssueRequest()
const isPreview = await this.shouldIssueRequest()
this.render(async () => {
this.cacheSnapshot()
if (this.isSamePage) {
Expand Down Expand Up @@ -391,11 +391,11 @@ export class Visit {
return typeof this.response == "object"
}

shouldIssueRequest() {
async shouldIssueRequest() {
if (this.isSamePage) {
return false
} else if (this.action == "restore") {
return !this.hasCachedSnapshot()
} else if (this.action === "restore") {
return !(await this.hasCachedSnapshot())
} else {
return this.willRender
}
Expand Down
6 changes: 1 addition & 5 deletions src/core/native/browser_adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,7 @@ export class BrowserAdapter {

visitRequestStarted(visit) {
this.progressBar.setValue(0)
if (visit.hasCachedSnapshot() || visit.action != "restore") {
this.showVisitProgressBarAfterDelay()
} else {
this.showProgressBar()
}
this.showVisitProgressBarAfterDelay()
}

visitRequestCompleted(visit) {
Expand Down
6 changes: 6 additions & 0 deletions src/tests/functional/navigation_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,13 @@ test("test following a POST form clears cache", async ({ page }) => {
await page.click("#form-post-submit")
await nextBeat() // 301 redirect response
await nextBeat() // 200 response

assert.equal(await page.textContent("h1"), "One")

await page.goBack()
await nextBeat()

assert.equal(await page.textContent("h1"), "Navigation")
assert.notOk(await hasSelector(page, "some-cached-element"))
})

Expand Down

0 comments on commit c207f5b

Please sign in to comment.