learningequality / kolibri

Kolibri Learning Platform: the offline app for universal education
https://learningequality.org/kolibri/
MIT License
808 stars 683 forks source link

Use useScrollPosition in Vue Router init #9101

Closed nucleogenesis closed 2 years ago

nucleogenesis commented 2 years ago

Context

The useScrollPosition module provides everything we need to provide the default expected behavior of saving the user's scroll position between navigations within the same plugin. It also provides a tool for ensuring that it isn't saved for pages on an ad hoc basis when wanted for any reason.

Previously this behavior lived in and was managed by CoreBase. This behavior, however, seems much better suited for use within Vue Router.

Currently, this is done at the definition of the core Vue Router in the Router#initRouter method by setting the options.scrollBehavior depending on the value of a destination route's params.scrollTo value. We can use the reactive values from useScrollPosition's public API to ensure that on every navigation, we set the scrollPosition to that which is saved for the current route.

Related: #7322 - this allowed setting the params.scrollTo to a document selector and this was then converted to the data structure expected by Vue Router { selector: string, offset: {x,y} }

Acceptance Criteria

Not to do

nucleogenesis commented 2 years ago

(cc @marcellamaki @rtibbles @MisRob @sairina)

I've dived into this further and wish I'd gotten as robust an understanding of this scrollPosition business when I initially reviewed CoreBase and hadn't assumed things worked as they suggested - however, this didn't really work in the first place :face_with_spiral_eyes: .

The useScrollPosition module - and the CoreBase logic it is based on - make sense on sight - they track scroll positions against a key, but there is a critical problem where it uses window.history.state.key to track the scrollPosition of a page between route navigations.

window.history.state.key is always just an ever increasing number that appears to just be the number of ms since the page loaded. I'd assumed it was a unique ID for whatever part of the history stack a route was on, and maybe it was when this was originally implemented, but this isn't the case (at least in Chrome). For example:

The assumption has been that going back to Route A would inherently have the same key as it is the same route - but that's not how it works.


When clicking the browser's back button, however, scroll position is saved. I tested this in IE11 and Chrome with and without JS disabled on a few websites.

One solution then maybe to consider refactoring some of our navigation links to instead use window.history.back() rather than directly linking via Vue Router to the previous page link. For example:

If that X link is set to call window.history.back() then my scrollPosition will be saved.

If it uses KRouterLink as-is, then it will be scrolled to the top of the list when I go back.

Note that this may be complicated if/when pagination is implemented.


Another alternative is to use the existing scrollTo behavior that looks to the $route.params.scrollTo value on every navigation, and then applies any value there to the Vue Router's built-in scroll behavior.

In the case of pagination, we could also then lean on the $route.params to house that data so going back to the previous example:

If the ImmersiveToolbar's route prop is set to a route object whose params look something like the following, we can account for pagination in the future as well as ensuring our navigations maintain scroll position.

{
    scrollTo: ;#user-1234567890abcdef', // note that we can scroll to a specific DOM element OR a numeric Y position
    page: 2,
}

In this case, I think the new issue then is to identify places where we specifically want scroll position to be saved when navigating back via the UI - then to dynamically set the scroll position on the destination route's params.scrollTo. Definitely would love some feedback on this and where/how this happens might deserve some collaborative frontend-dev + design cohacking to at least build the list of places where it should be done. There could be places, of course, where this isn't desirable - such as when switching pages in paginated content (ie, if I'm on the bottom of page 2 and click "Back" I want to be at the top, not the bottom again where I was when I clicked "Next" to get from page 1 to 2).

This seems to be the best way forward IMO.


One last thing about ScrollingHeader - there is some intended logic around hiding the header on scroll but it also never ever works because of how it is coded (basically, the CoreBase data that would hide the header is precluded entirely from ever being set to true, but is defaulted to false and is set to false a few places, but never to true).

So @jtamiace - is there any need for this feature of hiding the header when scrolling down moving forward?

rtibbles commented 2 years ago

Note that the previously implemented scroll position work in CoreBase was only ever a workaround to handle the fact that our async loading caused VueRouter's native scroll position to not work, as the page would be much shorter immediately after navigation, so the previous scroll position could not be restored.

If you are not referencing the Vue Router functionality that the previous work was built on top of, then I think there has been a misstep: https://v3.router.vuejs.org/guide/advanced/scroll-behavior.html

rtibbles commented 2 years ago

I think the suggestion to use more of the state history would be helpful for our 'back arrows', although I replacing clickable links with JS fired navigation events might not be great for semantics/accessibility.

Another possibility here is to move our data loading handling into component guards, which means that the route doesn't even navigate fully until the data needed for the page has loaded. This means that we could probably stop mucking around with scroll position, and just let VueRouter handle it completely.

nucleogenesis commented 2 years ago

@rtibbles

In review - there are two scroll-behavior managing systems in Kolibri right now.

1) CoreBase

This intended to map the scrollPosition of each page between route navigations to a unique history state key but fails because there is no mapping of that key value to past history states.

2) Using $route.params.scrollTo - which is set in the Vue Router init scrollBehavior function

This also doesn't work as expected - firstly because it doesn't appear to use the correct Vue Router API (where the value should be in CSS relative spacing key values ie, { left: 0, top: 50 } - instead using X/Y coords). This also doesn't work in the string case because the selector and offset keys are not those used in the documentation.

And to put icing on that bummer cake, even when the params do match the expected API, it still doesn't perform the scroll behavior. I tried to also pass a Promise resolving after 2500ms setTimeout (per the docs) in case it was an issue with needing the page to load first but that didn't work. Flat out calling window.scrollTo works fine though.


This puts us in a pickle of having to work out a new and better solution to this whole problem.

Your suggestion to use Router guards sounds like the best way forward combined with window.scrollTo. However, we'll want to devise and document our strategy for how best to store/track this position. I think that adding to the $route.params.scrollTo is a decent strategy

Noting also that if we want the "hide the header" logic to work again, that will require some additional work to move the window position watching into ScrollingHeader - and then may want to expose a Vuex action or some variable in the useScrollPosition composable that allows us to force the header into view (CoreBase prop-drilled this into a prop on ScrollingHeader).

nucleogenesis commented 2 years ago

Closing this as the scroll position business seems to be working with the implementation as is without complaints. Can revisit if/when issues arise but my experience so far is that Kolibri's scroll position stuff works as expected in places where CoreBase has been removed.