getSavedElementScrollTarget and getSavedWindowScrollTarget methods

[alexhisen]

getSavedElementScrollTarget and getSavedWindowScrollTarget methods.
Also now tests for no scroll restore on push (default browser behavior).

[alexhisen]

Any progress?

[alexhisen]

hello?

[taion]

I understand roughly what you're trying to accomplish here. I don't think this is the best way to do it.

I would instead modify react-router-scroll to take a custom factory function for creating a ScrollBehavior object, then supplying your own that will instead instantiate a subclass of ScrollBehavior.

Within the context of that subclass, I would e.g. override _getDefaultScrollTarget to perhaps return some special sentinel value, then override _getScrollTarget to call super._getScrollTarget(...), then translate that sentinel to false, and trigger whatever updates are needed in your system to notify the nested components that they should update the scroll position. (Or perhaps support e.g. returning false from _scrollToTarget to tell scroll-behavior to stop trying to update the window scroll position.)

This would also address the original use case of secretly exposing the state storage bit on this from shouldUpdateScroll, and give a more "proper" API for doing something like you've proposed with controlling the default scroll position.

What do you think? Note also that, as we support scrolling to anchors, if your unknown height still corresponds to an anchor, then you might not need custom code to handle that scrolling.

[taion]

Broadly, I understand what you're trying to do, and it is reasonable. The reason the state storage object is on context at all was to allow some way of setting some different default scroll position.

That said, though, instead of continuing to build on this somewhat clunky approach, I'd rather we expose setting the default scroll position as a first-class concept. Having shouldUpdateScroll read the saved scroll position seems non-ideal.

I guess alternatively we could pass the saved scroll position into shouldUpdateScroll as one more argument at the end.

[alexhisen]

The problem is timing. shouldUpdateScroll could fire earlier than the component mount that knows where it needs to scroll to by default. Sometimes this also is async as the component first needs to obtain some data, so it doesn't even happen on mount, it happens later. So that component is responsible for restoring scrolling, therefore it needs to be able to obtain the saved position. We have like 3 different use cases all leveraging this single method, so I don't think there is a cleaner approach than exposing somewhere a method that returns the stored position.

But perhaps that somewhere can be what's returned from useScroll or something like that.

[alexhisen]

So, what do you think?

[taion]

I'm still not really comfortable with this API, because it makes pretty hard assumptions about how the data are stored, and that they're necessarily retrievable in this form. That was sort of why I specifically added that backdoor access to the storage that I believe you're currently using.

Do your use cases all fall under "scroll to a default location if there is no saved scroll location, but use the saved scroll location otherwise"? The async stuff is a little odd, but that seems like a general enough thing that we should support directly, with an appropriate high-level API.

One way I can think of this working is that, if we add the saved scroll position as an argument to shouldUpdateScroll:

• Most users ignore it
• Users who need to scroll to a position that they can determine synchronously from the context can just return that from shouldUpdateScroll
• If there are async concerns, users can e.g. dispatch an action to the Redux store (or equivalent with e.g. Relay or MobX) signifying that the scroll needs to be updated later – then once the async callback fires, it can handle the scroll behavior

What do you think?

[alexhisen]

You know one thing we also have to do that's a hack now is to call this._saveWindowPosition(); in shouldUpdateScroll() whenever we return false in it:
// This hack is necessary because if manual restoration scrolls to 0,0 but we are already there,
// no scroll event will fire and the position won't be saved, so if you then navigate elsewhere
// and come back, it'll restore not to the last state the user saw at 0,0 but to the state
// that was saved previously when shouldUpdateScroll returned true.

[alexhisen]

Having the saved position as an argument will only address one of our use cases, which is a work-around for incorrect handling of direction:rtl in some browsers. In Chrome, I think, scrollLeft=0 on a direction:rtl element is its very end, not its very beginning. :-( If you just do nothing, and not restore, browser will default to right-most position for rtl elements but we have to know if there is a saved position or not that we should allow to be restored.

async is our second use case for this API right now. Because we need to be able to defer the scroll restoration and without support for this in react-router-scroll, we basically have to do it ourselves with window.scrollTo(...this.context.scrollBehavior.scrollBehavior.getSavedWindowScrollTarget()).

There is a whole fork of react-router-scroll that adds async support - https://github.com/dlmr/react-router-scroll-async - perhaps it's worthwhile to try to merge it in, though I haven't really looked at it in detail.

I also worry about how clean it would be to have components who know about the deferral somehow coordinate with a single global shouldUpdateScroll().

The third use case is we have multiple synchronized horizontally scrolling elements. Scrolling one, scrolls all of them in sync (using https://github.com/Filip-rspective/react-scroll-sync). However, some of these elements may not be shown/mounted during user scroll. So when the user then chooses to show one such element, upon mount in its shouldUpdateScroll, we need to return not the last saved position of THIS element but the current position of the other synced scrollables. To do this, we have a concept of a masterScrollKey, so there is one sync-scrolled element that's always visible and it's considered the master and so in the shouldUpdateScroll() we return this.getSavedElementScrollTarget(masterScrollKey). I suppose if we could use the same scrollKey on multiple ScrollContainers, which is currently not possible, we may not need to do this.

Bottom line is this simple API method enables so many different situations that solving all of them within the library itself will take a fair amount of effort and will probably make it overly complex. In my opinion it is better to officially document the method and the format in which the data is returned and make the scrollBehavior object where this method is found more easily accessible than via the context hack.

[taion]

Okay, given that it's the combination of all of those, I agree that this seems worthwhile. I'll take a look at fixing CI and getting this merged over the weekend.

[taion]

That said, would it be useful to pass in the saved position to shouldUpdateScroll?

[alexhisen]

It certainly seems logical for shouldUpdateScroll to be given what is intended to be used if true is returned.

[taion]

I haven't forgotten about this – will try to get this in tomorrow.

[taion]

Sorry to go back on this. What do you think of #124? Does this satisfy your needs?

I'm still not fully comfortable with exposing this as a "real" part of the API, but I think this extracted method both cleans up the existing code and gives you a more convenient way to access the data you need, while ensuring that it's the same code we use to calculate the actual scroll target.

Alternatively, taion/react-router-scroll#86 adds support for the use of a custom scroll behavior class with react-router-scroll, so you could properly expose the method yourself.

I don't intend on breaking it, and I respect your use cases, but I'm just not entirely comfortable adding this to the public API for everyone, at least unless other people ask for it too, and I think this provides a path forward to you that's better than what you have now.

[alexhisen]

Seeing how I already have to use a private API (_saveWindowPosition), I guess it's fine for this API to also be private.

[alexhisen]

Can you please publish a new version of react-router-scroll that uses an updated scroll behavior version, so I don't have to have it in my direct dependencies.

[taion]

I'm going to wait a couple of days to hear back on #112. Assuming there are no issues with the exposed APIs, I'll release both changes at the same time.

[taion]

Released in scroll-behavior v0.9.5 + react-router-scroll v0.4.4.