Browser API shouldUpdateScroll logic not being respected on pop state route transitions

[blimpmason]

Description

This is follow up issue to #27349 — that issue has been closed but I'm experiencing a second issue preventing me from orchestrating smooth page transitions. The issue of getSavedScrollPosition returning a single value rather than x, y coordinate array has been fixed and merged (#27384). Console.logging the saved scroll position now returns the expected [x, y] coordinate array, which is great.

Now I'm experiencing an issue my original demos didn't show: when navigating back using the browser back button from one page to the previous (when both pages have scrollable body content), the shouldUpdateScroll setTimeout seems to be ignored and scroll restoration applied immediately, disrupting the smooth effect of the page transition. This functionality worked correctly in previous versions of Gatsby (including v 2.19.43 as shown in my working comparison demo below).

// gatsby-browser.js

export const shouldUpdateScroll = ({
  routerProps: { location },
  getSavedScrollPosition,
}) => {
  if (location.action === "PUSH") {
    window.setTimeout(() => window.scrollTo(0, 0), 600)
  } else {
    const savedPosition = getSavedScrollPosition(location)
    window.setTimeout(
      () => window.scrollTo(...(savedPosition || [0, 0])),
      600
    )
  }
  return false
}

Steps to reproduce

Demonstration of the issue:
https://fix-test--gatsby-scroll-restoration-issue.netlify.app/.

You can see the issue when you scroll to the bottom of page 1, click the link to page 2 and then hit the browser back button. You will see an immediate scroll position jump that disrupts the animated fade transition, but then page 1 is restored at the correct scroll position. It seems the exiting page is immediately scrolled to the saved scroll position of the page that has not yet entered.

Demo repo:
https://github.com/blimpmason/gatsby-scroll-restoration-issue/tree/fix-test

Expected result

Original working comparison repo has been updated with the same page content as fix test branch above (uses Gatsby v2.19.43):
https://gatsby-scroll-restoration-issue-comparison.netlify.app/

This shows that the scroll restoration does not occur until the setTimeout has elapsed per the shouldUpdateScroll logic in gatsby-browser.js.

Working comparison demo repo:
https://github.com/blimpmason/gatsby-scroll-restoration-issue-comparison

Actual result

The restored scroll position is immediately applied to the exiting page before the setTimeout has elapsed, causing a janky page transition.

It's worth noting that the setTimeout is respected on Push state transitions.

Environment

  System:
    OS: macOS 10.15.7
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 14.4.0 - ~/.nvm/versions/node/v14.4.0/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.5 - ~/.nvm/versions/node/v14.4.0/bin/npm
  Languages:
    Python: 2.7.16 - /usr/bin/python
  Browsers:
    Chrome: 87.0.4280.88
    Firefox: 83.0
    Safari: 14.0.1
  npmPackages:
    gatsby: ^2.29.0-next.1 => 2.24.72 
    gatsby-image: ^2.4.20 => 2.4.20 
    gatsby-plugin-layout: ^1.3.13 => 1.3.13 
    gatsby-plugin-manifest: ^2.4.33 => 2.4.33 
    gatsby-plugin-offline: ^3.2.30 => 3.2.30 
    gatsby-plugin-react-helmet: ^3.3.12 => 3.3.12 
    gatsby-plugin-sharp: ^2.6.38 => 2.6.38 
    gatsby-source-filesystem: ^2.3.32 => 2.3.32 
    gatsby-transformer-sharp: ^2.5.16 => 2.5.16 
  npmGlobalPackages:
    gatsby-cli: 2.12.80
    gatsby: 2.25.3

[vrabe]

I find a weird behavior, but I don't know if it relates or not.

In this demo, if you

1. scroll to the bottom of page 1
2. click the link to page 2
3. hit the browser back button
4. hit the browser forward button
5. loop step 3 and 4

The stored (y-axis) positions change like:

action	page 1	page 2
click to page 2	2472	0
back to page 1	2472	2093
go to page 2	0	2093
back to page 1	0	2093
go to page 2	2093	2093
back to page 1	2093	0
go to page 2	2093	0
back to page 1	2093	2093
go to page 2	0	2093
back to page 1	0	2093
go to page 2	2093	2093
back to page 1	2093	0
go to page 2	2093	0
back to page 1	2093	2093
...	...	...

[vrabe]

window.scrollTo() is called twice instead of once when going back. First call is from here. I think it may be caused by #24306 .

Edit: I'm wrong. There are 2 scroll events when clicking back / forward button, but I don't know where does the other comes from.

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[blimpmason]

Not stale!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[blimpmason]

Not stale!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[gabrielkoo]

Not stale!!!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[vrabe]

Not stale!

[zuzy-dev]

hey,
are there any news to this one?
i'm having the same issue since my upgrade

[qgolsteyn]

@blimpmason I faced the same issue recently. It seems that gatsby v3.2.1 does not make any calls to window.scrollTo when shouldUpdateScroll returns false. Yet, the browser seems to be updating the scroll position anyways due to the default behaviour of the back button.

We can use the following code to disable this behaviour (tested on Chrome v89 only for now and taken from https://stackoverflow.com/a/48387790):

if ("scrollRestoration" in window.history) {
  // Back off, browser, I got this...
  window.history.scrollRestoration = "manual";
}

It may still be useful to include this snippet in gatsby however to make sure this behaviour is always disabled. So maybe still a bug.

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[blimpmason]

Not stale!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[vrabe]

Not stale!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[vrabe]

Not stale!

[jackfowler]

@qgolsteyn Your suggestion worked for me! This had been plaguing me on a client project for months, tysm.
For reference here's my final shouldUpdateScroll check:

//gatsby-brower.js

const transitionDelay = 700
export const shouldUpdateScroll = ({
  routerProps: { location },
  getSavedScrollPosition,
}) => {
  window.history.scrollRestoration = "manual"
  const currentPosition = getSavedScrollPosition(location)
  window.setTimeout(() => {
    window.scrollTo(...currentPosition)
  }, transitionDelay )
  return false
}

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[vrabe]

Not stale!

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[cwightrun]

Bump

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[vrabe]

Not stale!

[LekoArts]

Hi!

I'm closing this as a stale issue as in the meantime Gatsby 4 and related packages were released. You can check our Framework Version Support Page to see which versions currently receive active support. If this is a feature request, please create a discussion as we moved feature requests from issues to GitHub Discussions.

Please try the mentioned issue on the latest version (using the next tag) and if you still see this problem, open a new bug report. It must include a minimal reproduction.

Thanks!