Fix scroll related issues

[stefanprobst]

Attempt to solve some scroll related issues:

• remove hash-link scroll handling from <Link /> and put it in shouldUpdateScroll. Also, let scroll-behavior handle scrolling to element ids. This should fix Fix handling of hash links & scrolling #7017, Gatsby's Link overrides scrolling behavior with anchors #7450, Back button from anchor link does not scroll to previous location #8019.
• make the shouldUpdateScroll browser API more flexible by passing the whole routerProps object, and allowing access to the saved scroll positions. This allows things like delaying scroll updates until after a page transition animation has finished, should fix [using-page-transitions] Scrolls to top before animating #7921. Maybe this one is a little late for v2 and should be separated into another PR.

[pieh]

I think that if we leave pathname here for backward compatibility, this change would potentially could go in

[stefanprobst]

Not sure why location is still needed in <Link />, but tests fail without it, Will have a look tomorrow.

[stefanprobst]

This should be good to go, let me know if there's anything else missing from my side.

[tomsseisums]

You should update the docs @ https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/api-browser-docs.js#L56

[tomsseisums]

The naming here doesn't match the actual behavior. You don't get positions, you only get the storage.

But getting the storage here, implies you have to know how it works.

AFAIK, it isn't documented as it wasn't really a public API up until now. And when exposed to public, it should get documented, at least an example for how one might use it in shouldUpdateScroll...

[stefanprobst]

Yes, will update the docs with an example. Maybe it's enough to expose getSavedScrollPosition: this._stateStorage.read?

[tomsseisums]

That'd seem to be better, yeah.

[tomsseisums]

I'm scared about history being passed here, same as I'm not a fan of location.replace/location.reload.

I wouldn't expose functionality that could be abused outside the normal loop of things potentially causing unexpected results.

[stefanprobst]

Updated the docs, plucked off location from routerProps, and read() from the scroll position session store.
Still to do: docs should say what the return value of shouldUpdateScroll means, and maybe also that the positions are internally stored in SessionStorage, so survive a reload.
Question: Passing on read() works because of this, which is marked as a "hack". Is it okay to rely on that?

[tomsseisums]

Updated the docs, plucked off location from routerProps, and read() from the scroll position session store.

history, not location. 🙂

[stefanprobst]

Help! Could anybody give me some hints how to turn this into proper jsdoc, thanks!

/**
 * Allow a plugin to decide if the scroll position should update or
 * not on a route change.
 * 
 * @callback getSavedScrollPosition
 * @param {object} location A location object.
 * @param {string} location.pathname The path of the route.
 * @returns {Array|null} An [x, y] array of coordinates, or `null` if nothing was found.
 *
 * @function shouldUpdateScroll
 * @param {object} $0
 * @param {object} $0.prevRouterProps The previous state of the router before the route change.
 * @param {object} $0.routerProps The current state of the router.
 * @param {string} $0.pathname The new pathname (for backwards compatibility with v1).
 * @param {getSavedScrollPosition} $0.getSavedScrollPosition Takes a location and returns the coordinates of
 * the last scroll position for that location, or `null`. Gatsby saves scroll positions for each route
 * in `SessionStorage`, so they are available after page reload.
 * @returns {boolean|string|Array} 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.
 * @example
 * exports.shouldUpdateScroll = ({
 *   routerProps: { location },
 *   getSavedScrollPosition
 * }) => {
 *   const currentPosition = getSavedScrollPosition(location)
 *   const queriedPosition = getSavedScrollPosition({ pathname: '/random' })
 *
 *   window.scrollTo(...(currentPosition || [0, 0]))
 *
 *   return false
 * }
 */
exports.shouldUpdateScroll = true

[tomsseisums]

@stefanprobst what exactly is the problem over there?

[stefanprobst]

@joltmode Neither @callback nor the @returns show up (nothing in React DevTools). This is all very probably because I'm doing this wrong, but could also be something to do with documentationjs or the transformer gatsby uses.

Unfortunately I could not find an example in the gatsby docs to emulate - the closest seems onCreateWebpackConfig.getConfig, which incidentally is also broken.

So my question is how to document getSavedScrollPosition and the return values correctly?

[stefanprobst]

Updated the docs. Should be good now, apart from the fact that the return values don't show on the website, but that's a bug in gatsby-transformer-documentationjs (does not work for other documented APIs either) which should be fixed by #8208.

[pieh]

Thanks @stefanprobst! Finally got chance to get back to this PR

[stefanprobst]

Great, thanks!