Perform scrolling prior to Visit completion (#476)

* Perform scrolling prior to Visit completion Closes #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. * Fix flaky Form Submission Test Resolves a [flaky test][] by replacing a call to `nextBeat()` with a helper that will wait until the target element (in this case, `turbo-frame#frame`) dispatches a particular event (in this case, `turbo:frame-load`). [flaky test]: https://github.com/hotwired/turbo/runs/7570036350?check_suite_focus=true#step:12:16
hotwired · Jul 29, 2022 · 4450843 · 4450843
1 parent 18d088b
commit 4450843
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 6 deletions.
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
@@ -259,6 +259,7 @@ export class Visit implements FetchRequestDelegate {
         if (this.view.renderPromise) await this.view.renderPromise
         if (isSuccessful(statusCode) && responseHTML != null) {
           await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
+          this.performScroll()
           this.adapter.visitRendered(this)
           this.complete()
         } else {
@@ -301,6 +302,7 @@ export class Visit implements FetchRequestDelegate {
         } else {
           if (this.view.renderPromise) await this.view.renderPromise
           await this.view.renderPage(snapshot, isPreview, this.willRender, this)
+          this.performScroll()
           this.adapter.visitRendered(this)
           if (!isPreview) {
             this.complete()
@@ -325,6 +327,7 @@ export class Visit implements FetchRequestDelegate {
     if (this.isSamePage) {
       this.render(async () => {
         this.cacheSnapshot()
+        this.performScroll()
         this.adapter.visitRendered(this)
       })
     }
@@ -379,7 +382,7 @@ export class Visit implements FetchRequestDelegate {
   // Scrolling
 
   performScroll() {
-    if (!this.scrolled) {
+    if (!this.scrolled && !this.view.forceReloaded) {
       if (this.action == "restore") {
         this.scrollToRestoredPosition() || this.scrollToAnchor() || this.view.scrollToTop()
       } else {
@@ -459,9 +462,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/form_submission_tests.ts b/src/tests/functional/form_submission_tests.ts
@@ -592,7 +592,7 @@ test("test frame form submission with redirect response", async ({ page }) => {
 
   const button = await page.locator("#frame form.redirect input[type=submit]")
   await button.click()
-  await nextBeat()
+  await nextEventOnTarget(page, "frame", "turbo:frame-load")
 
   const message = await page.locator("#frame div.message")
   assert.notOk(await hasSelector(page, "#frame form.redirect"))

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")
@@ -196,6 +205,35 @@ 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) => {
+    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")
+
+  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) => {
+    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")
+
+  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)
 }