Perform scrolling prior to Visit completion

Closes hotwired#400 When processing a `Visit`, invoke `Visit.performScroll()` while loading a response, loading a Snapshot, or executing a same-page anchor navigation. After in-lining those calls to more appropriate points in the Visit lifecycle, this commit removes the `performScroll()` call from the `Visit.render()` method.
seanpdoyle · Jul 16, 2022 · f542cd0 · f542cd0
1 parent 6eb2cde
commit f542cd0
Show file tree

Hide file tree

Showing 7 changed files with 94 additions and 12 deletions.
diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
@@ -110,7 +110,7 @@ export class Navigator {
       if (fetchResponse.serverError) {
         await this.view.renderError(snapshot, this.currentVisit)
       } else {
-        await this.view.renderPage(snapshot, false, true, this.currentVisit)
+        await this.view.renderPage(snapshot, this.currentVisit || null, false, true)
       }
       this.view.scrollToTop()
       this.view.clearSnapshotCache()

diff --git a/src/core/drive/page_view.ts b/src/core/drive/page_view.ts
@@ -15,13 +15,10 @@ type PageViewRenderer = PageRenderer | ErrorRenderer
 export class PageView extends View<Element, PageSnapshot, PageViewRenderer, PageViewDelegate> {
   readonly snapshotCache = new SnapshotCache(10)
   lastRenderedLocation = new URL(location.href)
-  forceReloaded = false
 
-  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true, visit?: Visit) {
+  renderPage(snapshot: PageSnapshot, visit: Visit | null, isPreview = false, willRender = true) {
     const renderer = new PageRenderer(this.snapshot, snapshot, isPreview, willRender)
-    if (!renderer.shouldRender) {
-      this.forceReloaded = true
-    } else {
+    if (renderer.shouldRender) {
       visit?.changeHistory()
     }
     return this.render(renderer)

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -228,7 +228,8 @@ 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, this)
+          await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), this, false, this.willRender)
+          this.performScroll()
           this.adapter.visitRendered(this)
           this.complete()
         } else {
@@ -270,7 +271,8 @@ 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, this)
+          await this.view.renderPage(snapshot, this, isPreview, this.willRender)
+          this.performScroll()
           this.adapter.visitRendered(this)
           if (!isPreview) {
             this.complete()
@@ -295,6 +297,7 @@ export class Visit implements FetchRequestDelegate {
     if (this.isSamePage) {
       this.render(async () => {
         this.cacheSnapshot()
+        this.performScroll()
         this.adapter.visitRendered(this)
       })
     }
@@ -429,9 +432,6 @@ export class Visit implements FetchRequestDelegate {
     })
     await callback()
     delete this.frame
-    if (!this.view.forceReloaded) {
-      this.performScroll()
-    }
   }
 
   cancelRender() {

diff --git a/src/tests/fixtures/scroll/one.html b/src/tests/fixtures/scroll/one.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Scroll: One</title>
+    <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+    <script src="/src/tests/fixtures/test.js"></script>
+    <style>
+      html { scroll-behavior: smooth; }
+      .push-below-fold { margin-top: 100vh; }
+    </style>
+  </head>
+  <body>
+    <h1>Scroll: One</h1>
+    <hr class="push-below-fold">
+    <a id="one-below-fold" href="/src/tests/fixtures/scroll/two.html#two-below-fold">two.html#two-below-fold</a>
+    <hr class="push-below-fold">
+  </body>
+</html>
diff --git a/src/tests/fixtures/scroll/two.html b/src/tests/fixtures/scroll/two.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Scroll: Two</title>
+    <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+    <script src="/src/tests/fixtures/test.js"></script>
+    <style>
+      html { scroll-behavior: smooth; }
+      .push-below-fold { margin-top: 100vh; }
+    </style>
+  </head>
+  <body>
+    <h1>Scroll: Two</h1>
+    <hr class="push-below-fold">
+    <a id="two-below-fold" href="/src/tests/fixtures/scroll/one.html#one-below-fold">one.html#one-below-fold</a>
+    <hr class="push-below-fold">
+  </body>
+</html>
diff --git a/src/tests/fixtures/visit.html b/src/tests/fixtures/visit.html
@@ -5,13 +5,20 @@
     <title>Turbo</title>
     <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
     <script src="/src/tests/fixtures/test.js"></script>
+    <style>
+      .push-below-fold { margin-top: 100vh; }
+    </style>
   </head>
   <body>
     <section>
       <h1>Visit</h1>
       <p><a id="same-origin-link" href="/src/tests/fixtures/one.html">Same-origin link</a></p>
       <p><a id="same-origin-link-search-params" href="/src/tests/fixtures/one.html?key=value">Same-origin link with ?key=value</a></p>
       <p><a id="sample-response" href="/src/tests/fixtures/one.html">Sample response</a></p>
+      <p><a id="same-page-link" href="/src/tests/fixtures/visit.html">Same page link</a></p>
+      <hr class="push-below-fold">
+      <p><a id="below-the-fold-link" href="/src/tests/fixtures/one.html">one.html</a></p>
+      <hr class="push-below-fold">
     </section>
   </body>
 </html>
diff --git a/src/tests/functional/visit_tests.ts b/src/tests/functional/visit_tests.ts
@@ -1,7 +1,16 @@
 import { Page, test } from "@playwright/test"
 import { assert } from "chai"
 import { get } from "http"
-import { nextBeat, nextEventNamed, readEventLogs, visitAction, willChangeBody } from "../helpers/page"
+import {
+  isScrolledToSelector,
+  isScrolledToTop,
+  nextBeat,
+  nextEventNamed,
+  readEventLogs,
+  scrollToSelector,
+  visitAction,
+  willChangeBody,
+} from "../helpers/page"
 
 test.beforeEach(async ({ page }) => {
   await page.goto("/src/tests/fixtures/visit.html")
@@ -145,6 +154,37 @@ function contentTypeOfURL(url: string): Promise<string | undefined> {
   })
 }
 
+test("test can scroll to element after click-initiated turbo:visit", async ({ page }) => {
+  const id = "below-the-fold-link"
+  await page.evaluate((id: string) => {
+    addEventListener("turbo:load", () => document.getElementById(id)?.scrollIntoView())
+  }, id)
+
+  assert(await isScrolledToTop(page), "starts unscrolled")
+
+  await page.click("#same-page-link")
+  await nextEventNamed(page, "turbo:load")
+  await nextBeat()
+
+  assert(await isScrolledToSelector(page, "#" + id), "scrolls after click-initiated turbo:load")
+})
+
+test("test can scroll to element after history-initiated turbo:visit", async ({ page }) => {
+  const id = "below-the-fold-link"
+  await page.evaluate((id: string) => {
+    addEventListener("turbo:load", () => document.getElementById(id)?.scrollIntoView())
+  }, id)
+
+  await scrollToSelector(page, "#" + id)
+  await page.click("#" + id)
+  await nextEventNamed(page, "turbo:load")
+  await page.goBack()
+  await nextEventNamed(page, "turbo:load")
+  await nextBeat()
+
+  assert(await isScrolledToSelector(page, "#" + id), "scrolls after history-initiated turbo:load")
+})
+
 async function visitLocation(page: Page, location: string) {
   return page.evaluate((location) => window.Turbo.visit(location), location)
 }