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

Scroll position is lost on backwards navigation #896

Closed
aaliddell opened this issue Jan 14, 2021 · 12 comments
Closed

Scroll position is lost on backwards navigation #896

aaliddell opened this issue Jan 14, 2021 · 12 comments
Assignees
Labels
enhancement Optimization, improvement or maintenance task released Available in the stable release

Comments

@aaliddell
Copy link

When returning to a previous page, such as when leaving a calendar album with the browser back button, the scroll position of the previous page is lost and you are returned to the top of the search results. On pages with large numbers of sub-items, this then means scrolling down the page again and waiting for the lazy loading each time until you reach the scroll point you were previously at.

e.g Open 'Calandar' page, scroll down to the bottom, wait for last lazy loaded set to load and open the last item. Then try to navigate backwards to the previous page. You will be at the top of the calendar items rather than at the bottom.

This is particularly noticable when trying to look through photos from a long time back in the calendar month folders. Each time I go backwards to look at the next/previous month, I have to scroll all the way back from 2021 again.

The expected behaviour would be to store the scroll position for the previous pages (perhaps only N last pages), such that on backwards navigation we see the result item that we clicked to enter the sub-page.

Sorry if this is already an issue, I did search beforehand but found nothing.

Thanks 👍

Version: 210111-cc05c430-Linux-x86_64
Browser: Firefox 84 Desktop and Safari iOS
Env: Docker

@alexislefebvre
Copy link
Contributor

alexislefebvre commented Jan 14, 2021

What keys or buttons do you press to navigate backwards? Do you lose the scroll position when you close an image by using the cross button on top left (or top right, it depends if you opened the details or image view)?

@aaliddell
Copy link
Author

This is using the browser back button, the back key on a mouse or swiping backwards on iOS. This only occurs when entering and exiting search subsets, so opening and closing a single photo works ok.

Attached is an example of the issue on your demo site on mobile, although this is not a mobile specific issue. I start on the calendar page, scroll to the bottom and enter July 2002. I then use the Safari back button and the scroll position is now at the top of the list, not where I was prior to entering July 2002.

RPReplay_Final1610657605.mp4

@alexislefebvre
Copy link
Contributor

Sorry, I didn't tested from the Calendar view as you wrote. I can reproduce the issue on Firefox (desktop).

Thanks for the video.

@lastzero
Copy link
Member

Guess the issue here is lazy loading... so when you go back, the album is not visible anymore as we only load the fist X items initially. Also we don't know what album was selected when you go back to the page anyway. It's definitely a UX issue (not a bug), but I'm not sure how to improve this yet without adding tons of complexity.

@lastzero
Copy link
Member

A potential solution is to cache the results. Then, they may be outdated and the memory footprint of our app rises - which may cause other UX issues.

@alexislefebvre
Copy link
Contributor

Workaround: from the Calendar view, open a month in a new tab/window.

@aaliddell
Copy link
Author

aaliddell commented Jan 15, 2021

I can't comment on the internals of how you've implemented lazy loading, so please bear that in mind with the below: I may be missing complications of the local implementation.

One way of approaching this could be to store the offset of the clicked element (or the centrally visible element) when entering a sub-item (e.g I clicked item 1000 on the page). This could be stored in the history stack, since history.pushState allows storing arbitrary data or otherwise you could store the position/block in the URL fragment. Then, on a backwards navigation, you receive the state back and get the offset. Then rather than loading the first block, you instead load the block that contains the clicked element (e.g 100 items per lazy load block, so for element 1000 you'd load block 10). This would however require that photoprism support lazy loading on both up and down directions, since now if a user scrolls up from this non-first block it would need to lazy load the earlier blocks the same way it currently lazy loads blocks on downward scrolling.

Where this would break down is if new things are imported/created such that the offset no longer points to the clicked item, but I think that's a fair tradeoff and isn't likely to drift substantially (e.g. the clicked item will usually still be in the same block or one of its close neighbours).

If you are using Vue Router, there is a page on scroll behaviour: https://router.vuejs.org/guide/advanced/scroll-behavior.html. This would only be the foundations for implementing the above though.

Incidentally, having support for both up and down lazy loading would help #500, since you could unload earlier blocks as a user scrolls down, then reload them if a user scrolls back up. This means you only have a 'window' of blocks actively loaded at a time and that window slides up or down as the user scrolls. Blocks that fall outside the window are unloaded and replaced with spacers (to keep scrollbar happy) and therefore stop taking resources on the user's browser.

@lastzero
Copy link
Member

lastzero commented Jan 15, 2021

That's what I mean with complexity :D We'd rather start with batch edit or face recognition than getting into such optimizations at this point in time - also given that we only reached 4% of our funding goal so far. Unless that makes you become a sponsor?

@lastzero
Copy link
Member

Lazy loading a constant list of items would work like that, but all items may be changed or deleted at any time. So simply loading and hiding blocks won't work unless you're ok with bugs.

@aaliddell
Copy link
Author

That's what I mean with complexity :D We'd rather start with batch edit or face recognition than getting into such optimizations at this point in time - also given that we only reached 4% of our funding goal so far.

Ah gotcha 👍 . I'd agree those are more important things from a user satisfaction side.

Unless that makes you become a sponsor?

Possibly, I'm still at the point of evaluating PhotoPrism, but it's looking like it'll be the 'chosen one' and let me ditch iCloud. Either way though, I wouldn't expect it be to the level that'd justify making this a priority.

Lazy loading a constant list of items would work like that, but all items may be changed or deleted at any time. So simply loading and hiding blocks won't work unless you're ok with bugs.

Yep, pagination is somewhat 'unsolvable' for dynamic lists (without transaction snapshotting at the database level for every active user's view 🙄). Depending how you do the cursors, you either end up missing or repeating items. Generally systems just have to accept one of these, but it would only show up when active updates are occurring to the data store, which I would consider 'acceptable'.

@lastzero
Copy link
Member

This should mostly fix it for albums & labels. Won't load all previous results for photos / videos as that may be thousands, and we open them on the same page in a lightbox (for that very reason).

@lastzero lastzero self-assigned this Jan 16, 2021
@lastzero lastzero added enhancement Optimization, improvement or maintenance task please-test Ready for acceptance test labels Jan 16, 2021
@aaliddell
Copy link
Author

Awesome! I've just tested a build on the develop branch and it appears to work as intended. I only checked against a minimal synthetic library rather than giving it access to my full one, but I can't see why that'd change the result.

I think that solves the issue at hand here. Thank you!

@lastzero lastzero removed the please-test Ready for acceptance test label Jan 19, 2021
@lastzero lastzero added the released Available in the stable release label Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Optimization, improvement or maintenance task released Available in the stable release
Projects
Status: Released 🌈
Development

No branches or pull requests

3 participants