From a876b1ef3787516765131970b1c7c17164fb2067 Mon Sep 17 00:00:00 2001 From: Johann Rakotoharisoa Date: Wed, 4 Jan 2023 23:21:52 +0100 Subject: [PATCH 1/5] restore scroll when redirecting in an action --- packages/router/__tests__/router-test.ts | 31 ++++++++++++++++++++++++ packages/router/router.ts | 7 ++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 39d2c8aac2..4cd600cb46 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6485,6 +6485,37 @@ describe("a router", () => { expect(t.router.state.preventScrollReset).toBe(false); }); + it("does restore scroll on submissions that redirecting", async () => { + let t = setup({ + routes: SCROLL_ROUTES, + initialEntries: ["/tasks"], + hydrationData: { + loaderData: { + index: "INDEX_DATA", + }, + }, + }); + + expect(t.router.state.restoreScrollPosition).toBe(false); + expect(t.router.state.preventScrollReset).toBe(false); + let positions = { "/tasks": 100 }; + let activeScrollPosition = 0; + t.router.enableScrollRestoration( + positions, + () => activeScrollPosition, + (l) => l.pathname + ); + + let nav1 = await t.navigate("/tasks", { + formMethod: "post", + formData: createFormData({}), + }); + const nav2 = await nav1.actions.tasks.redirectReturn("/"); + await nav2.loaders.index.resolve("INDEX"); + expect(t.router.state.restoreScrollPosition).not.toBe(false); + expect(t.router.state.preventScrollReset).toBe(false); + }); + it("does not restore scroll on submissions", async () => { let t = setup({ routes: SCROLL_ROUTES, diff --git a/packages/router/router.ts b/packages/router/router.ts index 6365a49e58..adb823c9c2 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -771,6 +771,10 @@ export function createRouter(init: RouterInit): Router { ) : state.loaderData; + // Don't restore on submission navigations when no redirection + let preventScrollRestoration = + state.navigation.formData && + location.pathname + location.search === state.navigation.formAction; updateState({ ...newState, // matches, errors, fetchers go through as-is actionData, @@ -780,8 +784,7 @@ export function createRouter(init: RouterInit): Router { initialized: true, navigation: IDLE_NAVIGATION, revalidation: "idle", - // Don't restore on submission navigations - restoreScrollPosition: state.navigation.formData + restoreScrollPosition: preventScrollRestoration ? false : getSavedScrollPosition(location, newState.matches || state.matches), preventScrollReset: pendingPreventScrollReset, From f0f94a51d9a94fbc12d07e4669ea42505de65083 Mon Sep 17 00:00:00 2001 From: Johann R Date: Wed, 4 Jan 2023 23:30:51 +0100 Subject: [PATCH 2/5] Create scroll-restoration-action-redirect.md --- .changeset/scroll-restoration-action-redirect.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/scroll-restoration-action-redirect.md diff --git a/.changeset/scroll-restoration-action-redirect.md b/.changeset/scroll-restoration-action-redirect.md new file mode 100644 index 0000000000..186e3c8260 --- /dev/null +++ b/.changeset/scroll-restoration-action-redirect.md @@ -0,0 +1,5 @@ +--- +"@remix-run/router": patch +--- + +Fix scroll restoration when redirecting in an action From edabcedd50daf357759a3f8d029881fdea3e97e8 Mon Sep 17 00:00:00 2001 From: Johann Rakotoharisoa Date: Thu, 5 Jan 2023 00:04:59 +0100 Subject: [PATCH 3/5] avoid false positive with location.state._isRedirect --- packages/router/router.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index adb823c9c2..ba69c575fe 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -773,8 +773,7 @@ export function createRouter(init: RouterInit): Router { // Don't restore on submission navigations when no redirection let preventScrollRestoration = - state.navigation.formData && - location.pathname + location.search === state.navigation.formAction; + state.navigation.formData && location.state?._isRedirect !== true; updateState({ ...newState, // matches, errors, fetchers go through as-is actionData, From 0cdb3ac744ece8f1c4af22f2dcf3d8122d034491 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 10 Jan 2023 11:55:07 -0500 Subject: [PATCH 4/5] Add additional test --- packages/router/__tests__/router-test.ts | 41 ++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts index 4cd600cb46..cc5eceabec 100644 --- a/packages/router/__tests__/router-test.ts +++ b/packages/router/__tests__/router-test.ts @@ -6485,7 +6485,7 @@ describe("a router", () => { expect(t.router.state.preventScrollReset).toBe(false); }); - it("does restore scroll on submissions that redirecting", async () => { + it("restores scroll on submissions that redirect to the same location", async () => { let t = setup({ routes: SCROLL_ROUTES, initialEntries: ["/tasks"], @@ -6498,7 +6498,42 @@ describe("a router", () => { expect(t.router.state.restoreScrollPosition).toBe(false); expect(t.router.state.preventScrollReset).toBe(false); + // We were previously on tasks at 100 let positions = { "/tasks": 100 }; + // But we've scrolled up to 50 to submit. We'll save this overtop of + // the 100 when we start this submission navigation and then restore to + // 50 below + let activeScrollPosition = 50; + t.router.enableScrollRestoration( + positions, + () => activeScrollPosition, + (l) => l.pathname + ); + + let nav1 = await t.navigate("/tasks", { + formMethod: "post", + formData: createFormData({}), + }); + const nav2 = await nav1.actions.tasks.redirectReturn("/tasks"); + await nav2.loaders.tasks.resolve("TASKS"); + expect(t.router.state.restoreScrollPosition).toBe(50); + expect(t.router.state.preventScrollReset).toBe(false); + }); + + it("restores scroll on submissions that redirect to new locations", async () => { + let t = setup({ + routes: SCROLL_ROUTES, + initialEntries: ["/tasks"], + hydrationData: { + loaderData: { + index: "INDEX_DATA", + }, + }, + }); + + expect(t.router.state.restoreScrollPosition).toBe(false); + expect(t.router.state.preventScrollReset).toBe(false); + let positions = { "/": 50, "/tasks": 100 }; let activeScrollPosition = 0; t.router.enableScrollRestoration( positions, @@ -6512,10 +6547,10 @@ describe("a router", () => { }); const nav2 = await nav1.actions.tasks.redirectReturn("/"); await nav2.loaders.index.resolve("INDEX"); - expect(t.router.state.restoreScrollPosition).not.toBe(false); + expect(t.router.state.restoreScrollPosition).toBe(50); expect(t.router.state.preventScrollReset).toBe(false); }); - + it("does not restore scroll on submissions", async () => { let t = setup({ routes: SCROLL_ROUTES, From 859b09350268e4d579f9428925d6911d98c70151 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 10 Jan 2023 11:56:58 -0500 Subject: [PATCH 5/5] Extract entire restoreScrollPosition variable --- packages/router/router.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/router/router.ts b/packages/router/router.ts index ba69c575fe..b6cab2498e 100644 --- a/packages/router/router.ts +++ b/packages/router/router.ts @@ -771,9 +771,12 @@ export function createRouter(init: RouterInit): Router { ) : state.loaderData; - // Don't restore on submission navigations when no redirection - let preventScrollRestoration = - state.navigation.formData && location.state?._isRedirect !== true; + // Don't restore on submission navigations unless they redirect + let restoreScrollPosition = + state.navigation.formData && location.state?._isRedirect !== true + ? false + : getSavedScrollPosition(location, newState.matches || state.matches); + updateState({ ...newState, // matches, errors, fetchers go through as-is actionData, @@ -783,9 +786,7 @@ export function createRouter(init: RouterInit): Router { initialized: true, navigation: IDLE_NAVIGATION, revalidation: "idle", - restoreScrollPosition: preventScrollRestoration - ? false - : getSavedScrollPosition(location, newState.matches || state.matches), + restoreScrollPosition, preventScrollReset: pendingPreventScrollReset, });