Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Totally confused on scroll behaviours #690

Closed
moreofmorris opened this issue Jan 15, 2015 · 37 comments
Closed

Totally confused on scroll behaviours #690

moreofmorris opened this issue Jan 15, 2015 · 37 comments

Comments

@moreofmorris
Copy link

Hey there.

I can tell by looking through the issues that scrolling and React Router seems have a lot of confusion around it. I honestly have gone through the issues and even done searches through the source code to work this out.

I have my routes set up like this:

var routes = (
  <Route name="app" path="/" handler={MainLayout} />
    <Route name="favourites" handler={FavouritesPage} />
    <Route name="profile" path="profile/:id" handler={ProfilePage} />
    <Route name="detail" path="detail/:id" handler={DetailPage} />
    <NotFoundRoute handler={NotFoundPage}/>
  </Route>
);

Router.run(routes, function (Handler, state) {
  React.render(<Handler params={state.params}/>, document.getElementById('mount'));
});

Take the ProfilePage for example, this ajax loads data for lots of sub components on it's page. Sometimes they can make the page very long vertically. Those sub components can also link to other ProfilePage routes and so on a profile it could have a URL like /#/profile/123 and on that it could also link to another profile page like /#/profile/999. If I click a sub component of a profile page that is down the list, I want it to just load the new profile page and have it scroll back to the top, so it feels like a whole new page has loaded. At the moment it seems to preserve the scroll position (as best it can) which is mostly down the list or around where I clicked the link for the other profile, which is very annoying. I don't want it to do that, I just want it to go back up to the top like it's loaded a brand new profile.

I've tried everything to try and make this work. I seem to see a lot about adding ignoreScrollBehavior on the routes. I've done this, but it doesn't fix the problem. I'm really, really stumped. Can someone help please? It would be really appreciated!

Great work so far on this project by the way, it's been very useful. Just this problem is the thing that I'm stumped with.

@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2015

This is what should happen by default. Can you repro this?
I have a sample project using react-router and can't reproduce this problem.

@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2015

You can set a breakpoint here and see whether you hit it:
https://github.com/rackt/react-router/blob/master/modules/behaviors/ImitateBrowserBehavior.js#L10

Any change of something like bad Chrome plugins overriding window.scrollTo? Any errors in console?

@moreofmorris
Copy link
Author

It's a little difficult to share the source as it's an internal prototype for my work.

I think I know what the problem is... It seems that because I have my structure like this:

-----------------------------------------
| #mount                                |
| ------------------------------------- |
| | #topbar                           | |
| ------------------------------------- |
| ------------ ------------------------ |
| | #sidebar | | #content             | |
| |          | |                      | |
| |          | |                      | |
| |          | |                      | |
| |          | |                      | |
| |          | |                      | |
| |          | |                      | |
| |          | |                      | |
| ------------ ------------------------ |
| ------------------------------------- |
| | #footer                           | |
| ------------------------------------- |
-----------------------------------------

The router is setup so the <RouterHandler /> resides in the #content area. It's in the content area that holds the main content (which can be quite long vertically) for the app. Everything around it is fixed in place, the content scrolls but is within it's boundaries (the scroll bar doesn't overlap the #topbar or the #footer).

Looking deeper in the code, it makes sense as the default behavior is to window.scrollTo(0,0) but that wouldn't work in my case because it's the #content that needs it's scrollTop to be set back to 0.

Does that seem right? But also, this sort of layout doesn't seem too unusual for a web app? I don't want the scroll bars of the #content to overlap the #topbar or the #footer and so I want #content to be contained, in the whole app only the #content has a vertical scroll bar. When a route changes, the layout never changes, just the contents in the #content area.

For me then, I'd need to override the scroll behavior when I first create the router? Or is there some other tricks React Router has that would help with this? I have tried this:

var router = Router.create({
  scrollBehavior: {
    updateScrollPosition: function () {
      document.getElementById('content').scrollTop = 0;
    }
  },
  routes: routes
});

But it doesn't work 100% all the time. It doesn't always go to the top when the URLs change. Would really appreciate any help on this.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2015

Right, that makes sense..
If you use a container with overflow: scroll we can't just window.scrollTo.

But also, this sort of layout doesn't seem too unusual for a web app? I don't want the scroll bars of the #content to overlap the #topbar or the #footer and so I want #content to be contained, in the whole app only the #content has a vertical scroll bar.

I see. We actually had a similar layout at one point in our webapp. I'm glad we don't use it anymore because we had all sorts of issues with this (e.g. you can't click Safari top bar on iPhone to scroll to top—precisely because window.scrollTo doesn't do anything).

A way around this would be to give #topbar and #footer position: fixed and add corrsponding margins to the content. But you said you don't want scrollbars to overlap #topbar and #footer. I'd say you should reconsider this because it doesn't look bad unless your topbar or footer are really large:

screen shot 2015-01-16 at 16 34 42
(Medium does this)

screen shot 2015-01-16 at 16 35 06
(Twitter does this)

If you still want to do like you said, yes, you should probably specify custom scrollBehavior.

First, remove any ignoreScrollBehavior options on your routes.
Then, try changing your scrollBehavior#updateScrollPosition method to be like this:

  updateScrollPosition: function (position, actionType) {
    function scrollTo(x, y) {
      var contentEl = document.getElementById('content');
      if (contentEl) {
        contentEl.scrollLeft = x;
        contentEl.scrollTop = y;
      } else {
        window.scrollTo(x, y);
      }
    }

    switch (actionType) {
      case LocationActions.PUSH:
      case LocationActions.REPLACE:
        scrollTo(0, 0);
        break;
      case LocationActions.POP:
        if (position) {
          scrollTo(position.x, position.y);
        } else {
          scrollTo(0, 0);
        }
        break;
    }
  }

(I took code from default ImitateBrowserBehavior.js and tweaked it a little bit.)

Does this help? This is similar to what you wrote but I just want to make sure before trying to diagnose further. If it doesn't work, can you please write specific steps to reproduce the problem?

@moreofmorris
Copy link
Author

Wow, thank you. That's great, you went above and beyond with that reply.

It's great that you've setup React Router in such a way to allow easy customisation and thank you for the examples from Medium and Twitter. This was pretty much my last bit to crack when it came to React Router and I can see now that there isn't really a generic way this could be handled as it would depend on the particular layout.

Is it insane that another attribute could be introduced called something like "scrollToTop" that you could pass to a particular route which could do the same thing as your code? Meaning I wouldn't have to "hard code" the content id into a custom scroll behaviour? I totally understand that it's mostly a better idea to avoid layouts with overflow: scroll but I could also other use cases, particularly in more complex interfaces in the future. Just curious really, maybe you tried that before?

Thanks again. Great reply.

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2015

Is it insane that another attribute could be introduced called something like "scrollToTop" that you could pass to a particular route which could do the same thing as your code?

The problem is we can't know which particular div is scrollable (and whether your layout is intended to be scrollable via a vertical div or document body). Even if we find a div with overflow: scroll, maybe it's actually part of the content, and the actual outer container has small height at the time. We also can't use particular route's root div because your layout may be slightly different (e.g. an inner div two or three levels deep might be the actual container).

Since we can't do that for generic case, we solve this by allowing you to plug completely custom behavior with scrollBehavior.

BTW, just to clarify, the code I gave you is based on ImitateBrowserBehavior. It's not exactly “scroll to top” because it will attempt to restore previous position on Back key. If you need strictly “scroll to top”, take a look at ScrollToTopBehavior.

@moreofmorris
Copy link
Author

Hey.

So I made the layout changes you advised. There is no funky inner scroll, the main content is just on the body now. So I've changed my code to be this:

var router = Router.create({
  scrollBehavior: {
    updateScrollPosition: function () {
      window.scrollTo(0, 0);
    }
  },
  routes: routes
});

router.run(function (Handler) {
  React.render(<Handler/>, document.getElementById('mount'));
});

Because the default behaviour didn't seem to work. But I'm still having this problem. When I'm loading content (so the <RouteHandler/> changes) it does scroll to the top of the window, but then maybe half a second later it'll go back down the page to some random position.

So I have a route like this /tracks/123 and on that page is a track, plus other tracks to go to. I click on one of those other tracks and the route goes to /tracks/999, the page scrolls to the top but maybe half a second later it scrolls down. It seems to scroll down to where my AJAX loaded track sub components are, or thereabouts. Those sub component track could take a while to load and re-render, would that affect the scrolling?

I'm stumped :(

@gaearon
Copy link
Contributor

gaearon commented Jan 21, 2015

Because the default behaviour didn't seem to work.

Let's start with this.
What exactly didn't work with the default scroll behavior?

Edit: it would help if you could share your code (or parts of it) so I could run them.

@moreofmorris
Copy link
Author

Hey there.

The default scroll behavior does just what I described in my message. And with my custom one above it does the same. Maybe I'm just not understanding what the browser is doing? The window does scroll to the top, but after half a second or so, goes back down the page. I was trying to find a pattern... It's possibly trying to return to a point on the page it remembers per URL? ...if I return to the same URL, it tries to go back to that particular scroll point? Or is it to do with the sub components loading (as their states are being loaded from an API).

I'm doing nothing else when it comes to scrolling anywhere else in the app. Maybe I'm just not understanding what the browser is trying to do. I'm coming from a Backbone world, so maybe I'm having a hard time working it out.

I can't really share this particular code base as it's for a prototype for work.

@agundermann
Copy link
Contributor

Does this happen after clicking a link, or browser back/forward? Which browser?

@moreofmorris
Copy link
Author

On browser click.

The best example is one I explained. It has a track/1234 route. On that page are other track sub components that have been loaded from an API and are further down the page. I click on one of the other tracks and so the route then changes to track/5678 and it'll go to the top, but then half a second or so later, it'll take me back down the page. I'm trying hard to understand if there is a pattern, but can't seem to find one.

@kilometers
Copy link

Which browser?

Obvious question but have you already tried several browsers?

@gaearon
Copy link
Contributor

gaearon commented Jan 21, 2015

I click on one of the other tracks and so the route then changes to track/5678 and it'll go to the top, but then half a second or so later, it'll take me back down the page. I'm trying hard to understand if there is a pattern, but can't seem to find one.

This seems to be the problem because you can reproduce it even with a custom scroll behavior. AFAIK router only sets the scroll position in scroll behavior, so if the problem persists, it likely isn't with the router.

Since you can't share the code, if I were you, I'd try:

  • A few other browsers—is this a browser issue?
  • Replace subcomponents on the problematic page with <Link>s and remove anything else—is the problem component-specific? I don't know about your setup but things that come to mind are jQuery plugins, components doing this.getDOMNode().scrollIntoView() or something like that.

Finally, are you using HashLocation or HistoryLocation?

@moreofmorris
Copy link
Author

Interesting, even trying it in Safari... It doesn't do it. Seems to only be Chrome. (can't test easily in Firefox, I have -webkit- prefixes all over the place)

Note as well, this only happens when I'm returning back to the same route (or with the same ) so a track/123 and from there clicking on another track/456. Default scroll behaviour seems to work fine when the changes with route a different route. I'm using HashLocation too, if that helps?

Definitely no jQuery plugins or this.getDOMNode().scrollIntoView(), the components and sub components are very dumb and just update with props. The associated store does load AJAX calls though.

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

can't test easily in Firefox, I have -webkit- prefixes all over the place

You should use Autoprefixer :-)

I'm using HashLocation too, if that helps?

Can you try HistoryLocation? Just to make sure it's not related to #707.

@moreofmorris
Copy link
Author

Totally on the Autoprefixer, but it's just a prototype and so only needs Chrome really ;)

Hmm, HistoryLocation doesn't seem to do too much. A pattern seems to be emerging though. It's like it's remembering where the scroll position was on a certain route. So if I'm on track/123, scroll down a bit, click on track/456 it goes to the top fine, but if I scroll back down and reselect track/123 it'll go back down to where it thinks it was previously on track/123... I think. Does that sound normal?

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

Lol I think I know what this is, but this sounds I like I made the stupidest mistake possible when writing this. :-P

Can you verify whether this change fixes your issue?

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

Or maybe it's not enough..

I think we're recording every push and store positions by route, but this will bite us as soon as you go A -> B -> A without ever going back.

We should probably keep a stack instead.

@moreofmorris
Copy link
Author

Hey there, no didn't do anything there.

Have I hit an edge case? But it seems like a legit thing that an app might do, right?

Thanks so much for your patience with this.

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

Thanks for sticking with us. I was just thinking aloud.

Do I understand correctly that this is your issue?

  1. You open /a
  2. You scroll down to the middle
  3. You follow a link to /b
  4. You follow a link to /a and instead of opening at the top, it opens in the middle

@moreofmorris
Copy link
Author

So... with the default scroll behavior (nothing custom or funny)

My app has a range of routes.
/profile
/about
/track/123

Going to any of these by following <Link/>s works fine and goes to the top. But on the /track/123 route, that particular page has a selection of other tracks to choose from. These are lower down the page and are loaded from an API. If I click on a sub track on the main track and keep clicking through to another track and another track, that's where this weird scrolling behavior starts to happen, it doesn't always go to the top.

So it's more like following these /track/123 to /track/234 to /track/345 etc.

Hope that makes sense!

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

Oh well. This may be a Webkit issue I bumped into, but it's not really related to the router. Is there any chance that at the moment you're transitioning to the next track, inertial (rubber-band) scrolling is overflowing?

(not overflowing:)

screen shot 2015-01-22 at 19 25 07

(overflowing:)

screen shot 2015-01-22 at 19 25 15

You can check by waiting for rubber band effect to finish before clicking on the link to next track.

@moreofmorris
Copy link
Author

Definitely nothing to do with the rubber band effect. I know what you mean there, but even if I wait for a long time before clicking on another link, it has this weird behavior :(

@gaearon
Copy link
Contributor

gaearon commented Jan 22, 2015

If I click on a sub track on the main track and keep clicking through to another track and another track, that's where this weird scrolling behavior starts to happen, it doesn't always go to the top.

Do you ever click on a track you've been on before?

Is this

/tracks/1
/tracks/2
/tracks/3
/tracks/1

kind of situation?

If this is the case, can you reproduce the problem without going to a route you've been on before?

@moreofmorris
Copy link
Author

Yes, the pattern seems to be when I go to a route I've been before, it's like it's attempting to return me back to where that route was before. That's fine for a back button action, but not when you're trying to click a link and it simply be at the top. It's confusing that it scrolls back down. For the back button though, that sort of behavior would make sense.

@gaearon
Copy link
Contributor

gaearon commented Jan 24, 2015

Makes sense. This is how router currently works, but I agree it needs fixing.
I'll give this more thought on Monday and will try to come up with a solution.

Thank you for helping me understand the issue!

@agundermann
Copy link
Contributor

@gaearon Would you mind explaining how this happens? As far as I can tell, the router itself never scrolls to a recorded position for actions other than POP. And even that would happen only with ImitateBrowserBehavior, which he tried to replace.

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

@taurose is right, I must have been high when I wrote this.

Default scrolling behavior does not use stored position when handling PUSH. Therefore this has nothing to do with #707. Even if we mistakingly pass the wrong action, your custom scrolling behavior is not even using saved position and you still have this issue.

There is no other place in router that calls scrollTo, so we're left with two possibilities:

  • Browser tries to restore browser position by itself and we're not aware of the case when it happens;
  • Something in your code that calls scrollTo (which you say isn't the case).

Unfortunately, I can't really help with first possibility without a reproducible example.

Here's a sample app (source) that uses HashLocation (in GH pages build) with default scrolling behavior, also loads data via AJAX, and going from gaearon to gaearon/flowcheck-loader to gaearon again does not mess up scroll position.

I'm closing this for now, but feel free to reopen if you have ideas about how we can reproduce this.
Thank you for the conversation, and I'm sorry I couldn't help you.

@gaearon gaearon closed this as completed Jan 26, 2015
@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2015

One thing you could check is put an alert inside your custom scroll's behavior and verify it is being called every time route changes.

If it is not being called for the transitions where scroll positions gets messed up, make sure that:

  • It is params and not just query that changes (changes in query don't cause scroll update by design);
  • You don't have ignoreScrollBehavior anywhere in route's ancestors or route itself.

Please feel free to reopen if you find that custom scroll behavior's method is not being called when it should!

@moreofmorris
Copy link
Author

Thank you for all your help.

I still can't work out what is going on. I have made my own function which takes me back up to the top if the id of the route has changed. I'm happy enough with this for the moment, can't work out what else to do.

@gaearon
Copy link
Contributor

gaearon commented Jan 30, 2015

I guess the easiest way is to take everything out of your components until you find what is causing the issue.

@ntucker
Copy link

ntucker commented Mar 13, 2015

I had this problem when I was moving from the save route, but changing the query parameters. It would go to the top any time I changed something else, but query parameters it seemed to think weren't different.

@gaearon
Copy link
Contributor

gaearon commented Mar 13, 2015

@ntucker

That's how it's expected to work, unless I misunderstood your comment.

Query parameters don't change scroll position.
Route parameters will change scroll position unless you slap ignoreScrollBehavior on the route.

@ntucker
Copy link

ntucker commented Mar 13, 2015

How do GET parameters not mean a new page? Is there an option to configure it to scroll to the top? I use GET parameters for pagination, and I expect when a user clicks another page that it will scroll to the top as the entire contents of the page changes.

@gaearon
Copy link
Contributor

gaearon commented Mar 14, 2015

Is there an option to configure it to scroll to the top?

As I wrote above, you can either promote page number to be in params and it will work by default, or you can scroll to top manually in componentWillReceiveProps. It's not difficult:

componentWillReceiveProps(nextProps) {
   // assuming you pass query in props from parent route handlers
   if (this.props.query.page !== nextProps.query.page) {
     window.scrollTo(0, 0);
   }
}

How do GET parameters not mean a new page?

Not all query parameters are the same. Consider a search page. You wouldn't want page to scroll every time you type a letter, but search terms likely go into the query. Similar for "filter" options on page with filters and sorting. So you can't say "scroll to top" is desirable for every use case with query parameters. And as I wrote above you have easy ways out.

@ntucker
Copy link

ntucker commented Mar 14, 2015

Forgive me, my browser did not find any other results for componentWillReceiveProps on this page.

However, it seems like a bad user experience to have every letter they type show up in their browser history. They'll but hitting the back button forever!

My situation is actually a search engine! Every time they do a new search, the entire page loads with new data, just like any other normal history-adding event. So they need to see all that lovely new data! If I scroll to the top every time this changes though, the users will be very upset when they hit the back button and the page isn't where it was when they went forward! This is the great thing about react-router, because it keeps track of those things, so I don't have to.

@gaearon
Copy link
Contributor

gaearon commented Mar 14, 2015

it seems like a bad user experience to have every letter they type show up in their browser history. They'll but hitting the back button forever!

You can use replaceWith so there is just one history entry while the user is typing. Of course this would only make sense if you show results immediately as user types. If you have a Search button, it makes sense to use transitionTo and only create history entries when user presses a button.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants