fix(scroll): use location.key for scroll behaviours

[hentielouw]

Description

This is a simple fix to use location.key for saving scroll state, if it's available.

Based on the suggestions found in this discussion: taion/scroll-behavior#135 (comment)

Related Issues

Fixes #12390.

[KyleAMathews]

What does locationKey look like when you browse around?

Also I'm assuming you verified the bug I posted doesn't happen any longer?

[KyleAMathews]

And thanks for jumping on this!

[hentielouw]

I've tested it locally and looks like it's working as expected. I haven't added any automated tests since the package doesn't seem to have any tests at the moment - happy to add tests if you think that's needed, but might need a hand!

To illustrate how locationKey changes, I've added a console.log for locationKey and set up a test site (Gatsby Default Starter with some Lorem Ipsum). I took the screenshot below after navigating using "Go to page 2" and "Go back to the homepage" links a few times (without using "back"). I then used browser "back" to go to previous pages.

On first load the locationKey value is initial. Scrolling up and down logs the same value multiple times. On each navigation locationKey changes to a new integer value. Pressing "back" uses previous key values.

Hope that makes sense! Gets a bit muddy talking about this scrolling/navigation behaviour.

[pieh]

I've added tests for scroll behavior simulating example scenario with some changes (from taion/scroll-behavior#135 (comment) )

visit /
PUSH to /long-page
Scroll to middle-of-the-page
PUSH to /
PUSH to /long-page
Scroll to bottom-of-the-page
PUSH to /
POP to /long-page
  assert path is /long page
  assert we are scrolled to bottom-of-the-page
  assert we are NOT scrolled to middle-of-the-page
POP to /
POP to /long-page
  assert path is /long page
  assert we are scrolled to middle-of-the-page
  assert we are NOT scrolled to bottom-of-the-page

Which looks like this in cypress logs - hopefully in works in CI correctly:

[pieh]

Thanks @hentielouw!

[gatsbot]

Holy buckets, @hentielouw — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

[t2ca]

Looking for some help, this update stopped the following code from working on my project.

exports.shouldUpdateScroll = () => {
  if (window.__navigatingToLink === true) {
    return [0, 0]
  }
  return true
}

exports.onRouteUpdate = () => {
  window.__navigatingToLink = false
}

I added this code to gatsby-browser.js to fix an issue I was having, but since this update, this code no longer works for me. Would love some help, thanks!

[KyleAMathews]

What was the issue you were having and what was the above code supposed to do and how does it behave now? Can you open a new issue with answers to those questions? Thanks! That'll help us help you.

[t2ca]

Thanks Kyle for taking the time to respond, i opened a new issue #12997

[narration-sd]

Please see where the fix rolled in at 2.0.7 is breaking importantly expected 2.0.6 behavior, so that we need this regression optionable-out, thanks: #12997 (comment)