shouldUpdateScroll returning Array not working

[std4453]

Description

In the documentation of shouldUpdateScroll it says that:

Should return either an [x, y] Array of coordinates to scroll to, a string of the id or name of an element to scroll to, false to not update the scroll position, or true for the default behavior.

However, returning an array won't actually work.

Steps to reproduce

I've dug into this for a while and I think (probably, since I'm not very familiar to gatsby) I've found the actual cause of the problem. Instead, I'll simply write down what I've found during my analysis. If I was wrong, please point me out.

It all started in root.js：

gatsby/packages/gatsby/cache-dir/root.js

Lines 68 to 72 in 205847b

	<RouteUpdates location={location}>
	<ScrollContext
	location={location}
	shouldUpdateScroll={shouldUpdateScroll}
	>

It imports shouldUpdateScroll from navigate.js:

gatsby/packages/gatsby/cache-dir/navigation.js

Lines 125 to 151 in 83926c8

	function shouldUpdateScroll(prevRouterProps, { location }) {
	const { pathname, hash } = location
	const results = apiRunner(`shouldUpdateScroll`, {
	prevRouterProps,
	// `pathname` for backwards compatibility
	pathname,
	routerProps: { location },
	getSavedScrollPosition: args => this._stateStorage.read(args),
	})
	if (results.length > 0) {
	// Use the latest registered shouldUpdateScroll result, this allows users to override plugin's configuration
	// @see https://github.com/gatsbyjs/gatsby/issues/12038
	return results[results.length - 1]
	}
	if (prevRouterProps) {
	const {
	location: { pathname: oldPathname },
	} = prevRouterProps
	if (oldPathname === pathname) {
	// Scroll to element if it exists, if it doesn't, or no hash is provided,
	// scroll to top.
	return hash ? decodeURI(hash.slice(1)) : [0, 0]
	}
	}
	return true
	}

It uses apiRunner, which I suppose (due to the lack of knowledge about the internals) should collect shouldUpdateScroll from all plugins, which, in my case, is only provided by gatsby-remark-autolink-headers.

This function is then passed to ScrollHandler, which uses it in its shouldUpdateScroll instance method:

gatsby/packages/gatsby-react-router-scroll/src/scroll-handler.tsx

Lines 91 to 102 in 40724e8

	shouldUpdateScroll = (
	prevRouterProps: LocationContext | undefined,
	routerProps: LocationContext
	): boolean => {
	const { shouldUpdateScroll } = this.props
	if (!shouldUpdateScroll) {
	return true
	}
	// Hack to allow accessing this._stateStorage.
	return shouldUpdateScroll.call(this, prevRouterProps, routerProps)
	}

So who uses this.shouldUpdateScroll? The answer is windowScroll and scrollToHash:

gatsby/packages/gatsby-react-router-scroll/src/scroll-handler.tsx

Lines 71 to 89 in 40724e8

	windowScroll = (
	position: number,
	prevProps: LocationContext | undefined
	): void => {
	if (this.shouldUpdateScroll(prevProps, this.props)) {
	window.scrollTo(0, position)
	}
	}
	scrollToHash = (
	hash: string,
	prevProps: LocationContext | undefined
	): void => {
	const node = document.getElementById(hash.substring(1))
	if (node && this.shouldUpdateScroll(prevProps, this.props)) {
	node.scrollIntoView()
	}
	}

The problem is that both method didn't actually use the return value of shouldUpdateScroll, so if the return value is an array, it is simply ignored.

Relationship to other issues

My motive is that we wanted to click on markdown headers and update the hash, and found out that sometimes, clicking on a header makes it jump to the screen top, and sometimes it doesn't. That is similar to the behaviour described in #25778.

First I thought that maybe gatsby-remark-autolink-headers got something wrong, but it didn't. The actual cause is, again, rooted within gatsby-react-router-scroll. So in componentDidUpdate:

gatsby/packages/gatsby-react-router-scroll/src/scroll-handler.tsx

Lines 56 to 69 in 40724e8

	componentDidUpdate(prevProps: LocationContext): void {
	const { hash, key } = this.props.location
	let scrollPosition
	if (key) {
	scrollPosition = this._stateStorage.read(this.props.location, key)
	}
	if (hash && scrollPosition === 0) {
	this.scrollToHash(decodeURI(hash), prevProps)
	} else {
	this.windowScroll(scrollPosition, prevProps)
	}
	}

This is supposed to be called when the path changes, and tries to restore the user's last scroll position on the new page, however it also gets called when the page hash updates, when it still tries to do the same thing.

That said, clicking on an in-page anchor does two things:

1. Updates page hash, emitting a hashchange event.
2. Scrolls (actually jumps) to the referenced element, emitting a scroll event.

If the former comes first, the router calls componentDidUpdate, which uses the last scroll position and calls window.scroll, which somehow 'cancels out' the latter one (I presume that's because the browser sends only one scroll event per frame), so in the end, nothing changes.

If the latter comes first, the page first scrolls, updating last scroll position, and componentDidUpdate calls window.scroll with the new scroll position, so the page jumps to the desired position.

That might solve the myth of #25778, although in my case another weird thing is that my browser (Chrome 84.0.4147.89) seems to stick to an order for an uncertain amount of time, and then switches to another.

The simple solution is to return false from shouldUpdateScroll when nothing but the hash changes, and let the browser do everything for you, but I'm thinking about preventing the auto-jump behavior of the browser and passing control to shouldUpdateScroll, however had yet no luck of finding a solution.

Hope that'd be of help.

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[schmalliso]

We're still seeing this issue! We'd hoped that #28555 would address it, but sadly we're still having problems with our scroll situation.

[KevinTss]

We're still seeing this issue! We'd hoped that #28555 would address it, but sadly we're still having problems with our scroll situation.

Here is my workaround (inside gatsby-browser.js):

exports.shouldUpdateScroll = ({ routerProps }) => {
  const isHash = routerProps.location.hash;
  const gatsbyWrapper = document.getElementById('gatsby-focus-wrapper');
  /* Do not scroll top if the route contain a hash */
  gatsbyWrapper && !isHash && (gatsbyWrapper.scrollTop = 0);

  return isHash && isHash.replace('#', '');
};

Hope it can help you to bypass the issue

[github-actions]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

[github-actions]

Hey again!

It’s been 60 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to comment on this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜