Disable automatic scrolling when clicks are used to navigate

[mjackson]

Navigation by clicking on a link shouldn't take you back to the last scroll position you were at.

[gaearon]

This isn't confined to <Link>s btw, I have the same issue with my custom link component that uses transitionTo.

[mjackson]

Yeah. I want to try and nail down all the places where we're auto-scrolling and shouldn't be, and fix them all at once.

[gaearon]

I think we should confine auto-scrolling to popstate-initiated transitions.

[mjackson]

Agree.

From my experiments in Chrome, it looks like using the forward button preserves scroll state as well. Something we want to support?

[gaearon]

I assumed Forward is also popstate (not sure though, haven't worked with history API directly for a long time). Definitely I'm for supporting Forward restoring.

[gaearon]

I don't mean to be impatient, but we have scheduled a release with react-router Friday next week, and this is currently a breaking issue.

Can you please indicate if this will be in the works by Monday? If not, I'm happy to do it myself and submit a PR—just don't want to duplicate the work if someone's already doing it.

Thanks!

[mjackson]

@gaearon I'd love it if you could make a PR to fix this. It really shouldn't take too much work. All actions go through the LocationActions module. If I were to fix this myself, I'd probably create a separate type of action in the dispatcher (maybe called HISTORY_ACTION or STATE_ACTION or something) and then just branch on that type inside the PathStore. Anyway, that's just one possible fix I've been toying around with. Feel free to patch it however you'd like.

Thank you!

[gaearon]

Thanks, I was thinking about it similarly. Will do.

[ferrannp]

Hello, I think I have an issue related to this. Let's see... In Firefox, when I scroll the page and I press 'F5' to refresh, I see that the scroll bar is in the same place. However, in Chrome, when i press F5, the scroll bar is on the top BUT when I touched it (or just touch the wheel of my mouse) the scroll bar just jumps into the position it was before refresh. I am assuming that the router is causing that.

I also noticed that changing preserveScrollPosition to true or false, doesn't make any difference.

[gaearon]

@Difort This looks more like Chrome trying to restore scrolling position on its own, no?
(I mean, if preserveScrollPosition makes no difference, it's probably a browser issue.)

[ferrannp]

Can be, because if I set preserveScrollPosition to true/false, then if I go to another page and go back, works with the expected behavior. So the F5 is a browser stuff... And Firefox is handle it fine for me but not Chrome. Maybe the router could save the srollPosition into sessionStorage in order to restore it even after F5? (could be an optional option).

[gaearon]

@mjackson I have a few questions, can we maybe have a little chat on IRC?

[gaearon]

@mjackson

I'm confused by the docs.
They say:

preserveScrollPosition

If true, the router will not scroll the window up globally when any route is transitioned to. Defaults to false. When false, the <Route/> gets to decide whether or not to scroll on transition.

The “will not scroll window up globally” wording is misleading isn't it? Because, when set to false (or not set), it doesn't just “scroll window up globally”. It tries to restore previous position (tries to act smart, but wording makes it sound like a simple scroll-to-top behavior).

All this time I was under impression that this flag does the precise opposite. I though “preserve” means “preserve previous position on that page” and not the apparent “preserve current(?) position” that it means.

[gaearon]

Do I (now) understand correctly that preserveScrollPosition={true} is “let browser do its thing” and false is “try to remember and restore positions ourselves”?

[ryanflorence]

Maybe the default is documented wrong. I personally don't want to mess with the scroll position by default until we get it right, but I think it does by default.

You can do <Routes preserverScrollPosition={false}/> and then manage it yourself until we get it right.

[ryanflorence]

Oh, its super confusing now actually. We need a new name like emulateBrowserScrollPosition.

Originally we only meant to scroll it up. So preserveScrollPosition meant that when you switch routes, the window would just stay scrolled at its current position, which is usually not what you want.

Now that we actually try to "preserve" the position when using the back/forward buttons, the word "preserve" is completely ambiguous.

I hope that makes sense.

I'll change the name soon, no time for code today though :(

Also, @mjackson, until our scrolling antics are rock solid, I would prefer you have to opt-in to it, rather than opt-out.

[gaearon]

@rpflorence Thanks! I'll chatting with @mjackson now about that, will post later.

[mjackson]

So, had a chat with @gaearon. The highlights are (@gaearon please feel free to add to this):

• preserveScrollPosition is a terrible name. Nobody (even the person who created it, me) can ever remember what it means. Change it to imitateBrowserScrolling (we can discuss other names). It should default to true
• *Location objects should be refactored to trigger LocationActions instead of an internal onChange callback. In Flux terminology, these are "action creators". This lets us know what caused the change in location (i.e. user action, popstate, etc.)
• Refactor the portions of PathStore that deal with scrolling into a new ScrollStore. ScrollStore waits for PathStore to update before updating the scroll position. It also discerns between various action "sources" to know when to update the scroll position and when to leave it alone
• Remove the awkward UPDATE_SCROLL event from LocationActions

Now, the only question left in my mind is where to put the imitateBrowserScrolling setting... per-<Route(s)> feels unnecessary. I could be wrong here, but would love to hear some use cases first (i.e. a particular route where you want to disable auto-scrolling but you want it enabled in the rest of your app, or vice versa).

Can't we just use a top-level Router.disableScrolling() method to turn it off?

[ryanflorence]

preserveScrollPosition is a terrible name. Nobody (even the person who created it, me) can ever remember what it means.

We used to only scroll up, so if you didn't want us scrolling up, you wanted to preserve the scroll position. It got confusing once we started getting fancy and preserving the scroll position (just read that a few times, you'll remember).

imitateBrowserScrolling

I would vote for emulateBrowserScrollPosition, but I'm okay with what you've proposed too.

Now, the only question left in my mind is where to put the imitateBrowserScrolling

Exactly where it is now. <Routes/> defines global behavior, <Route/> can override.

[mjackson]

Exactly where it is now.

Use case?

I'm asking because leaving it where it is now doesn't make sense to me and makes things difficult implementation-wise.

[gaearon]

I like top-level method, as it kinda mirrors React's initializeTouchEvents.
It's a global setting, enabling optional behavior, that doesn't win anything in being declarative IMO.

[mjackson]

I agree with @gaearon

[gaearon]

Could we make scroll behavior implemented as pluggable strategies? Just like React batching strategy can be injected at runtime.

For example, we may have ScrollToTopStrategy as the default, and offer DoNothingStrategy and RememberScrollPositionStrategy as options. If somebody wishes to implement their own, let them do it without having to listen to browser events themselves.

This may also be closer to @rpflorence's “stupid defaults” (as ScrollToTopStrategry seems simple enough) without sacrificing behavior completely. One needs only a Google query to find out about RememberScrollPositionStrategy.

[mjackson]

@gaearon I like that idea. It's also similar to what we currently do with the *Location objects.

[ryanflorence]

I like all of this

[gaearon]

@rpflorence You're still in favor of per-Route overriding aren't you? I admit that might be useful e.g. if you have an image gallery on some page, but want other pages to scroll to top.

However I'm not sure current code handles per-Route overriding correctly. (Please amend me if I'm wrong.)

Say, I have /galleries route with gallery list and /gallery/:id/image/:index that represents a single gallery. In single gallery, we can press left/right to switch images.

In this case, we want to avoid scrolling to top when switching between gallery images. However, when switching to a gallery image the first time after gallery list route, we do want to scroll to top. In other words, per-Route override should only be used when we're transitioning between two routes that are both described by that Route.

It may be that router currently works that way but my impression from the code was that it only looks on next route, and not the previous one.

My suggestion is following:

1. Make it prop scrollStrategy=none|scrollToTop|imitateBrowser, default is scrollToTop. Can be applied to Route or Routes.
2. Allow per-Route overrides but only choose them when both previous and current match that route.
3. When target and source routes don't match, use scrollStrategy of their closest common ancestor.
4. Allow to inject other strategies with Router.injectScrollStrategy(name, strategy)

Scroll strategy should receive all actions, as well as current and previous route.

Does this make sense?

[gaearon]

In other words, in order to properly support route nesting and scroll strategy overrides, when we transition from route A to route B we need to:

1. Find closest common ancestor that has scrollStrategy explicitly specified
2. If none found, we will bubble up to Routes, for which we have a default
3. The search is inclusive, so if route A is the same as route B, and this route specifies its own scrollStrategy, use it

I believe this should be enough to cover 90% of use cases and when you want extra flexibility, you can always inject your own strategy.

[ryanflorence]

Yeah, all in or all out seems better. So either config on Routes or Router.initiateScrollEmulation (or whatever we call it)

Sent from my iPhone

On Sep 28, 2014, at 2:20 AM, Dan Abramov [email protected] wrote:

@rpflorence You're still in favor of per-Route overriding aren't you? I admit that might be useful e.g. if you have an image gallery on some page, but want other pages to scroll to top.

However I'm not sure current code handles per-Route overriding correctly. (Please amend me if I'm wrong.)

Say, I have /galleries route with gallery list and /gallery/:id/image/:index that represents a single gallery. In single gallery, we can press left/right to switch images.

In this case, we want to avoid scrolling to top when switching between gallery images. However, when switching to a gallery image the first time after gallery list route, we do want to scroll to top. In other words, per-Route override should only be used when we're transitioning between two routes that are both described by that Route.

It may be that router currently works that way but my impression from the code was that it only looks on next route, and not the previous one.

My suggestion is following:

Make it prop scrollStrategy=none|scrollToTop|imitateBrowser, default is scrollToTop. Can be applied to Route or Routes.
Allow per-Route overrides but only choose them when both previous and current match that route.
When target and source routes don't match, use scrollStrategy of their closest common ancestor.
Allow to inject other strategies with Router.injectScrollStrategy(name, strategy)
Scroll strategy should receive all actions, as well as current and previous route.

Does this make sense?

—
Reply to this email directly or view it on GitHub.

[gaearon]

I'm a bit confused, do you mean you'd rather get rid of Route-specific overrides? I think they can be useful but I'm not sure they're worth the additional complexity.

[ryanflorence]

Confirm. I don't think route overrides have simple use cases. It depends on the transition. I'd say just have one, non-overridable config.

Sent from my iPhone

On Sep 28, 2014, at 8:35 AM, Dan Abramov [email protected] wrote:

I'm a bit confused, do you mean you'd rather get rid of Route-specific overrides? I think they can be useful but I'm not sure they're worth the additional complexity.

—
Reply to this email directly or view it on GitHub.

[gaearon]

Good. So you agree that we can have a few builtin strategies and if you want to get fancy you can implement your own?

What would the API for a strategy be? It should be aware of all transitions, it should also know whether they're initiated by user or browser. It should be able to specify scrollY (which is its primary function).

So the "strategy" is an object that can handle location actions (PUSH, POP, etc) and has getScrollY() method. It's pretty much a pluggable scroll store. Does that work for you?

[ryanflorence]

I'd say getScroll that returns {x, y}. Not all apps only scroll vertically.

I like the scroll strategy idea a lot.

Sent from my iPhone

On Sep 28, 2014, at 8:52 AM, Dan Abramov [email protected] wrote:

Good. So you agree that can have a few builtin strategies and if you want to get fancy you can implement your own.

What would the API for a strategy be? It should be aware of all transitions, it should also know whether they're initiated by user or browser. It should be able to specify scrollY (which is its primary function).

So the "strategy" is an object that can handle location actions (PUSH, POP, etc) and has getScrollY() method. It's pretty much a pluggable scroll store. Does that work for you?

—
Reply to this email directly or view it on GitHub.

[gaearon]

All right.
I'll try to flesh this out in a few days:

1. Change Locations to fire actions so we know what was caused by browser and what by user
2. Move scroll handling to ScrollStrategies (DoNothing, ImitateBrowser, ScrollToTop)
3. Fix ImitateBrowser to only consider popstate-initiated events
4. Replace prop with Router.setScrollingStrategy(strategy)
5. Update docs and examples

I have very tight deadlines (need to ship a product rewrite by Friday) so I only have till Tuesday evening to fix this. In case I won't make it, I'll post my work in progress (likely to not include docs), and @mjackson kindly agreed to take over in this case.

Please let me know if you have any naming, backward compatibility or API preferences.

[gaearon]

The only thing that bothers me API-wise is that we have different mechanisms (prop vs method) for very similar things (setting location vs scrolling strategy). With scrolling strategy pluggability is a must (so a string prop won't cut it), but with location, you're unlikely to ever need to customize it (and it needs to know about Dispatcher anyway).

Are we okay with similar things achieved by different means in API?

[ryanflorence]

make it happen, we can bike shed later. we're not afraid of api changes right now, the UPGRADE_GUIDE is working great for people.

[gaearon]

OK, that's exactly what I was worried about! No problem then.
Also just realized location accepts a string as well as an object—I'll make it the same way with scrollStrategy then.

[ryanflorence]

yeah, string for built-in stuff for convenience.

On Sun, Sep 28, 2014 at 9:55 AM, Dan Abramov [email protected]
wrote:

OK, that's exactly what I was worried about! No problem then.
Also just realized location accepts a string as well as an object—I'll
make it the same way with scrollStrategy then.

—
Reply to this email directly or view it on GitHub
#252 (comment).

[gaearon]

I've hacked together a fix!
It lacks docs, but I tested it both with history and hash and it seems to work.

#326