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

[Bug]: ScrollRestoration does not go to page top when redirecting in an action #9577

Closed
john-evan08 opened this issue Nov 9, 2022 · 8 comments
Assignees
Labels

Comments

@john-evan08
Copy link

john-evan08 commented Nov 9, 2022

What version of React Router are you using?

6.4.3

Steps to Reproduce

<Route 
     path="/"
     element={
	<div>
		<ScrollRestoration />
		<Outlet />
	</div>
	}
>
            <Route
	            path="post_route"
	            action={async ({ params }) => {
                           await patchServer()
	                  return redirect(`redirect_route`)
                     }}
            />

</Route>

The action is called with a <Form method="patch"/>

Expected Behavior

The action is called and redirect to the url redirect_route, but without scrolling to the top... I expected the page to scroll to the top when calling the action (and returning the redirect)

Actual Behavior

The action is called and the page redirects to the new route, but without scrolling to the top (it stays at the same place)

@john-evan08 john-evan08 added the bug label Nov 9, 2022
@timdorr timdorr changed the title [Bug]: [Bug]: ScrollRestoration does not go to page top when redirecting in an action Nov 17, 2022
@brophdawg11
Copy link
Contributor

Hm, yeah by default scroll restoration is skipped on submissions, but if you redirect from an action that should probably re-enable it. We would then likely need to add preventScrollReset to <Form> like we have on <Link> as well.

@john-evan08
Copy link
Author

@brophdawg11 Thank you !! I am not sure I understood anything. How should I fix this issue in my case ?

@brophdawg11 brophdawg11 self-assigned this Nov 28, 2022
@brophdawg11
Copy link
Contributor

I think it's a bug, so I don't think there's a way to work around it at the moment unfortunately. We'll try to get to it in an upcoming release, but if you or anyone else was interested in taking a stab at a PR, I think the change would happen in completeNavigation in packages/router/router.ts.

Right now, we always disable scroll restoration on submissions:

updateState({
  ...
  restoreScrollPosition: state.navigation.formData
    ? false
    : getSavedScrollPosition(location, newState.matches || state.matches),
});

But that's incorrectly including redirects after submissions because the formData sticks around. I think that probably needs to become something like the following to only prevent scroll restoration when the new location matches the location the form was submitted to (meaning the action didn't redirect):

let preventScrollRestoration = (
  state.navigation.formData && 
  location.pathname + location.search === state.navigation.formAction
);

updateState({
  ...
  restoreScrollPosition: preventScrollRestoration
    ? false
    : getSavedScrollPosition(location, newState.matches || state.matches),
});

@ferdinandsalis
Copy link

I have been trying to fix this but to no avail via patch-package. Specifically I made the above changes locally, built the router package and copied the files over to my node_modules/@remix-run/router folder of my project. However the changes do not seem to make any difference.

Is there anything I am missing here?

This bug is quite problematic in terms of ux in my app.

@jrakotoharisoa
Copy link
Contributor

Hi, I encounter the same issue.

The fix proposed by @brophdawg11 seems to solve the problem for me so I've made a PR.

@brophdawg11
Copy link
Contributor

This is now released in a 6.7.0-pre.1 prerelease if you want to give it a shot to see if it fixes your issues. The stable should be out early next week hopefully.

@jrakotoharisoa
Copy link
Contributor

Thanks, this release fix my issue 🙂

@brophdawg11
Copy link
Contributor

Released in 6.7.0

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

No branches or pull requests

4 participants