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

[v2] Navigating to previously visited pages with <Link> retains scroll position #7454

Closed
robinpyon opened this issue Aug 19, 2018 · 51 comments · Fixed by #7758 or #8359
Closed

[v2] Navigating to previously visited pages with <Link> retains scroll position #7454

robinpyon opened this issue Aug 19, 2018 · 51 comments · Fixed by #7758 or #8359
Assignees
Labels
help wanted Issue with a clear description that the community can help with. stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@robinpyon
Copy link
Contributor

robinpyon commented Aug 19, 2018

Description

Clicking on a <Link> in v2 (111) appears to scroll you to a previous scroll position (if it exists) rather than scroll you to the top.

Steps to reproduce

I've been able to recreate this in gatsby-starter-default:
https://gatsby-v2-beta-111-scroll.netlify.com/

(The only change is artificial padding added to the first page above the /page-2 <Link>)

Expected result

You are taken back to the top of the home page

Actual result

You are taken back to the home page but scrolled down the page (at the exact same position you were when you linked the page-2 link)

Environment

System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 8.11.1 - ~/.nvm/versions/node/v8.11.1/bin/node
    Yarn: 1.9.4 - ~/.nvm/versions/node/v8.11.1/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v8.11.1/bin/npm
  Browsers:
    Chrome: 68.0.3440.106
    Firefox: 61.0.1
    Safari: 11.1.2
  npmPackages:
    gatsby: next => 2.0.0-beta.111
    gatsby-plugin-manifest: next => 2.0.2-beta.6
    gatsby-plugin-offline: next => 2.0.0-beta.9
    gatsby-plugin-react-helmet: next => 3.0.0-beta.4
  npmGlobalPackages:
    gatsby-cli: 1.1.58

Other notes

  • This behaviour doesn't occur on the v1 starter.
  • I understand that there was a switch to reach-router, could this be linked?
  • Manually adding window.scrollTo(0, 0) / window.scroll({top: 0}) in page componentDidMount had no effect, though it would work after wrapping it in an arbitrary setTimeout - though this yields an initial flash of content in the wrong scroll position.

By default, I imagine the desired behaviour would be for all clicked <Link> elements to scroll you to the top of that target page and that only using the browser back / forward buttons would retain scroll position state in this manner.

@stoutlabs
Copy link
Contributor

I can confirm this same behavior is happening on a couple sites I'm working on. I just noticed it today, but have no idea how long it has been happening. (I came here to search for the same issue or create a new one.)

@Manoz
Copy link
Contributor

Manoz commented Aug 20, 2018

Could be related to #7450 ?
I noticed the same behavior. I though it was a feature since it’s not totally stupid.
If I read a very long blog post and click on a link it’s nice to go back where I was without scrolling the whole page, even if it’s a weird behavior.

@colbyhub
Copy link

I'd rather it default to the top of the page rather than the previous scroll position, it would create a more predictable experience.
Could the <Link> component default to not scrolling to the previous scroll position, but still have it as an option via a prop?

@DylanVann
Copy link

DylanVann commented Aug 20, 2018

Could be related: #5656

The logic discussed for determining if something is a pop could be the issue. We want to restore scroll on pop (back button pushed), not if you're just going to a link you've been to before.

There is no action on Reach Router so this probably doesn't work anymore: https://github.com/gatsbyjs/gatsby/pull/3775/files

@Chuloo
Copy link
Contributor

Chuloo commented Aug 20, 2018

Verified, Here's a simple repro using the Blog-Starter .
Clone repo and run npm install followed by gatsby develop.

@Chuloo Chuloo added type: bug An issue or pull request relating to a bug in Gatsby help wanted Issue with a clear description that the community can help with. labels Aug 20, 2018
@Chuloo Chuloo added this to Needs Verification in Gatsby v2 Release via automation Aug 20, 2018
@thebigredgeek
Copy link
Contributor

So it seems like this might be blocked by a necessary API change to reach-router? Otherwise it sounds like we have to keep a hash of seen routing keys in order to determine push vs pop

@humphreybc
Copy link
Contributor

humphreybc commented Aug 23, 2018

FYI I'm seeing this not just with previously visited links, but with all links. Unless “previously visited” includes visiting the URL in a previous browser session.

@monsieurnebo
Copy link
Contributor

Same behavior over here.

@ryanwiemer
Copy link
Contributor

ryanwiemer commented Aug 24, 2018

I too can confirm this behavior when using gatsbyv2. It is a particularly bad user experience when you have next/previous links at the bottom of the page as it keeps the user at the bottom.

@jkimbo
Copy link

jkimbo commented Aug 24, 2018

We came across this issue on https://www.zego.com and developed a work around by using our own custom Link component that sets a flag on the window:

// components/Link.js
import { Link as BaseLink } from 'gatsby';

if (typeof window !== 'undefined') {
  window.__navigatingToLink = false;
}

const Link = ({ children, ...props }) => (
	<BaseLink onClick={() => { window.__navigatingToLink = true; }} {...props}>
		{children}
	</BaseLink>
);

Then in gatsby-browser.js:

// gatsby-browser.js
exports.shouldUpdateScroll = () => {
  if (window.__navigatingToLink === true) {
    return [0, 0];
  }
  return true;
};

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

It's pretty hacky but it works. All our page links scroll to the top of the page like expected and the back button still maintains the scroll position on the previous page.

@CanRau
Copy link
Contributor

CanRau commented Aug 24, 2018

great thanks for the tip @jkimbo that works 👍

CanRau added a commit to GaiAma/gaiama.org that referenced this issue Aug 26, 2018
we like to jump to the menu on page navigations, excerpt for browser made ones like back button
I had to fix this behaviour as reach/router does not (yet) provide the used action (POP, PUSH)
using a quick method suggested by gatsbyjs/gatsby#7454 (comment)
@KyleAMathews
Copy link
Contributor

@jkimbo great solution! I'll add that to core while we're awaiting a fix upstream in @reach/router.

@KyleAMathews
Copy link
Contributor

Here's the link to the @reach/router issue reach/router#119

@steverandy
Copy link

I'm using v2.0.0-rc.24 and it seems to be having similar issue again.

@ryanwiemer
Copy link
Contributor

ryanwiemer commented Sep 15, 2018

@KyleAMathews,

Did your temporary fix get reverted or canceled out somehow? I am seeing the undesirable scrolling behavior again using v2.0.0-rc.25.

@valin4tor
Copy link
Contributor

valin4tor commented Sep 15, 2018

Some navigation code was rewritten recently which caused this behaviour to appear again - I think this is on @pieh's todo list

edit: see #7812 (comment)

@valin4tor valin4tor reopened this Sep 15, 2018
Gatsby v2 Release automation moved this from Done to Needs Verification Sep 15, 2018
@ryanwiemer
Copy link
Contributor

@davidbailey00 and @pieh,

Thanks for the update and background info! Hopefully this gets resolved in the near future as v2 has been officially released.

Let me know if you need help with anything.

@pieh
Copy link
Contributor

pieh commented Sep 18, 2018

Yeah, I probably broke it and will take a look on this

@AllanPooley
Copy link
Contributor

Hi guys, debugging this for a few hours last night, to no avail.

However, this morning I was blessed with an epiphany sent by the software engineering gods! 🕊️

I had some pretty funky styles to make a togglable navigation drawer / menu appear and disappear by pushing away the app's main containing element.

To make this work, I had added:

html {
  overflow: hidden;
}

In my case, removing this style fixed the behaviour!!

Hope this helps someone else.

@thundernixon
Copy link

thundernixon commented Feb 11, 2019

@davidbailey00 I'm currently having this issue.

link-scroll-oddity

Here's a repo that reproduces this issue on gatsby v2.0.118 (currently the latest version):

https://github.com/thundernixon/gatsby-link-scroll-test

Not only is it linking to previously-scrolled-to spots in pages (bad), but it’s also linking to the current location on unvisited pages (worse). That is, the pagination at the bottom of a “blog post” type of page will link to the bottom of the next page. I also have a header drop-down menu with links to different pages, and these also don't link to the tops of these pages.


UPDATE: Nevermind; it was an issue with how I was using Gatsby Transition Link. It was resolved at TylerBarnes/gatsby-plugin-transition-link#21.

@valin4tor
Copy link
Contributor

@thundernixon Thanks for your update! I'll presume there's no further issue

@AllanPooley Please could you share your repo, or a reproduction? It sounds like this could still be an issue, even if it only happens with the body styled as overflow: hidden, so ideally we'd like to get this fixed for all cases :)

@ergunpp
Copy link

ergunpp commented Feb 24, 2019

Hi,
I have tried as @jkimbo with my project but I'm still having the same problem. I have added a Link.js under components, which I import to pages linking to other pages. I also added the code to my gatsby-browser.js. Same problem

@valin4tor
Copy link
Contributor

@ergunpp Please could you share your repo, or a minified reproduction, so we can investigate this?

@ergunpp
Copy link

ergunpp commented Mar 10, 2019

@davidbailey00 repo name: ergunpp/chefschoice

@t2ca
Copy link
Contributor

t2ca commented Mar 16, 2019

The same issue currently exists when using semantic-ui-react and it's because of the following css code:

body {
  height: 100%;
}

So i fixed it with the following:

html {
  height: initial;
}

@ergunpp
Copy link

ergunpp commented Mar 16, 2019 via email

@valin4tor
Copy link
Contributor

valin4tor commented Apr 2, 2019

@ergunpp I just had a look at your site - it seems the problem is caused by your overflow-x: hidden and styling with the body tag. If you remove this property, as well as the overflow-y property, and then fix the page overflowing by changing various instances of 100vw to 100%, the problem is fixed :)

@valin4tor
Copy link
Contributor

I think after @pieh's fixes, any problems occurring are due to the scroll-behavior package, which is outside of our control, since the scroll update events seem to be firing correctly.

@KyleAMathews
Copy link
Contributor

If scroll-behavior has problems, we can PR fixes there. We've done that several times.

@valin4tor
Copy link
Contributor

@KyleAMathews I think it's not even a problem with scroll-behavior - even calling window.scrollTo(0, 0) in the console doesn't work with some of those styles applied, so I think this isn't something we can fix, apart from telling people not to set overflow: hidden. Should we close?

@ergunpp
Copy link

ergunpp commented Apr 3, 2019

@ergunpp I just had a look at your site - it seems the problem is caused by your overflow-x: hidden and styling with the body tag. If you remove this property, as well as the overflow-y property, and then fix the page overflowing by changing various instances of 100vw to 100%, the problem is fixed :)

Hi @davidbailey00, Your suggestions worked like a charm. Thanks for the great help!

@gatsbot
Copy link

gatsbot bot commented Apr 24, 2019

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 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. You can also add the label "not stale" to keep this issue open!

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

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Apr 24, 2019
@valin4tor
Copy link
Contributor

Closing, since this has been fixed (or else caused by dodgy plugins / CSS). Please open a new issue if any further problems occur :)

@nandorojo
Copy link
Contributor

nandorojo commented May 7, 2019

This worked for me.

To scroll to the top, make sure your path starts with /. Example:

// this works
<Link to="/some-path" />

I realized it wasn't scrolling to the top when I didn't have a / at the start like this:

// this doesn't work
<Link to="some-path" />

Not sure if that works for every case, but it worked for me. Hope it helps.

@mavam
Copy link

mavam commented Jun 8, 2019

@t2ca your html CSS tweak fixed it for me after trying pretty much everything else here. 💯

I'm also using Semantic UI React and have added the following to site.overrides:

html {
  height: initial;
}

@rishichawda
Copy link

After wasting a whole hour going through all the issues and references, nothing worked except manually setting the scrollTop on document.body to 0 like so:

export const shouldUpdateScroll = () => {
    document.body.scrollTop = 0
}

Works for both refresh and page visits.

@Uhiyamind
Copy link

Uhiyamind commented Jul 9, 2020

This worked for me.

gatsby-browser.js

exports.onInitialClientRender = () => {
  window.scrollTo(0, 0)
}

I found this solution on
https://github.com/gatsbyjs/gatsby/issues/19488#issuecomment-554007194

@lvtz
Copy link

lvtz commented Oct 14, 2020

This worked for me.

gatsby-browser.js

exports.onInitialClientRender = () => {
  window.scrollTo(0, 0)
}

It does not work, and seems it can't - definition of this function looks like this:

Called when the initial (but not subsequent) render of Gatsby App is done on the client.

In my case it work great, no more scroll retain issue when navigating using Link from Gatsby.

// gatsby-browser.js

export const onPreRouteUpdate = () => {
  document.body.scrollTop = 0
}

@GesJeremie
Copy link

GesJeremie commented Jan 20, 2021

This worked pretty well:

// gatsby-browser.js
export const shouldUpdateScroll = () => {
  return false
}

Source: https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#shouldUpdateScroll

edit: ^ this fucks up /#anchors (duh!), don't recommend, 0/10

edit2: ended up with this

// gatsby-browser.js
export const shouldUpdateScroll = ({ routerProps }) => {
  const hasAnchor = !!routerProps.location.hash; // (ie. /#features)

  return hasAnchor
}

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. stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby
Projects
No open projects
Gatsby v2 Release
  
Needs Verification