Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scroll restoration no longer works on popState when using shouldUpdateScroll browser API to orchestrate page transitions #27349

Closed
blimpmason opened this issue Oct 9, 2020 · 8 comments
Labels
help wanted Issue with a clear description that the community can help with. status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@blimpmason
Copy link

blimpmason commented Oct 9, 2020

Description

At some point in the last six months or so the shouldUpdateScroll browser API hook and related functionality seems to have been changed under the hood and introduced breaking changes to scroll restoration for Gatsby sites that use a simple animated page transition implementation (as introduced by @ryanwiemer: https://github.com/ryanwiemer/gatsby-using-page-transitions).

I'd guess there could be a significant number of sites affected considering the many themes and starters that make use of this page transition technique.

Here's the bit of code that gets added to gatsby-browser.js to support scroll restoration with animated page transitions (which rely on gatsby-plugin-layout and your animation library of choice):

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

Essentially, scroll restoration no longer works correctly on popState when page transitions are involved.

Console logging the savedPosition that gets retrieved in the code snippet above always returns 0. There are two issues here:

  1. The function is returning a single number rather than an [x, y] coordinate array as expected per the documentation here: https://www.gatsbyjs.com/docs/browser-apis/#shouldUpdateScroll
  2. The function always returns 0 despite the scroll position saved in session storage, so even when modifying the code above to window.scrollTo(0, savedPosition || 0), the user is always sent to the top of the page when using browser back or forward buttons.

Steps to reproduce

On the demo link below, scroll all the way to the bottom of the index page, click the link to Page 2, and then click the browser back button. You will land back at the top of the index page rather than where you left off at the bottom.

Demo link: https://gatsby-scroll-restoration-issue.netlify.app/
Demo repo: https://github.com/blimpmason/gatsby-scroll-restoration-issue

Expected result

On the comparison demo link below (which uses an older version of Gatsby, v2.19.43, along with matching dependency versions), scroll all the way to the bottom of the index page, click the link to Page 2, and then click the browser back button. You should always land you exactly where you were at the bottom of the index page.

Demo link (working comparison): https://gatsby-scroll-restoration-issue-comparison.netlify.app/
Demo repo (working comparison): https://github.com/blimpmason/gatsby-scroll-restoration-issue-comparison

Actual result

You always end up back at the top of the page.

Environment

  System:
    OS: macOS 10.15.6
    CPU: (8) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
    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: 85.0.4183.121
    Firefox: 81.0
    Safari: 14.0
  npmPackages:
    gatsby: ^2.24.72 => 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

Hi to @wardpeet and @blainekasten who may have seen my misplaced comments on this issue in a merged PR thread!

@blimpmason blimpmason added the type: bug An issue or pull request relating to a bug in Gatsby label Oct 9, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 9, 2020
@blimpmason blimpmason changed the title Scroll restoration no longer works on popState when using shouldUpdateScroll browser API to coordinate page transitions Scroll restoration no longer works on popState when using shouldUpdateScroll browser API to orchestrate page transitions Oct 9, 2020
@vladar vladar added status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. topic: reach/router and Links and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 9, 2020
@vladar
Copy link
Contributor

vladar commented Oct 9, 2020

Thank you for opening this @blimpmason

And big kudos for the excellent reproduction! ❤️

I've bisected it and seems that it was introduced in [email protected]: compare.

The only suspicious PR is this one: #21626 and it is marked as a breaking change (but for the plugin not for gatsby). So this interesting.

I didn't have a chance yet to see what we can do here, so if someone is ready to tackle this - we are happy to accept a PR with a fix!

@vladar vladar added the help wanted Issue with a clear description that the community can help with. label Oct 9, 2020
@vrabe
Copy link
Contributor

vrabe commented Oct 10, 2020

Hello, I think it's because key format change in SessionStorage.

The compared one is "@@scroll|" + (location.key or location.pathname)(location.key preferred) + key(It seems key is null or undefined).

The current one is "@@scroll|" + location.path + key(We pass location.key as the key parameter).

In getSavedScrollPosition(), it doesn't pass the key parameter, so the key can't match in SessionStorage.

@github-actions
Copy link

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! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Oct 31, 2020
@blimpmason
Copy link
Author

Not stale!

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 2, 2020
@github-actions
Copy link

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! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 22, 2020
@vrabe
Copy link
Contributor

vrabe commented Nov 22, 2020

Not stale!

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 22, 2020
@vladar
Copy link
Contributor

vladar commented Dec 1, 2020

Thank you for opening this, @blimpmason

This should be fixed in #27384 and will be published in the next couple of days in 2.28.x release line.

@vladar vladar closed this as completed Dec 1, 2020
@blimpmason
Copy link
Author

Thanks @vrabe and @vladar! I just updated to [email protected] to test the fix and it looks like the scroll coordinates are now coming back as an array as expected so scroll position can be used once again without modifying the gatsby-browser shouldUpdateScroll function.

Unfortunately I'm still experiencing an issue in which the page scrolls to the restored position before the setTimeout has completed — it seems the router is ignoring the shouldUpdateScroll logic.

Example here: 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.

I will open a new issue to address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issue with a clear description that the community can help with. status: confirmed Issue with steps to reproduce the bug that’s been verified by at least one reviewer. type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

No branches or pull requests

4 participants