phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.21k stars 930 forks source link

Back button scroll position is buggy #2685

Open bdtomlin opened 1 year ago

bdtomlin commented 1 year ago

Environment

Actual behavior

I modified todo_trek to make it easy to see the behavior by surrounding each log entry in a <.link navigate={...}> tag and a dummy log entry "show" page: https://github.com/bdtomlin/todo_trek/tree/back-button-issue

  1. When I click on one of the log entries while still on page 1 and then use the back button everything works as expected and the scroll position is maintained exactly.
  2. When I scroll to page 2, click on a log entry, then use the back button the page is maintained but not the correct scroll position.
  3. When I scroll to any page 3 or beyond, click on the log entry, then navigate back it always puts me on page 2.

Expected behavior

I expected any time I click a link and then the back button that the page and scroll position would both be correctly maintained.

enewbury commented 1 year ago

I imagine this happens because when we navigate "back" to the timeline view, it initially only loads the 1st page of entries, and the page is shorter than the saved scroll position. So we essentially just scroll to the bottom of the first page. Then because we are at the bottom of the page, this triggers the 2nd page to load, however at that point, the attempt to scroll is done and it stays where it is.

It might be kind of naive, but I suppose we could check if page height is less than the scroll position we are trying to move to, then we register another scroll attempt after a delay, but that seems pretty shaky and depends heavily on the implementation of the application logic that loads the rest of the page. For instance, what if you had a website with a button to "load more" manually.

I kind of feel like this is more of a concern with the application, rather than the framework. For instance, if I were implementing this kind of functionality in an application, I might save "pages=2" in the url or session or something, which would instruct the server on initial render of the view to populate the correct number of pages on first load, rather than load 1 -> scroll -> load 2 -> scroll.

For what it's worth, I think remembering scroll position with "infinite scroll" is also a problem/concern for the other JS based libraries out there.

I don't know, @chrismccord do you have any thoughts?

bdtomlin commented 1 year ago

@enewbury @chrismccord

hmmm, due to the way it “almost” worked I thought it was intentionally designed to handle these infinite scroll scenarios and the back button, so I thought this was a bug.

If live view doesn’t plan on handling this case I would think you would want to have the back button just put you at the top of page 1 which is how this would work normally and leave it to the developer to come up with a solution.

The way it works now just appears to be broken. Maybe it’s just an artifact of how live view handles the back button in general and it is what it is. No biggie, I just thought it was a bug.

I haven’t had a chance to dig into the live view js to really figure out how things are working, then again I don’t really want to as that’s part of the point of live view 😀

SteffenDE commented 9 months ago

In my opinion this is indeed something that should be implemented in the application. @enewbury already gave some good points. The current page should probably be stored in the URL to load the correct page when navigating back. Furthermore the position of the topmost item could be stored and restored using the functionality provided via https://github.com/phoenixframework/phoenix_live_view/pull/3051.