Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Scroll position is not restored after changing search param with goto #1321

Closed
Jayphen opened this issue Jul 15, 2020 · 1 comment · Fixed by #1697
Closed

Scroll position is not restored after changing search param with goto #1321

Jayphen opened this issue Jul 15, 2020 · 1 comment · Fixed by #1697

Comments

@Jayphen
Copy link
Contributor

Jayphen commented Jul 15, 2020

Describe the bug
This is slightly difficult to explain, so my apologies if it's a little bit confusing. I wrote test cases linked below that will help understanding.

Normally, when either clicking a link or using goto, the scroll position before the navigation event occurs is cached. On popstate (back/forward), Sapper looks up the destination page in the cache and restores using scrollTo.

It seems that if you use goto to change the search params (but leave the pathname the same - common when applying filters or a search query for example), the scroll position is no longer recorded before navigating away from the page. When a popstate event occurs, the scroll position is restored to the point it was at before the search params were changed.

So, for example:

  1. Visit a page
  2. Scroll down 500px and click a button that appends ?q=cheese to the URL using goto (your scrollY will now be 0)
  3. Scroll down 1000px and click a link.
  4. When on the next page, press back in your browser.
  5. 🐛 Your scrollY will be at 500px, instead of at 1000px.

I have tested this (link to test cases below), and the above works as expected as long as the button in step 2 navigates to a different pathname, instead of only changing the query param.

To Reproduce
I have written test cases here:
master...Jayphen:scrollY-after-search-param-change

The first test (restores scroll on popstate events) passes. I couldn't find any other tests in the Sapper repo that actually test scroll restoration on popstate, so I wrote this one.

The second test ('restores scroll on popstate events preceded by search param changes') fails. I've included some comments to hopefully explain what's happening.

Severity
Quite severe - we won't launch our ecommerce project until this is fixed. This results in losing your scroll position when viewing a product from a search result page and then going back, which is bad UX.

@istarkov
Copy link
Contributor

seems like its because goto implementation calls pushState before navigate call (so cid changed after pushState and not before)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants