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

Feature request: scroll restoration #300

Open
davidje13 opened this issue Apr 11, 2023 · 13 comments
Open

Feature request: scroll restoration #300

davidje13 opened this issue Apr 11, 2023 · 13 comments
Labels

Comments

@davidje13
Copy link
Contributor

davidje13 commented Apr 11, 2023

This has been requested before: #132, (partially) #166, #189 - but the suggested code is an incomplete solution:

import { useEffect } from "react";
import { useLocation } from "wouter";

export default function ScrollToTop() {
  const [ pathname ] = useLocation();

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [pathname]);

  return null;
}

This will scroll to the top of the page on any navigation, including when the user presses the back / forward history buttons, and when the user navigates in ways that only affect part of the page (e.g. changing a tab component which records the current tab in the URL). It will also have a brief flicker due to using useEffect rather than useLayoutEffect. A more complete solution would only reset the scroll position when navigating to new pages, restoring previous positions when navigating forward / back, and allowing an override on links to disable scrolling to the top of the page.

React Router provides this as a separate component, but with some necessary integration with the core. That seems like a reasonable approach to follow here too, since it fits the philosophy of not bloating the core library (saving space for people who don't need the feature).

https://reactrouter.com/en/main/components/scroll-restoration

Their (MIT licensed) code shows that there are quite a few considerations here, so it seems beneficial to offer this in the library rather than having each user recreate the functionality for themselves. This will also make it possible to add integrations such as allowing specific links to bypass the scrolling behaviour.

Generally, their approach:

  • takes over the native scroll behaviour (i.e. when the whole page actually switches) by setting history.scrollRestoration (docs)
  • listens for the pagehide event to capture current scroll position before navigating to another page (also captures refreshing) (docs)
  • stores scroll locations in session storage (likely to avoid taking over the state parameter used in the history API)
  • provides a custom implementation of scrolling to anchor elements (likely due to disabling the browser's native handling; this might not be required, but worth finding out what the disadvantages are of allowing native scroll handling)
  • gives links / navigate the ability to bypass auto-scrolling by setting a flag
  • updates the scroll position inside a useLayoutEffect call to avoid flickering that useEffect would cause.

I think a minimal approach could:

  • listen for pagehide events to capture scroll position
  • record this position in the history API for the current page (by calling history.replaceState) - this can be put into the state object since currently it's always just set to null, which avoids the need to use session storage [edit: actually this probably won't work because by the time pagehide fires, the history has (probably?) already updated, so using session storage might be a requirement]
  • set up a useLayoutEffect with useLocation()[0] as a dep. Internally this can check history.state to see if it needs to scroll
    • it would be better if this could depend on the history.state object directly - would it be possible to expose this from useLocation somehow?
@davidje13
Copy link
Contributor Author

To clarify: I'm happy to work on a PR for this, but I want to make sure there's agreement on an approach first.

@droganov
Copy link

droganov commented Aug 24, 2023

const [ pathname ] = useLocation();

  useEffect(() => {
    window.scrollTo(0, 0);
  }, [pathname]);

is not a best option, ideally we don't restore scroll on replace event

the solution should respect history back and replace state, where we want to keep the scroll

@peterbe
Copy link

peterbe commented Jan 21, 2024

the solution should respect history back and replace state, where we want to keep the scroll

Do you know how that can be achieved?

@peterbe
Copy link

peterbe commented Jan 22, 2024

This missing feature is a game changer. I'm really eager for a solution. Even if it comes as a third party package. After all, just like the naive sample hook snippet at the top of the issue, it can all come from something/somewhere else.

In my app that I'm working on I wrote my own horrible version. Total hack. It essentially stores the previous URLs loaded, and if the 0th and the 2nd in the array of URLs is equal, I assume they user clicked the back button :) And in all other case, I trigger the window.scrollTo(0, 0).

I don't know if a third-party solution would work with wouter unless wouter exposes more.

@davidje13
Copy link
Contributor Author

I don't believe this can be done purely from a third-party plugin; it needs some level of integration with the core route handling.

You can see my original post for a full walkthrough of what would be needed for this feature, which draws from the way it is implemented by react-router. I was hoping to hear back from the maintainers about whether this would be a feature they'd consider, especially since it's likely to increase the library size at least a bit.

@peterbe
Copy link

peterbe commented Jan 22, 2024

I don't believe this can be done purely from a third-party plugin; it needs some level of integration with the core route handling.

I believe that.
Especially once you get into the finer points, as you laid out.

Apr 2023 is a pretty long time ago. :(
You mentioned,

To clarify: I'm happy to work on a PR for this, but I want to make sure there's agreement on an approach first.

Perhaps it's best to come with code then. If the maintainer(s) isn't available, I'd be willing at least to help out. I can test and and I can review. Hopefully that will make the final maintainer review easier and faster. Granted, you do make the point that " it's likely to increase the library size at least a bit."

@pReya
Copy link

pReya commented Jan 30, 2024

@molefrog What's your take on this feature? Do you consider it to be in scope? Are you open to community contributions?

@molefrog
Copy link
Owner

molefrog commented Feb 6, 2024

Hey everyone, this def sounds like an essential feature to have. I'm freezing the v2 branch as we've just rolled out the new version. I reworked the internals a bit so probably it might help. What are the extensions that the core needs?

@davidje13
Copy link
Contributor Author

@molefrog I could imagine an implementation like this in user-space:

myBrowserLocationRouter.addEventListener('beforenavigate', (e) => {
  const historyID = e.detail.oldPage?.state;
  if (historyID) {
    if (e.detail.navigateOptions.replace) {
      sessionStorage.deleteItem(`history-${historyID}`);
      return;
    }
    const pageState = sessionStorage.getItem(`history-${historyID}`);
    sessionStorage.setItem(`history-${historyID}`, {
      ...pageState,
      scrollX: window.scrollX,
      scrollY: window.scrollY,
      backScroll: !e.detail.navigateOptions.noScroll,
    });
  }
});

myBrowserLocationRouter.addEventListener('afternavigate', (e) => {
  const scroll = !e.detail.navigateOptions.noScroll;

  if (!e.detail.newPage.state) {
    const historyID = crypto.randomUUID();
    history.replaceState(historyID);
    if (scroll) {
      window.scrollTo(0, 0);
    }
    sessionStorage.setItem(`history-${historyID}`, {
      scrollX: window.scrollX,
      scrollY: window.scrollY,
      forwardScroll: scroll,
    });
  } else {
    const historyID = e.detail.newPage.state;
    const pageState = sessionStorage.getItem(`history-${historyID}`);
    if (!pageState) {
      return;
    }
    if (e.detail.type === 'back' && !pageState.backScroll) {
      return;
    }
    if (e.detail.type === 'forward' && !pageState.forwardScroll) {
      return;
    }
    window.scrollTo(pageState.scrollX, pageState.scrollY);
  }
});

Then in some component:

const [location, setLocation] = useLocation();
return (
  <div>
    <button onClick={() => setLocation('/p1')}>Page 1</button>
    <button onClick={() => setLocation('/p2')}>Page 2</button>
    <div>
      <button onClick={() => setLocation('/p1/t1', { noScroll: true })}>Tab 1</button>
      <button onClick={() => setLocation('/p1/t2', { noScroll: true })}>Tab 2</button>
    </div>
  </div>
);

With Link and Redirect also having pass-through arguments to set this second parameter of setLocation. Right now that exists for things like replace, but ideally it would be possible to set arbitrary properties, or at a minimum, possible to set this new noScroll property.


So other than being able to pass this noScroll property through the helper objects, there's not much in the main API that needs changing. The bulk of the changes are in the browser router:

beforenavigate needs to be fired whenever the location changes (using the same logic that triggers the hook to update now), as well as when the user navigates away (via the pagehide event), and ideally is not fired when the page first loads. It needs to be fired before the page re-renders (i.e. before any hooks see the new value), and needs to be given:

  • oldPage: the page we are navigating from. Ideally contains url and state from the history API
  • navigateOptions: if the navigation comes from setLocation etc., this should be the second argument which was passed. If it comes from browser events, this should be {} or null or similar.

afternavigate needs to be fired after the page has re-rendered (probably via something like Promise.resolve().then(afternavigate)), and needs to be fired when the page is first loaded (and ideally not fired when the page is closed). It needs to be given:

  • newPage: the new page we have navigated to. Ideally contains url and state from the history API
  • navigateOptions: same as beforenavigate
  • type: "back" / "forward" / "navigate" depending on the trigger for the current navigation

Note also that afternavigate might need to call replaceState, as in this example, so there needs to be some handling to avoid infinite loops.

(for simplicity I think it would be nice if both events got the same set of data)

I think adding these callbacks to the browser router represents the bulk of the core features needed to make scroll restoration possible.

@molefrog
Copy link
Owner

molefrog commented Feb 7, 2024

Link and Redirect already proxy all their props to the navigate method. So anything that is passed to the <Link foo="bar" /> will appear as an option in navigate.

Regarding the API, I'm pretty sure we can fit everything in a custom hook. This will allow some hacks to ensure that useLayoutEffect is called only once per navigation but I'm confident we can make this work.

import { useLocationWithScrollRestoration } from "wouter/scroll-restoration"

<Router hook={useLocationWithScrollRestoration}>
  <Link to="/" preserveScroll={false} />
</Router>

// implementation ideas
import { useBrowserLocation } from "wouter/use-browser-location";

export const useLocationWithScrollRestoration = (router) => {
   // all location hooks receive router object when called
   // we can use it as a singleton and store app-wide data
   // the problem is that new routers are created when they are inherited from parent
   // but I can extend the core to support something like `router.store.foo = "bar"` 

  const [path, navigate] = useBrowserLocation(router)
  useLayoutEffectOnce(() => {
     // after navigate
  })

  const navigateWithBefore = () => {
     // before navigate

    navigate()
  }
  
  return [path, navigateWithBefore]
}

This is for V3.

@davidje13
Copy link
Contributor Author

davidje13 commented Feb 8, 2024

I assume the useLayoutEffectOnce is intended to have path in its deps?

I think the potential gotcha is that (in the form I illustrated above), both events need to fire for all kinds of navigation (i.e. when using the browser back/forward buttons as well as when navigating programmatically), but in your code the "before" event will only fire when navigate is called.

I wonder when the teardown function of useLayoutEffect runs; I'll experiment to see if it works, but something like this might actually work: (or potentially this could run the teardown too "late"; after the page has already been updated with some new content and - critically - the page height has updated)

// assume beforeNavigate & afterNavigate behave as I illustrated in my earlier comment

export const useLocationWithScrollRestoration = (router) => {
  const [path, navigate] = useBrowserLocation(router)

  useLayoutEffectOnce(() => {
    if (router.state.currentNavigateOptions) {
      afterNavigate({
        newPage: { path, state: history.state },
        navigateOptions: router.state.currentNavigateOptions,
        type: "navigate",
      })
      router.state.currentNavigateOptions = null
    } else {
      afterNavigate({
        newPage: { path, state: history.state },
        navigateOptions: {},
        type: "??", // TODO: detect forward or back (necessary for correct handling of noScroll)
        // this might be possible if we carefully pick history.state values to be incrementing
      })
    }
    router.state.lastPageState = history.state

    return () => {
      // Note that here, router.state.currentNavigateOptions is from the call that
      // triggered the current teardown, NOT the same as its value above.
      // Also, lastPageState has not been updated yet, so it refers to the previous
      // page's history.state
      beforeNavigate({
        oldPage: { path, state: router.state.lastPageState },
        navigateOptions: router.state.currentNavigateOptions ?? {},
      })
    }
  }, [path])

  useEffect(() => {
    const handle = () => {
      beforeNavigate({
        oldPage: { path, state: router.state.lastPageState },
        navigateOptions: {},
      })
    }
    window.addEventListener('pagehide', handle)
    return () => window.removeEventListener('pagehide', handle)
  }, [path])

  const wrappedNavigate = (...args) => {
    // if this capturing could happen internally in the standard `navigate`,
    // we could make this whole hook independent of `useLocation`, so users
    // could just include it once (saving the need to make useLayoutEffectOnce)

    router.state.currentNavigateOptions = args[1] ?? {}
    navigate(...args)
  }
  
  return [path, navigate]
}

Note this adds some extra handling to track the state from the history API. This is needed for 2 reasons:

  1. to cope with situations where (e.g.) the user navigates from page 1 to page 2, then back to page 1 (via a link, not history), then navigates back through the history: page 1 has 2 scroll positions depending on which history entry we are at, so the path alone isn't sufficient
  2. if the user leaves the page (e.g. following an external link) then returns via the history, we can't rely on local variables - all storage needs to be in the history stack and/or session storage

@davidje13
Copy link
Contributor Author

so it turns out to be absurdly simple to get something which 99% works:

import { useLayoutEffect } from 'react';
import { useLocation } from 'wouter';
import { useBrowserLocation } from 'wouter/use-browser-location';

const hypotheticalRouterGlobalState = {};

export const interceptingHook = (router) => {
  const [path, navigate] = useBrowserLocation(router);

  const wrappedNavigate = (...args) => {
    hypotheticalRouterGlobalState.currentNavigateOptions = args[1] ?? {};
    navigate(...args);

    // ideally we'd probably clear currentNavigateOptions here (instead of in updateScroll)
    // Perhaps something like:
    // setTimeout(() => { hypotheticalRouterGlobalState.currentNavigateOptions = null; }, 0);
  };

  return [path, wrappedNavigate];
};

const updateScroll = () => {
  const options = hypotheticalRouterGlobalState.currentNavigateOptions;
  hypotheticalRouterGlobalState.currentNavigateOptions = null;
  if (!options || options.noScroll) {
    return;
  }
  const hash = document.location.hash?.substring(1);
  const target = hash ? document.getElementById(hash) : null;
  if (target) {
    target.scrollIntoView({ behavior: 'instant', block: 'start', inline: 'nearest' });
  } else {
    window.scrollTo(0, 0);
  }
};

export const ScrollRestorer = () => {
  const [path] = useLocation();
  useLayoutEffect(updateScroll, [path]);

  return null;
};

usage:

<Router hook={interceptingHook}>
  <ScrollRestorer />
  ...
</Router>

The browser's own scroll restoration is perfectly able to cope with the history navigation, just as long as we can stay out of its way and only force the scroll position when navigating ourselves (hence the hypotheticalRouterGlobalState.currentNavigateOptions tracking)

The core library could be updated in some small ways to make this nicer:

  • Capturing the currentNavigationOptions in the built-in useBrowserLocation hook (i.e. in the core library) would save the need for a custom hook entirely. Then <ScrollRestorer /> would be the only thing users need to add to enable this feature.
  • Relatedly, putting hypotheticalRouterGlobalState into the router somehow, as you hinted at above, would make for slightly nicer encapsulation.

But also this approach is not perfect. Specifically: hash/anchor handling in Links fails:

If we have a <Link href="#my-anchor"> (linking to an anchor in the current page), the currentNavigateOptions will be set, but updateScroll will not be called, because the path from useLocation doesn't change. We also don't get a hashchange event fired (because we changed it programmatically)

If we could get the current hash from the router, then it could be added as one of the deps in the layout effect, which would fix this. As far as I can tell this isn't currently available by any means, so would need to be added to the core library (either as part of useLocation, or in a new hook).

@molefrog
Copy link
Owner

Regarding the state, I haven't come up with anything more elegant than this. The plus is that it doesn't require us to modify the library:

const store = new WeakMap()

/* 
  Returns an object that can keep state associated with the current location hook 
*/
const useLocationState = () => {
  const hook = useRouter().hook;

  if (!store.has(hook)) {
    store.set(hook, {})
  }

  return store.get(hook)
} 

I don't think that storing navigation parameters should be part of the library though. This sounds like a narrow use case. We could provide a way to hook into internal events via router.events for example:

export const ScrollRestorer = () => {
  const state = useLocationState();
  const router = useRouter();

  useLayoutEffect(() => {
    const ubsub = router.events.on("beforeNav", (to, options) => {
      state.currentNavigateOptions = { to, options }
    });

    return () => unsub();
  }, [router, state])

  useLayoutEffect(() => {
    const options = state.currentNavigateOptions;

    // ... restore the scroll
  }, [path]);

  return null;
};

The obvious benefit is that this is location hook independent.
This is just a draft idea, I will keep exploring if we could easily integrate it without sacrificing the size.

noemica added a commit to noemica/cog-minder that referenced this issue Jun 26, 2024
This still has issues with overwriting the current location on forward/backward
navigation. See molefrog/wouter#300. Might still be
worthwhile to have anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants