search

hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.74k stars 430 forks source link

Browser back & forward button loses scroll position #171

Closed mxlje closed 3 years ago

mxlje commented 3 years ago

When navigating between two pages using back/forward in the browser, the scroll position is always reset and the page is scrolled to the top.

I have an older application that is using Turbolinks where this does not happen.

Thank you!

seanpdoyle commented 3 years ago

Thank you for opening this. Does https://github.com/hotwired/turbo/pull/57 resolve this?

mxlje commented 3 years ago

Thank you for your swift reply, unfortunately that doesn’t seem to fix it. I installed this dev build to try it out, I hope that was how I should test it out. Otherwise I’m open to more testing, of course.

I can’t seem to replicate this bug on the hotwire docs site btw, which as far as I can tell also uses Turbo: https://turbo.hotwire.dev/handbook/introduction

I have created a tiny demo page that replicates the bug: https://maxlielje.co/turbo-scroll-demo/ Scroll down, click the link to the sub page, and then navigate back. This resets the scroll position to the top.

mxlje commented 3 years ago

Digging deeper, the way that the turbo docs site uses turbo is like this:

<script type="module">
  addEventListener("load", () => import("https://cdn.skypack.dev/@hotwired/turbo?min"))
</script>

Changing my demo to use that load event triggered import ~seems to fix it~ fixes it sometimes.

I will investigate if that works for my main application, too and see if I can reproduce it even more consistently.

mxlje commented 3 years ago

I have done some more testing with the load-based import and it seems that the scroll position is lost when navigating back quite quickly after navigating to a page. It is restored correctly when the visit on the target page lasts a second minimum, at least that’s what it feels like. The load-based import is an improvement over the "regular" (or bundled as I have in my main app) import as at least it sometimes works. It still doesn’t always, though, even after waiting.

On the demo page (https://maxlielje.co/turbo-scroll-demo/), when scrolling down and navigating to the sub page and immediately clicking back, which is easy to do on a trackpad for example with swipe gestures, the scroll position is lost.

When navigating to the sub page, waiting a second, and then navigating back, the scroll position is restored.

I also tested this with a turbolinks based app and there the it works correctly even when navigating back very quickly. I couldn’t get turbolinks to lose the scroll position at all.

frederikhors commented 3 years ago

Same problem here, I'm using:

<script type="module" src="https://cdn.skypack.dev/@hotwired/turbo"></script> in <head>.

frederikhors commented 3 years ago

It doesn't work for me the docs website too... Chrome 88, Windows 10. @mxlje can you confirm?

mxlje commented 3 years ago

Docs website: It does work for me most of the time on macOS and Brave, so chromium. It never works with Safari.

It feels like a timing issue / race condition to me, like it has something to do with how fast the target page is loaded completely and if that is interrupted somehow. It’s very inconsistent.

@seanpdoyle does anything come to mind that might cause this?

Edit: I also feel like it has something to do with the snapshots and caching. If I navigate back too quickly from a page and it loses the scroll position, it doesn’t work if I go to the page again, wait, and then navigate back.

vovayatsyuk commented 3 years ago

@mxlje for me your demo page (https://maxlielje.co/turbo-scroll-demo/) was working correctly (regardless of how fast I'm going back), but at some point stopped.

I've spent a few minutes trying to reproduce what's caused a failure and I found that it's a "click".

  1. Open demo page https://maxlielje.co/turbo-scroll-demo/
  2. Scroll down to the link and click it.
  3. Go back and see that everything is fine.
  4. Click the link again.
  5. Click on any part of the page (text, title, empty space).
  6. Go back. The scroll position is lost.

Scroll position will not work too when clicking on the first page too:

  1. Open demo page https://maxlielje.co/turbo-scroll-demo/
  2. Click on text or empty space.
  3. Scroll down to the link and click it.
  4. Go back and see scroll position is lost.

p.s. After the scroll position is lost the only way to fix it until the next "click" is to refresh the page.

nmondollot commented 3 years ago

@vovayatsyuk I can repro the same bug you describe on Chrome 89 but on Firefox 86 I can never repro it and on Safari 14 it happens 100% of the time, even when I don't click anywhere on the page before going to the next page (I'm on macOS Catalina 10.15.7)

🤔

JeroenJochems commented 3 years ago

There seem to be two issues in the demo above:

  1. On Safari 14.0.3 every time when I navigate back, it first goes to the right location and after a second or 2 jumps up.
  2. On Chrome 90 it breaks as described above: only after clicking on random places in the first page before clicking the link.
xhafan commented 3 years ago

I reverted my project from Turbo back to Turbolinks until this issue is fixed. I managed to utilize Turbo Frames on the project in the meantime, so had to create some temp code to load <turbo-frame tags manually.

Obversity commented 3 years ago

Just discovered this issue as well, and it'll be a big problem with the app I'm building. Only tested in Safari 14.0.2 and Chrome 89 on mac.

Managed to replicate on the turbo docs as well:

https://user-images.githubusercontent.com/7999019/116958844-06fd2200-acdb-11eb-9fa1-73607579746e.mp4

vwong commented 3 years ago

I hacked together a fix guys! I didn't make this a PR because this is truly a hack. I don't know enough of the rest of the code base and conventions. Perhaps this can be a starting point for someone to make a proper one?

diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
index 1187f55..02889a8 100644
--- a/src/core/drive/visit.ts
+++ b/src/core/drive/visit.ts
@@ -38,6 +38,7 @@ export type VisitOptions = {
   action: Action,
   historyChanged: boolean,
   referrer?: URL,
+  restorationIdentifier?: string,
   snapshotHTML?: string,
   response?: VisitResponse
 }
@@ -310,7 +311,7 @@ export class Visit implements FetchRequestDelegate {
   }

   scrollToAnchor() {
-    if (getAnchor(this.location) != null) {
+    if (getAnchor(this.location) != "") {
       this.view.scrollToAnchor(getAnchor(this.location))
       return true
     }
diff --git a/src/core/native/browser_adapter.ts b/src/core/native/browser_adapter.ts
index 44c2bee..4252359 100644
--- a/src/core/native/browser_adapter.ts
+++ b/src/core/native/browser_adapter.ts
@@ -15,7 +15,7 @@ export class BrowserAdapter implements Adapter {
   }

   visitProposedToLocation(location: URL, options?: Partial<VisitOptions>) {
-    this.navigator.startVisit(location, uuid(), options)
+    this.navigator.startVisit(location, options?.restorationIdentifier || uuid(), options)
   }

   visitStarted(visit: Visit) {
diff --git a/src/core/session.ts b/src/core/session.ts
index 92d8ccb..97189bb 100644
--- a/src/core/session.ts
+++ b/src/core/session.ts
@@ -105,9 +105,9 @@ export class Session implements FormSubmitObserverDelegate, HistoryDelegate, Lin

   // History delegate

-  historyPoppedToLocationWithRestorationIdentifier(location: URL) {
+  historyPoppedToLocationWithRestorationIdentifier(location: URL, restorationIdentifier: string) {
     if (this.enabled) {
-      this.navigator.proposeVisit(location, { action: "restore", historyChanged: true })
+      this.navigator.proposeVisit(location, { action: "restore", historyChanged: true, restorationIdentifier })
     } else {
       this.adapter.pageInvalidated()
     }
tleish commented 3 years ago

In debugging this, I can confirm that when restoring a page the URL is valid, but the restorationIdentifier is new every time. It appears some of the code used to behave this way, but was removed.

For example, see the following refactoring in Session

However,History still passes restorationIdentifier to historyPoppedToLocationWithRestorationIdentifier() even though the parameter has since been removed.

see: https://github.com/hotwired/turbo/blob/main/src/core/drive/history.ts#L96

Oddly enough, there are Scroll Restoration Tests which I'm guessing still pass.

tleish commented 3 years ago

Ah, looks like some of this was already discovered by the team in https://github.com/hotwired/turbo/pull/57, but the PR was just closed a few days ago.

tleish commented 3 years ago

As a temporary fix you could add something like the following:

class ScrollPosition {
  save() {
    document.body.dataset.scrollPosition = JSON.stringify([window.scrollX, window.scrollY]);
  }

  restore() {
    const scrollPosition = JSON.parse(document.body.dataset.scrollPosition || '[]');
    if (scrollPosition.length > 0) {
      window.scrollTo(...scrollPosition);
    }
    delete document.body.dataset.scrollPosition;
  }
}

const scrollPosition = new ScrollPosition();

document.addEventListener('turbo:before-cache', function (_event) {
  scrollPosition.save();
});

document.addEventListener('turbo:load', function (_event) {
  scrollPosition.restore();
});
vwong commented 3 years ago

Ah could've saved me some time debugging, if I knew about #57 - it's very similar to my hack! Wonder why it was closed without merging.

Nice solution @tleish

JeroenJochems commented 3 years ago

It's fixed in beta 8! 🥳

mxlje commented 3 years ago

Sorry for my late reply here and thanks everyone for helping narrow it down and then of course fix it. It indeed does look like the problem is fixed in beta 8 🎉