fix: Scroll restoration bug caused by `idx` reset to 0 on reload #36861

AkifumiSato · 2022-05-12T14:35:21Z

If experimental.scrollRestoration = true, reloading resets the value of Router's _idx to 0, so the scroll position cannot be restored correctly between pages.
Since this is stored in session storage, the scroll position may be restored with incorrect values.

Load /person/1

+-----------+
| /person/1 |
| idx: 0    |
+-----------+

Move to /person/2

+-----------+-----------+
| /person/1 | /person/2 |
| idx: 0    | idx: 1    |
+-----------+-----------+

Reload /person/2

+-----------+-----------+
| /person/1 | /person/2 |
| idx: 0    | idx: 0    |
+-----------+-----------+

This modification restores the position of the browser back after reloading.
At the end of the first video, the scroll is not restored on browser back after reloading and is set to 0.
In the second video, the position is restored correctly.

before.mov

after.mov

I made a similar correction in the following PR, but I had to make a new one because I had included other correction.
#35770

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes by running yarn lint

changed key from index to random string, to be inconsistent with session storage when reloading

mikestead · 2022-05-13T07:24:54Z

Thanks for doing this work @AkifumiSato , I've been running into a related issue. Worked around my one partially by accessing idx inside history.state but that's obv not ideal at all, and fundamentally using index numbering causes restore bugs.

As you've addressed, a unique key is needed... you can see the same approach used if you check history.state of sites like twitter and instagram.

I would ask that you ideally make the key public. This allows developers to write their own page restoration logic if they need to, as I'm needing to currently (using idx hack). Otherwise I'll just have to access via history.state again, not ideal.

ijjk · 2022-05-13T14:13:45Z

Failing test suites

Commit: 17fa5d0

yarn testheadless test/integration/cli/test/index.test.js

CLI Usage > info > should print output

Expand output

● CLI Usage › info › should print output

expect(received).toBe(expected) // Object.is equality

Expected: ""
Received: "warn  - Latest canary version not detected, detected: \"12.1.7-canary.4\", newest: \"12.1.7-canary.5\".
        Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
        Read more - https://nextjs.org/docs/messages/opening-an-issue
"

  496 |       // warning will show so skip this check for the stable release
  497 |       if (pkg.version.includes('-canary')) {
> 498 |         expect(info.stderr || '').toBe('')
      |                                   ^
  499 |       }
  500 |       expect(info.stdout).toMatch(
  501 |         new RegExp(`

  at Object.<anonymous> (integration/cli/test/index.test.js:498:35)

Read more about building and testing Next.js in contributing.md.

ijjk · 2022-05-13T14:30:02Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
buildDuration	15.2s	15.3s	⚠️ +51ms
buildDurationCached	6.4s	6.4s	-16ms
nodeModulesSize	479 MB	479 MB	⚠️ +335 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.751	3.703	-0.05
/ avg req/sec	666.54	675.06	+8.52
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.169	1.144	-0.03
/error-in-render avg req/sec	2138.71	2185.65	+46.94

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	29.2 kB	29.2 kB	⚠️ +31 B
webpack-HASH.js gzip	1.54 kB	1.54 kB	✓
Overall change	72.9 kB	73 kB	⚠️ +31 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	308 B	308 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	359 B	359 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.73 kB	5.73 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.65 kB	2.65 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	391 B	391 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16 kB	16 kB	✓

Client Build Manifests

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
index.html gzip	533 B	533 B	✓
link.html gzip	547 B	547 B	✓
withRouter.html gzip	528 B	528 B	✓
Overall change	1.61 kB	1.61 kB	✓

Diffs

Diff for main-HASH.js

@@ -4533,6 +4533,7 @@
       exports.isLocalURL = isLocalURL;
       exports.interpolateAs = interpolateAs;
       exports.resolveHref = resolveHref;
+      exports.createKey = createKey;
       exports["default"] = void 0;
       var _normalizeTrailingSlash = __webpack_require__(2700);
       var _routeLoader = __webpack_require__(2497);
@@ -4931,6 +4932,11 @@
             throw err;
           }));
       }
+      function createKey() {
+        return Math.random()
+          .toString(36)
+          .slice(2, 10);
+      }
       var Router = /*#__PURE__*/ (function() {
         function Router(pathname1, query1, as1, param) {
           var initialProps = param.initialProps,
@@ -4955,7 +4961,7 @@
           this.sdr = {};
           // In-flight middleware preflight requests
           this.sde = {};
-          this._idx = 0;
+          this._key = createKey();
           this.onPopState = function(e) {
             var state = e.state;
             if (!state) {
@@ -4987,11 +4993,11 @@
             var url = state.url,
               as = state.as,
               options = state.options,
-              idx = state.idx;
+              key = state.key;
             if (false) {
               var v;
             }
-            _this._idx = idx;
+            _this._key = key;
             var pathname2 = (0, _parseRelativeUrl).parseRelativeUrl(url)
               .pathname;
             // Make sure we don't re-render on initial load,
@@ -5947,8 +5953,8 @@
                     as: as,
                     options: options,
                     __N: true,
-                    idx: (this._idx =
-                      method !== "pushState" ? this._idx : this._idx + 1)
+                    key: (this._key =
+                      method !== "pushState" ? this._key : createKey())
                   }, // Passing the empty string here should be safe against future changes to the method.
                   // https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState
                   "",

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
buildDuration	17.6s	17.5s	-155ms
buildDurationCached	6.3s	6.4s	⚠️ +62ms
nodeModulesSize	479 MB	479 MB	⚠️ +335 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.717	3.794	⚠️ +0.08
/ avg req/sec	672.67	658.88	⚠️ -13.79
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.122	1.171	⚠️ +0.05
/error-in-render avg req/sec	2228.89	2135.23	⚠️ -93.66

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.7 kB	42.7 kB	✓
main-HASH.js gzip	29.6 kB	29.7 kB	⚠️ +32 B
webpack-HASH.js gzip	1.54 kB	1.54 kB	✓
Overall change	74 kB	74.1 kB	⚠️ +32 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	324 B	324 B	✓
dynamic-HASH.js gzip	2.89 kB	2.89 kB	✓
head-HASH.js gzip	357 B	357 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.82 kB	5.82 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.78 kB	2.78 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	392 B	392 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16.3 kB	16.3 kB	✓

Client Build Manifests

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
_buildManifest.js gzip	457 B	457 B	✓
Overall change	457 B	457 B	✓

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	AkifumiSato/next.js fix-scroll-restoration	Change
index.html gzip	532 B	531 B	-1 B
link.html gzip	547 B	546 B	-1 B
withRouter.html gzip	529 B	527 B	-2 B
Overall change	1.61 kB	1.6 kB	-4 B

Diffs

Diff for main-HASH.js

@@ -4533,6 +4533,7 @@
       exports.isLocalURL = isLocalURL;
       exports.interpolateAs = interpolateAs;
       exports.resolveHref = resolveHref;
+      exports.createKey = createKey;
       exports["default"] = void 0;
       var _normalizeTrailingSlash = __webpack_require__(2700);
       var _routeLoader = __webpack_require__(2497);
@@ -4931,6 +4932,11 @@
             throw err;
           }));
       }
+      function createKey() {
+        return Math.random()
+          .toString(36)
+          .slice(2, 10);
+      }
       var Router = /*#__PURE__*/ (function() {
         function Router(pathname1, query1, as1, param) {
           var initialProps = param.initialProps,
@@ -4955,7 +4961,7 @@
           this.sdr = {};
           // In-flight middleware preflight requests
           this.sde = {};
-          this._idx = 0;
+          this._key = createKey();
           this.onPopState = function(e) {
             var state = e.state;
             if (!state) {
@@ -4987,11 +4993,11 @@
             var url = state.url,
               as = state.as,
               options = state.options,
-              idx = state.idx;
+              key = state.key;
             if (false) {
               var v;
             }
-            _this._idx = idx;
+            _this._key = key;
             var pathname2 = (0, _parseRelativeUrl).parseRelativeUrl(url)
               .pathname;
             // Make sure we don't re-render on initial load,
@@ -5947,8 +5953,8 @@
                     as: as,
                     options: options,
                     __N: true,
-                    idx: (this._idx =
-                      method !== "pushState" ? this._idx : this._idx + 1)
+                    key: (this._key =
+                      method !== "pushState" ? this._key : createKey())
                   }, // Passing the empty string here should be safe against future changes to the method.
                   // https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState
                   "",

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-06bbb6769d0ae4b4.js"
+      src="/_next/static/chunks/main-dc9db7a6be528c25.js"
       defer=""
     ></script>
     <script

Commit: ccd31a0

mikestead · 2022-05-15T00:02:22Z

I wanted to add a little more detail of why I believe it's important to expose key for custom page restoration wiring.

Window scroll position is only a single restore state to factor in, but there can be things like carousel horizontal scroll positions inside of a page, or a ui component that's been expanded and then a link clicked. These are states that are not ideally stored as query parameters, but do need to be unique to the history position as the same page may be seen multiple times in history and have different internal restore state.

AkifumiSato · 2022-05-15T12:30:00Z

@mikestead Thanks for your comment!
I have the same problem and once created a PR to publish the key.
#35770
Remix has a similar feature, and the WICG is considering a similar policy.
https://github.com/remix-run/history/blob/main/docs/api-reference.md#locationkey
https://github.com/WICG/navigation-api#the-current-entry
If the maintainer allows, I would like to publish the key as well.

AkifumiSato · 2022-05-15T12:33:53Z

The test is failing, but it appears to be unrelated to my fix.
https://github.com/vercel/next.js/runs/6424377024?check_suite_focus=true

ijjk

Thanks!

AkifumiSato · 2022-05-26T05:26:59Z

@mikestead New pull request created.
#37192

fix scroll restoration bug

17fa5d0

changed key from index to random string, to be inconsistent with session storage when reloading

AkifumiSato requested review from timneutkens, ijjk, shuding and huozhi as code owners May 12, 2022 14:35

ijjk added the type: next label May 12, 2022

AkifumiSato mentioned this pull request May 12, 2022

fix: scroll restoration #35770

Closed

10 tasks

Merge branch 'canary' into fix-scroll-restoration

ccd31a0

ijjk approved these changes May 23, 2022

View reviewed changes

ijjk merged commit ad7f728 into vercel:canary May 23, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Scroll restoration bug caused by `idx` reset to 0 on reload #36861

fix: Scroll restoration bug caused by `idx` reset to 0 on reload #36861

AkifumiSato commented May 12, 2022

mikestead commented May 13, 2022

ijjk commented May 13, 2022

ijjk commented May 13, 2022 •

edited

Loading

Diffs

Diffs

mikestead commented May 15, 2022

AkifumiSato commented May 15, 2022 •

edited

Loading

AkifumiSato commented May 15, 2022

ijjk left a comment

AkifumiSato commented May 26, 2022

fix: Scroll restoration bug caused by idx reset to 0 on reload #36861

fix: Scroll restoration bug caused by idx reset to 0 on reload #36861

Conversation

AkifumiSato commented May 12, 2022

Bug

Feature

Documentation / Examples

mikestead commented May 13, 2022

ijjk commented May 13, 2022

Failing test suites

ijjk commented May 13, 2022 • edited Loading

Stats from current PR

Diffs

Diffs

mikestead commented May 15, 2022

AkifumiSato commented May 15, 2022 • edited Loading

AkifumiSato commented May 15, 2022

ijjk left a comment

Choose a reason for hiding this comment

AkifumiSato commented May 26, 2022

fix: Scroll restoration bug caused by `idx` reset to 0 on reload #36861

fix: Scroll restoration bug caused by `idx` reset to 0 on reload #36861

ijjk commented May 13, 2022 •

edited

Loading

AkifumiSato commented May 15, 2022 •

edited

Loading