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

Back button scroll position is buggy #2685

Open
bdtomlin opened this issue Jun 12, 2023 · 3 comments
Open

Back button scroll position is buggy #2685

bdtomlin opened this issue Jun 12, 2023 · 3 comments

Comments

@bdtomlin
Copy link

Environment

  • Elixir version (elixir -v): 1.14.5
  • Phoenix version (mix deps): 1.7.2
  • Phoenix LiveView version (mix deps): 0.19.2
  • Operating system: macOS Ventura 13.4
  • Browsers you attempted to reproduce this bug on (the more the merrier): Arc (chrome), Safari, Chrome, Firefox (all latest versions)
  • Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

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
Copy link
Contributor

enewbury commented Jul 18, 2023

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
Copy link
Author

@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
Copy link
Collaborator

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 #3051.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants