Cannot reset scroll in the popstate event

[mx1700]

waku/packages/waku/src/router/client.tsx

Lines 893 to 904 in a685fc1

	useEffect(() => {
	const callback = () => {
	const route = parseRoute(new URL(window.location.href));
	changeRoute(route, { shouldScroll: true }).catch((err) => {
	console.log('Error while navigating back:', err);
	});
	};
	window.addEventListener('popstate', callback);
	return () => {
	window.removeEventListener('popstate', callback);
	};
	}, [changeRoute]);

If scroll is not reset here, the browser will automatically restore the scrolling position, which is in line with the general behavior of browsers.

[dai-shi]

Thanks for opening up a new issue.
Though, I'm not sure if I understand the description. Do you mean shouldScroll: true is problematic? We would like to make it the general browser behavior.

[mx1700]

Thanks for opening up a new issue.
Though, I'm not sure if I understand the description. Do you mean shouldScroll: true is problematic? We would like to make it the general browser behavior.

Yes!
shouldScroll should be false
When you scroll to the bottom of the page, click on a link, and then click on the browser's back button, it should generally return to the previous page, while the scrolling position is still at the bottom
The current situation is that because shouldScroll is true, it always returns to the top of the page

[dai-shi]

Sounds about right. I'm not sure why I did shouldScroll: true.

@rmarscher @tylersayshi Any thoughts?

[rmarscher]

I agree with this. This might be related too - #1212 (comment)
I'm not sure that the back button should use refetch.

[tylersayshi]

this was related as well: #791

[dai-shi]

this was related as well: #791

@tylersayshi So, what do you think? What should be shouldScroll for popstate?

[hi-ogawa]

If I remember correctly, popstate transition / rerender needs to happen synchronously for browser native scroll restoration to properly work. This means back button should avoid refetch and render cached pages (unless, for example, server function revalidated). Some popstate specific quirks/expectations are discussed in https://bsky.app/profile/brophdawg11.com/post/3m5uadwmpas27, reactwg/async-react#5 and I also plan to create simple demo on rsc plugin vitejs/vite-plugin-react#939

[tylersayshi]

while trying this just now it seems like the scroll positioning does not work with shouldScroll: true or false. I believe this was working when it was first implemented, but cannot remember exactly.

We need something for scroll restoration to work correctly as @hi-ogawa is saying here too. Currently, the behavior is just broken.

[mx1700]

while trying this just now it seems like the scroll positioning does not work with shouldScroll: true or false. I believe this was working when it was first implemented, but cannot remember exactly.

Modifying shouldScroll: false does not work, this issue is more complex than I understand

[mx1700]

I achieved the desired effect through some modifications, but to be honest, I am not sure about the principle behind it and whether it is applicable to other browsers

waku/packages/waku/src/router/client.tsx

Lines 895 to 897 in a685fc1

	const route = parseRoute(new URL(window.location.href));
	changeRoute(route, { shouldScroll: true }).catch((err) => {
	console.log('Error while navigating back:', err);

shouldScroll: true -> shouldScroll: false

waku/packages/waku/src/router/client.tsx

Lines 870 to 873 in a685fc1

	startTransitionFn(() => {
	if (options.shouldScroll) {
	handleScroll();
	}

handleScroll(); -> setTimeout(handleScroll, 1);

I guess handleScroll occurred before the browser recorded the scrolling position, causing the previous page to always record the top position

[dai-shi]

Thanks for investigating.
The first one seems good, but the second one smells. I don't think setTimeout would be the right solution. If it's too early, maybe useEffect? But useEffect also smells.

Hope @tylersayshi or @rmarscher has some thoughts.

And, I too hope to tackle it eventually, but I wonder if @mx1700 could create e2e tests for the expected behavior.

[mx1700]

I think my guess is correct. I found the code, in router.push

waku/packages/waku/src/router/client.tsx

Lines 162 to 172 in a685fc1

	await changeRoute(parseRoute(url), {
	shouldScroll: options?.scroll ?? newPath,
	});
	if (window.location.pathname === currentPath) {
	window.history.pushState(
	{
	...window.history.state,
	waku_new_path: newPath,
	},
	'',
	url,

The changeRoute is called first, which will scroll to the top and then window.History.pushState will be used to record the position
The scrolling operation should be executed after the pushState in order to record the position properly

I have very little front-end knowledge and have never written e2e tests. Let me give it a try

[dai-shi]

btw, #1399 might be related.

[rmarscher]

btw, #1399 might be related.

And #1459