[WIP-PoC] [added] useScrollToTopBehavior

[taion]

Don't merge this as-is (obviously, since there are no tests or documentation and it's probably not even in the right project).

This is a PoC for #2469 demonstrating a reasonable way to add scroll behavior management without introducing any new concepts.

I was planning on adding another implementation that used an abstract route, but this just seems overall better.

This should really live in rackt/history instead of here, but the relevant (no longer used) DOMUtils functions were in this repo, so it seemed clearer to add it here.

[taion]

The idea is trivially that you just use this as another history enhancer, e.g. useScrollToTopBehavior(createBrowserHistory)().

[timdorr]

This could be viable as a PR (and not a throwaway) if you also add one for ImitateBrowserBehavior.

[taion]

I started with that, but it didn't seem necessary - at least locally, I see browser-style behavior maintained when I go back/forward between history entries. The only missing thing is actually scrolling to top on new history entries.

[taion]

I guess we should rename this useStandardScrollBehavior and actually add a useScrollToTopBehavior that always scrolls to top.

That said, what do you think of this API of using it as a history enhancer? This is actually not directly relevant for 1.0.0 if we drop it History anyway (beyond cutting the relevant DOM utils out of this repo).

[timdorr]

What about with a hash history? I'm not familiar with that as much as pushState. Does it maintain scroll position when navigating?

[taion]

Yes, it works with hash history.

[agundermann]

I started with that, but it didn't seem necessary - at least locally, I see browser-style behavior maintained when I go back/forward between history entries. The only missing thing is actually scrolling to top on new history entries.

Some problems I recall with that:

• Firefox restores scroll position before popstate (ie. before we can render), which causes issues when the referring page is smaller than the target scroll position
• Async rendering could be problematic, since browsers behave so differently and native scroll events can't be cancelled (though this will hopefully change with scroll restoration API)

That said, I think only scrolling to top like that is a decent approach. I don't think it's possible to fix the issues I mentioned, and I never quite liked the 0.13 implementation from a technical point of view, though based on the lack of complaints it seemed to do well.

We should probably consider #2020 though.

[taion]

@taurose welp: #2470

[timdorr]

All the more reason to remove #2469 from the 1.0.0 milestone and wait until post-1.0.0 to implement. This isn't a simple fix and requires some nuanced logic. I'd rather not try to fight against that and hold up a bunch of other great features for what is ultimately UX enhancement.

[taion]

Let me update this to match the v0.13 implementations so we have a comparison point. I see the glitchiness on Firefox. I still think we can handle this correctly on our side.

#2020 will take some additional thought but I don't think our choice of API is going to meaningfully limit us there. The tradeoff really just is that the router feels a little incomplete without scroll management.

[taion]

Oh my goodness, I did not read #707 (comment) carefully enough. This is more difficult than I thought.

[taion]

@taurose I believe the implementation I just added works for all the cases you've outlined in #707 on both Chrome and Firefox for both browser and hash history. Predictably, it's now significantly more complicated, and I'm correspondingly less convinced that I want to add this - but I believe it does work.

[agundermann]

It doesn't work with async transition hooks though, does it?

[taion]

@taurose Oh - I guess we'd need a little more logic there - essentially on e.g. popState I'd need to undo the browser's automatic scroll change from the listenBefore hook. I think this approach can definitely extend to handle that, though.

[agundermann]

on e.g. popState I'd need to undo the browser's automatic scroll change from the listenBefore hook

I kinda gave up on getting that to work smoothly (ie without any unnecessary scrolls visible to the user) in all browsers I tested back then (Chrome, IE, Firefox).

[taion]

Actually, on second thought, maybe you don't do that? In practice an async transition with a remembered scroll position is going to be a POP, which means that it should complete almost instantly, since in most cases it shouldn't actually require extra network requests to fetch more routes/components/data... hopefully.

[gaearon]

The only missing thing is actually scrolling to top on new history entries.

This is actually the important part. 😉

[taion]

My statement above was wrong, though - restoring scroll position doesn't quite work properly on Firefox when doing a POP to a page where you were scrolled past the length of the current page.

It's not perfect, but would we be comfortable with having merging https://github.com/rackt/react-router/pull/2471/files#diff-ddf5b5adac06d19993f464b68e4034cc or something comparable as a blocker for 1.0.0-final?

Another option is the much simpler taion@d7bdd87 which works on Chrome and half-works on Firefox for the "standard" behavior, to the extent that we care more about scrolling to the origin on a new page and less about always restoring the old scroll position.

cc @mjackson @ryanflorence

[taion]

@darreng Regarding #2469 (comment), are you using the version from taion@47a6329? Which OS/browser?

[LDMFD]

Exactly - taion/react-router@47a6329.

OS X 10.10.5, Chrome 46.0.2490.80.

Unable to repro in Firefox and Safari.

[taion]

Hmm. I'm not seeing that behavior. ~the same setup but I'm on Chrome 48.

[LDMFD]

Can you send or share with me somehow, your test-case?

[taion]

I just took the animations example in the repo and repeated the content of the Page1 and Page2 components several times.

[timdorr]

For Chrome, I suggest running the tests in incognito or a fresh profile. Extensions can tend to mess with things and cause test variability.

[taion]

Per @ryanflorence I think the idea will be to leave this in a separate library for the time being until we've stabilized it enough to merge into either React Router or History proper.

[timdorr]

@taion You should put up a repo in @rackt when you've got a 0.1-ish version ready to go. It signals it's something we're seriously thinking about and may get merged back in here.

[DanielSundberg]

So the conversation has moved elsewhere? Could someone link the new repo here when it is created?

[timdorr]

We'll link up here and #2469 when @taion has a home for it. Should be in @rackt soon. Worry not!

[timdorr]

As promised: https://github.com/rackt/scroll-behavior