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

Fix scrollpos after returning from download in iOS pwa mode #2782

Merged

Conversation

heikomat
Copy link
Contributor

  • This PR improves upon the bugfix from UX: Restore scroll position when navigating back #2648
    • This improved fix now instantly puts the user at the correct scrollPos without any unnecessary reloads
  • It also fixes all the related bugs found by @graciousgrey here
    • It does so by implementing a general solution to the underlying ios-pwa-scrollpos-restore-after-download-bug that all the mentioned bugs share

As it turns out, iOS in PWA mode uses something called bfcache (backward-forward-cache) to cache the state of the whole webpage before navigating to the fullscreen-download-page, and restores the whole page from that cache after returning to the website.

The regular scrollpos-restore-mechanism found in app.js just doesn't support that bfcache-scenario. To fix this, i made the regular scrollpos-restore-mechanism compatible with bfcache scnearios by

  • detecting them using pageshow and pagehide events
  • remembering the current scrollpos when the page is about to be cached
  • providing the old scrollpos to the restore-mechanism upon page-restore-from-cache.

To make it fully work i also skipped resetting lastFilter in the $route-watcher of album-photos and search-view. That way the search-method detects that the filter is unchanged and skips the search, thus not resetting the scrollpos to 0,0. i'm not 100% sure why the lastFilter was always being reset on every $route change before and hope that that doesn't break anything.

I expect the potential for new bugs caused by these fixes to otherweise be very small, because other than ios-pwa-fullscreen-downloads, there is barely any scenario where pageshow or pagehide are triggered, especially with event.persisted === true.

Acceptance Criteria:

  • Features and enhancements are fully implemented so that they can be released at any time without additional work
  • Automated unit and/or acceptance tests have been added to ensure the changes work as expected and to reduce repetitive manual work
  • User interface changes are fully responsive and have been tested on all major browsers and various devices
  • Database-related changes are compatible with SQLite and MariaDB
  • Translations have been / will be updated (specify if needed)
  • Documentation has been / will be updated (specify if needed)
  • Contributor License Agreement (CLA) has been signed

@heikomat heikomat changed the title Bug/ios pwa scrollpos after download Fix scrollpos after returning from download in iOS pwa mode Oct 14, 2022
@lastzero
Copy link
Member

lastzero commented Oct 15, 2022

The reason $route() currently resets this.lastFilter and other component variables is that VueJS automatically reuses an existing component instance when the next route uses the same type. So, for example, if you navigate from /archive to /browse or /review.

Otherwise, you might see pictures that require a review when you navigate back or forward to /browse. I'm not 100% sure without testing if lastFilter is an exception here that doesn't necessarily need to be reset, but in general it is required.

Note that with my next commit, I will change the frontend routes to be prefixed with /library in order to implement the following requests:

@heikomat
Copy link
Contributor Author

@lastzero then maybe a solution could be to only reset lastFilter when the routeName changes. Do you think that could work?

@lastzero
Copy link
Member

Might work!? Current implementation was meant as a workaround until we find a better solution. You could force the router to always reload everything, but that's hurting performance.

@heikomat
Copy link
Contributor Author

heikomat commented Oct 16, 2022

Will Test it later today, by first checking if the issues you mention happen for me when not resetting lastFilter at all, and then testing if these issues dissaperar when only resetting on routName-change.

If that works for both cases (the issues you mentioned and the returning from fullscreen-Download) then we have a winner

@lastzero
Copy link
Member

I think it is used to figure out if the cached results need to be replaced (otherwise they can just be displayed or an offset can be used to retrieve more results from the server). Unfortunately I have no idea how this would work in combination with Apple's caching.

@lastzero
Copy link
Member

Make sure you also check the raw request data and not just what is displayed in the UI, as I recently added server-side logic to prevent content in review from being displayed by accident and tampering. This is part of our security enhancements for multi-user support.

…t use the same components but with different settings
@heikomat
Copy link
Contributor Author

heikomat commented Oct 16, 2022

@lastzero your hint with why the lastFilter was reset was completely right, and it looks like my idea with the routeName works:

scenario switching between /browse and /search returning from cached browser-state
current develop: lastFilter is reset on every $route-change ❌ a search is triggerd, resetting the scroll-pos
local test: lastFilter is never reset ❌ search results don't change, because no search is triggered
this PR until 5 minutes ago: lastFilter is only reset if this.filter changes ❌ search results don't change, because no search is triggered
this PR now: lastFilter is only reset if routeName changes

I still need to check wether the raw-request data is fine, but i have a guess that they probably are.
The only thing that changed is when the prevent Search-Api-call because filter is unchanged-early-exit triggers. if the search is not prevented, then it is executed with the same parameters as before this PR.

As for the cached-and-restored state of the website: If i understood it correctly, from the websites point of view, it is 100% as if the user had never left. The whole in-memory-state of the website gets cached and restored as if nothing happened at all. That is also why components don't rerender upon return

@lastzero
Copy link
Member

Excellent work, as always! We should test it again, but it sounds plausible that it can work this way, since the configuration of the page component depends mainly on the route, e.g. which static search filters to use.

Dynamic filters and other settings, on the other hand, should be restored as they were by the URL query parameters.

There are two special cases to keep in mind:

  1. the navigation contains links to "pages" that are actually just the regular search, but with additional search filters to serve as an example, e.g. Monochrome
  2. settings like the view type are also stored in localStorage to remember the last used value

Let me know when you want this to be merged! 👍

@heikomat
Copy link
Contributor Author

Excellent work, as always! We should test it again, but it sounds plausible that it can work this way, since the configuration of the page component depends mainly on the route, e.g. which static search filters to use.

Dynamic filters and other settings, on the other hand, should be restored as they were by the URL query parameters.

There are two special cases to keep in mind:

1. the navigation contains links to "pages" that are actually just the regular search, but with additional search filters to serve as an example, e.g. [Monochrome](https://demo.photoprism.app/browse?q=mono%3Atrue%20quality%3A3%20photo%3Atrue)

2. settings like the view type are also stored in localStorage to remember the last used value

Let me know when you want this to be merged! 👍

just tested these special cases, and it looks like they work just fine. But that makes sense!
lastFilter is now only reset on route change. and clicking on monochrome doesn't change the route, but it DOES change the filter, so a new search is triggered (because this.filter is now !== this.lastFilter).

Thinking about it in a different way: if the route is unchanged, and the filter is unchanged, then the results should probably also not change!

  • If the route changes, the filter is reset -> new search
  • if the filter changes it is !== lastFilter -> new seach

Do you know of any cases where a new search needs to be triggered upon $route-change even though the routeName and filter are unchanged?

@heikomat
Copy link
Contributor Author

@lastzero From my point of view this can be merged, but maybe we should wait for feedback from @graciousgrey. She was the one who found the original bugs this fixes, and if i recall correctly, she wanted to test these changes anyway.

@lastzero
Copy link
Member

Sorry for the conflicts! 🤣

@heikomat
Copy link
Contributor Author

Can't look at them right now. I'll quickly resolve the conflicts once I'm at my laptop

@lastzero
Copy link
Member

Renamed the pages folder to page for consistency.

@heikomat
Copy link
Contributor Author

strange that github showed conflicts there. i was able to merge cleanly, because git detected that the files were just renamed

@lastzero
Copy link
Member

Thanks! I'll merge this now so we can test it on our demo and the temporary :unstable image we are currently using internally until a new development preview is available (it should be more stable than we can guarantee at the moment).

@lastzero lastzero merged commit 4da33d4 into photoprism:develop Oct 18, 2022
@lastzero lastzero added please-test Ready for acceptance test ux Impacts User Experience merged Changes should be tested again after they have been integrated labels Oct 18, 2022
@graciousgrey
Copy link
Member

Just tested all cases again on different devices/browsers/pwa. Works perfect ❤️

@lastzero lastzero added released Available in the stable release and removed please-test Ready for acceptance test labels Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged Changes should be tested again after they have been integrated released Available in the stable release ux Impacts User Experience
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

3 participants