Only update history when Turbo visit is renderable (#601)

* use location.replace instead of location.reload

* update when we change the history

* get location from visitStart

* always pass the visit to renderError/Page

* rollback name update

* revert visit_control view hack

* await nextBeat for the URL to be ready

* remove console log

Author: manuelpuyol

src/core/drive/navigator.ts

@@ -108,9 +108,9 @@ export class Navigator {
     if (responseHTML) {
       const snapshot = PageSnapshot.fromHTMLString(responseHTML)
       if (fetchResponse.serverError) {
-        await this.view.renderError(snapshot)
+        await this.view.renderError(snapshot, this.currentVisit)
       } else {
-        await this.view.renderPage(snapshot)
+        await this.view.renderPage(snapshot, false, true, this.currentVisit)
       }
       this.view.scrollToTop()
       this.view.clearSnapshotCache()

src/core/drive/page_view.ts

@@ -4,6 +4,7 @@ import { ErrorRenderer } from "./error_renderer"
 import { PageRenderer } from "./page_renderer"
 import { PageSnapshot } from "./page_snapshot"
 import { SnapshotCache } from "./snapshot_cache"
+import { Visit } from "./visit"
 
 export interface PageViewDelegate extends ViewDelegate<PageSnapshot> {
   viewWillCacheSnapshot(): void
@@ -16,15 +17,18 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
   lastRenderedLocation = new URL(location.href)
   forceReloaded = false
 
-  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true) {
+  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true, visit?: Visit) {
     const renderer = new PageRenderer(this.snapshot, snapshot, isPreview, willRender)
     if (!renderer.shouldRender) {
       this.forceReloaded = true
+    } else {
+      visit?.changeHistory()
     }
     return this.render(renderer)
   }
 
-  renderError(snapshot: PageSnapshot) {
+  renderError(snapshot: PageSnapshot, visit?: Visit) {
+    visit?.changeHistory()
     const renderer = new ErrorRenderer(this.snapshot, snapshot, false)
     return this.render(renderer)
   }

src/core/drive/visit.ts

@@ -225,11 +225,11 @@ export class Visit implements FetchRequestDelegate {
         this.cacheSnapshot()
         if (this.view.renderPromise) await this.view.renderPromise
         if (isSuccessful(statusCode) && responseHTML != null) {
-          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender)
+          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
           this.adapter.visitRendered(this)
           this.complete()
         } else {
-          await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML))
+          await this.view.renderError(PageSnapshot.fromHTMLString(responseHTML), this)
           this.adapter.visitRendered(this)
           this.fail()
         }
@@ -267,7 +267,7 @@ export class Visit implements FetchRequestDelegate {
           this.adapter.visitRendered(this)
         } else {
           if (this.view.renderPromise) await this.view.renderPromise
-          await this.view.renderPage(snapshot, isPreview, this.willRender)
+          await this.view.renderPage(snapshot, isPreview, this.willRender, this)
           this.adapter.visitRendered(this)
           if (!isPreview) {
             this.complete()

src/core/native/browser_adapter.ts

@@ -17,6 +17,7 @@ export class BrowserAdapter implements Adapter {
 
   visitProgressBarTimeout?: number
   formProgressBarTimeout?: number
+  location?: URL
 
   constructor(session: Session) {
     this.session = session
@@ -27,9 +28,9 @@ export class BrowserAdapter implements Adapter {
   }
 
   visitStarted(visit: Visit) {
+    this.location = visit.location
     visit.loadCachedSnapshot()
     visit.issueRequest()
-    visit.changeHistory()
     visit.goToSamePageAnchor()
   }
 
@@ -121,7 +122,10 @@ export class BrowserAdapter implements Adapter {
 
   reload(reason: ReloadReason) {
     dispatch("turbo:reload", { detail: reason })
-    window.location.reload()
+
+    if (!this.location) return
+
+    window.location.href = this.location.toString()
   }
 
   get navigator() {

src/tests/fixtures/rendering.html

@@ -78,6 +78,7 @@ <h1>Rendering</h1>
     <button id="permanent-video-button" type="button">Play</button>
 
     <hr class="push-off-screen">
+    <p><a id="below-the-fold-visit-control-reload-link-middle" href="/src/tests/fixtures/visit_control_reload.html#middle">Visit control: reload - middle</a></p>
     <p><a id="below-the-fold-visit-control-reload-link" href="/src/tests/fixtures/visit_control_reload.html">Visit control: reload</a></p>
   </body>
 </html>

src/tests/fixtures/visit_control_reload.html

@@ -6,8 +6,17 @@
     <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
     <script src="/src/tests/fixtures/test.js"></script>
     <meta name="turbo-visit-control" content="reload">
-  </head>
-  <body>
-    <h1>Visit control: reload</h1>
+    <style>
+      .push-off-screen { margin-top: 1000px; }
+    </style>
+   </head>
+   <body>
+     <h1>Visit control: reload</h1>
+
+    <hr class="push-off-screen">
+
+    <h2 id="middle">Middle the page</h2>
+    <hr class="push-off-screen">
+    <h2>Down the page</h2>
   </body>
 </html>

src/tests/functional/visit_tests.ts

@@ -10,6 +10,8 @@ export class VisitTests extends TurboDriveTestCase {
     const urlBeforeVisit = await this.location
     await this.visitLocation("/src/tests/fixtures/one.html")
 
+    await this.nextBeat
+
     const urlAfterVisit = await this.location
     this.assert.notEqual(urlBeforeVisit, urlAfterVisit)
     this.assert.equal(await this.visitAction, "advance")