Prevously-saved scroll position is restored on a fresh page with same URL

[catamphetamine]

I found a small bug in scroll position restoration when using found router (which uses farce and scroll-behavior under the hood).

Scroll position is saved in web browser's "session storage" as:

"@@scroll|/x": [0, 123] 

where /x is the URL of the page.

Notice how the key is gonna be the same for different pages that have the same URL.

How could that happen?

For example, a user opens URL /a, then scrolls down, then focuses the web browser's address bar and enters URL /b, waits for it to load, then focuses the web browser's address bar and enters URL /a.

The result is the previously-saved scroll position being restored while it shouldn't have been because it's a completely different page, just with the same URL.

This is a result of the "session storage" being shared among all of the navigation history entries, and the INIT-ial location always doesn't have an autogenerated .key which results in such a weird bug.

My proposal would be to generate a location.key for the INIT-ial location, same way it's already done for any subsequent location.

[taion]

That's a good catch. The issue here is that the initial location doesn't get supplemented with a randomly-generated key, as we didn't want to trigger an immediate navigation action on page load.

Not quite sure how to deal with this one.

That said, this isn't specific to Found. The same thing will happen on e.g. most sites that use anything like this logic for determining page key, including Gatsby sites. You can see this behavior on e.g. https://reactjs.org.

cc @KyleAMathews @jquense – older versions of React Router actually would immediately redirect to a version of the page with a random key set on load, but I think users tended to not like that behavior much. Not sure what the best way to deal with it is. I don't think there's a way to deal with it inside this library, though, but it is a weird bug.

[catamphetamine]

@taion Yeah, I recall that in react-router@3 every location had its own random-generated key and I didn't like the idea of having it.

For the current situation, I was thinking about clearing all @@scroll| keys from Session storage on page load:

window.onload = function onPageLoad() {
  forEach((key) => {
    if (key.indexOf('@@scroll|') === 0) {
      sessionStorage.removeItem(key)
    }
  }
}

function forEach(func) {
  let i = 0
  while (i < sessionStorage.length) {
    const key = sessionStorage.key(i)
    func(key)
    i++
  }
}

[taion]

That's not quite the same, though. To emulate native browser scroll behavior, you do want to preserve scroll position if someone reloads the page.

[catamphetamine]

@taion Aaah, I see.

I wonder what's the point of preserving scroll position in browsers when a user reloads a page though. What's the rationale, the motivation. Whatever.

[taion]

And to be clear, we do still attach keys on navigation to locations after the first. But we don't do this for the first location because it would require triggering a history action to get it stamped on.

[hedgepigdaniel]

Whats wrong with storing a random key on each location entry?

That makes perfect sense to me - a key to identify each one so that even if the URL is the same, there is something to distinguish 2 separate location entries that should not share the same scroll position.

Hard to see how this library could work properly if the locations provided to it are not unique.

[taion]

Farce adds unique keys to all locations that it generates.

However, for the initial page load, I'm not aware of a way to update the location state. Old versions of history used to deal with this by immediately calling replaceState, but that led to weird bugs.

[hedgepigdaniel]

What kind of bugs did it cause?

[taion]

I honestly can't remember any more. It was enough that they took it out in history v4, though.

[catamphetamine]

I honestly can't remember any more. It was enough that they took it out in history v4, though.

I did a quck search in the history repo's issues and found only one instance of such bug which happened in IE 11: remix-run/history#220

That small bug was enough for one of the repo authors to discard the history.replaceState() workaround on page load.

The author specifically added it as a feature previously in remix-run/history#9 but after the IE 11 bug report he reconsidered and denied any similar feature requests: remix-run/history#502

My personal opinion: since I've already forked scroll-behavior to apply some minor unrelated adjustments, I might as well reinstate the history.replaceState() workaround on page load. IE 11 isn't widely used nowadays anyway. And document.referer being empty doesn't break anything besides Google Analytics which is a very insignificant inconvenience that can totally be neglected.

If anyone's interested, the workaround will be available in navigation-stack package:
https://www.npmjs.com/package/navigation-stack