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

Making SPA-navs-with-hashes work #237

Closed
domenic opened this issue Jun 10, 2022 · 1 comment · Fixed by #239
Closed

Making SPA-navs-with-hashes work #237

domenic opened this issue Jun 10, 2022 · 1 comment · Fixed by #239

Comments

@domenic
Copy link
Collaborator

domenic commented Jun 10, 2022

Scenario: you are on /a. You want to do a SPA navigation /b#hash, by which you mean, load the contents for /b, replace the DOM with them appropriately, and then scroll to the element with id="hash".

This needs to be handled manually by SPA routers today, because they intercept the entire navigation process. See e.g. this issue: remix-run/react-router#394. The code in, e.g., https://github.com/rafgraph/react-router-hash-link is extremely complicated, but I think the basic idea is: let the router do its normal thing, then call element.scrollIntoView() with element based on location.hash.

The navigation API in its current form does not help. It makes it easy to avoid intercepting navigations from /a to /a#hash. But navigations from /a to /b#hash will need to be intercepted with transitionWhile(), and once you do that, you lose any browser-native processing of following the fragment link.

We could fix this. I think we should fix it at the same time as fixing #231, which is also about how scrolling should be handled on push/replace navigations. #231 envisioned resetting the scroll position after the push/replace "finishes", in the sense of navigation.transition.finished, after all the DOM for /b is supposed to be loaded. Instead we should probably either reset the scroll position, or scroll to the location indicated by the hash.

The main question is how to combine our three scroll-related configurable pieces of functionality. We have:

  • Existing functionality: scrollRestoration for "traverse" navigations. (Should maybe also work for "reload".)
  • Proposed in Mismatch between scroll and focus handling #231: the ability to reset scroll position for "push" / "replace" navigations
  • Proposed here: the ability to scroll to the element indicated by the fragment, for "push" / "replace" navigations

One extreme is just scroll: "after-transition" | "manual", unifying scrollRestoration with this new option. If you want to configure scroll restoration different than scroll resetting or scrolling-to-fragment, you need to investigate navigateEvent.navigationType and/or navigateEvent.url.

Another is to keep with my argument in #231 (comment) for splitting up push/replace and traverse/reload, something like scrollRestoration for the former and initialScroll for the latter. Or maybe scrollReset is still a fine name for the latter; just like focusReset "resets" the focus to either the top, or to an explicitly-autofocused element, so scrollReset "resets" the scroll position to either the top, or to an explicitly-fragment-indicated element.

And yet another is to allow configuring scroll-to-fragment behavior differently from no-fragment pushes, so e.g. scrollRestoration + scrollReset + scrollFragmentHandling.

I'm starting to prefer just unifying them, though.

@atscott
Copy link
Contributor

atscott commented Jun 15, 2022

@domenic Thanks for creating an issue for this. I was recently investigating what we could do in the Angular router to resolve some of our scrolling issues. One of those options could be to include something that interacts with navigation API in a way that would eliminate a lot of the manual work we do for this. We have a whole RouterScroller and supporting BrowserViewportScroller that controls scrolling to the top of the page or an anchor on a "push" as well as restoring scroll position on a "traverse".

A short investigation into some other routers indicated this manual handling is something other Router authors have encountered as well:

https://sourcegraph.com/github.com/vuejs/router/-/blob/src/scrollBehavior.ts?L81-140 / https://sourcegraph.com/github.com/vuejs/router/-/blob/src/router.ts?L669-679
https://github.com/rafgraph/react-router-hash-link/blob/main/src/HashLink.jsx (remix-run/react-router#394 (comment))

While there is certainly value in some of the customizability these options provide, it would be great if there was a browser primitive that could handle the most common use-cases.

domenic added a commit that referenced this issue Jun 15, 2022
This changes the default behavior of intercepted navigations with respect to scrolling in the following ways:

* Scroll restoration is performed on reloads, not just traverses.
* For pushes and replaces, we either scroll to the top or scroll to the fragment.

This change requires some updated API surface: the scrollRestoration option to intercept() has become scroll, and the navigateEvent.restoreScroll() method has become navigateEvent.scroll().

Closes #237 by giving an easy default behavior for SPA navigations with hashes.

Closes #231 by making "push", "replace", and "reload" behave by default in an MPA-like manner with respect to scroll position, just like "traverse" does.
domenic added a commit that referenced this issue Jun 15, 2022
This changes the default behavior of intercepted navigations with respect to scrolling in the following ways:

* Scroll restoration is performed on reloads, not just traverses.
* For pushes and replaces, we either scroll to the top or scroll to the fragment.

This change requires some updated API surface: the scrollRestoration option to intercept() has become scroll, and the navigateEvent.restoreScroll() method has become navigateEvent.scroll(). The latter method now has more capabilities, working to give browser-default-like behavior for "push", "replace", and "reload" in addition to "traverse".

Similar to before, all of this can be opted out of by using scroll: "manual".

Closes #237 by giving an easy default behavior for SPA navigations with hashes.

Closes #231 by making "push", "replace", and "reload" behave by default in an MPA-like manner with respect to scroll position, just like "traverse" does.
domenic added a commit that referenced this issue Jul 13, 2022
This changes the default behavior of intercepted navigations with respect to scrolling in the following ways:

* Scroll restoration is performed on reloads, not just traverses.
* For pushes and replaces, we either scroll to the top or scroll to the fragment.

This change requires some updated API surface: the scrollRestoration option to intercept() has become scroll, and the navigateEvent.restoreScroll() method has become navigateEvent.scroll(). The latter method now has more capabilities, working to give browser-default-like behavior for "push", "replace", and "reload" in addition to "traverse".

Similar to before, all of this can be opted out of by using scroll: "manual".

Closes #237 by giving an easy default behavior for SPA navigations with hashes.

Closes #231 by making "push", "replace", and "reload" behave by default in an MPA-like manner with respect to scroll position, just like "traverse" does.
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

Successfully merging a pull request may close this issue.

2 participants