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

Jump to bottom

Experimental scrollRestoration flag causes browser to make a favicon request on every scroll event #16690

Closed
exogen opened this issue Aug 29, 2020 · 2 comments · Fixed by #20633
Closed

Experimental scrollRestoration flag causes browser to make a favicon request on every scroll event #16690

exogen opened this issue Aug 29, 2020 · 2 comments · Fixed by #20633
Assignees
@Timer
Labels
kind: bug Confirmed bug that is on the backlog
Milestone
iteration 15

Comments

@exogen
Copy link
Contributor

exogen commented Aug 29, 2020

Bug report

Describe the bug

When the experimental.scrollRestoration: true feature is enabled, the resulting replaceState triggered by each scroll event will (potentially depending on caching, I assume) cause Chrome to re-request the favicon URL.

(I'm not sure why Chrome would re-request that just for a replaceState, that doesn't seem necessary, but it's happening nonetheless.)

To Reproduce

  1. Enable experimental.scrollRestoration: true in your app.
  2. Start the app and open the Network panel.
  3. Scroll the page.

Expected behavior

Scrolling should not cause network requests.

Screenshots

Kapture 2020-08-29 at 15 32 47

System information

  • macOS
  • Chrome (maybe others)
  • Next.js 9.5.2

Additional context

Maybe a solution would be to capture the scroll position the moment before the page changes, instead of on every scroll event? Was that tried already?
@Timer Timer added this to the 9.x.x milestone Aug 30, 2020
@Timer
Copy link
Member

Timer commented Aug 30, 2020

We should probably store this state somewhere other than the router.

@Timer Timer added kind: bug Confirmed bug that is on the backlog type: experimental and removed kind: story labels Dec 28, 2020
@Timer Timer modified the milestones: 10.x.x, iteration 15 Dec 28, 2020
Timer added a commit to Timer/next.js that referenced this issue Dec 31, 2020
@Timer
fix(experimental scroll): use sessionStorage instead of history m…
12200b1 
…utation

Fixes vercel#16690
Fixes vercel#17073
Fixes vercel#20486
@Timer Timer mentioned this issue Dec 31, 2020
Merged
@kodiakhq kodiakhq bot closed this as completed in #20633 Dec 31, 2020
kodiakhq bot pushed a commit that referenced this issue Dec 31, 2020
@Timer
fix(experimental scroll): use sessionStorage instead of history (#…
dbe1e62 
…20633)

This pull request adjusts our experimental scroll restoration behavior to use `sessionStorage` as opposed to `History#replaceState` to track scroll position.

In addition, **it eliminates a scroll event listener** and only captures when a `pushState` event happens (thereby leaving state that needs snapshotted).

These merely adjusts implementation detail, and is covered by existing tests:
```
test/integration/scroll-back-restoration/
```

---

Fixes #16690
Fixes #17073
Fixes #20486
@Timer Timer added the point: 5 label Dec 31, 2020
@Timer Timer self-assigned this Dec 31, 2020
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Timer Timer

Labels
kind: bug Confirmed bug that is on the backlog
Projects
None yet
Milestone
iteration 15
Development

Successfully merging a pull request may close this issue.

fix(experimental scroll): use sessionStorage instead of history Timer/next.js
3 participants
@exogen @Timer @balazsorban44