dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.55k stars 10.05k forks source link

Blazor Server - NavigationManager push to history #51042

Open lofcz opened 1 year ago

lofcz commented 1 year ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

There is no clear way to implement a user-friendly list & detail page pattern. Consider an application consisting of two pages:

/articles
/article/{id}

The /articles endpoint enumerates articles, and then the user opens one. After the user is finished with the article, they navigate back to the /articles endpoint. This will result in the /articles page fetching and rendering the articles again and the vertical scroll position of the page incorrectly reset.

Now the user has to scroll to the place where they left off again. This is a bad user experience.

Currently, our options are:

  1. We can remedy the scrolling issue by storing the scroll position when the navigation is about to happen and restoring it after we are brought back to /articles via NavigationManager.RegisterLocationChangingHandler, NavigationManager.LocationChanged, and some JS.

However, this approach is flawed. In order to scroll back to the correct position we need to wait for the articles to be fetched, rendered and only after that we can scroll.

  1. We can exploit the fact that navigation where only the query string is changed eg. from /articles to /articles?detail={id} doesn't result in the original content being lost and with OnParametersSet implement a switch where we either render the articles or the requested detail. We still need to fix the scroll the same way as in (1) but this time we avoid fetching the articles again hence removing the delay and improving the end-user experience.

However, this way we make a compromise with the URLs. This isn't a sustainable way to solve the issue.

  1. We avoid Blazor's navigation manager altogether and solve the issue with a combination of JS's history API calls. This is fairly hard to get working correctly if we want to intercept only certain navigation events (considering there could be more endpoints and we don't want to get these involved). We can hook popstate, hrefs being clicked, and so on before Blazor's JS is loaded so we can snatch these events as needed.

Again, this is pretty hard and the framework is a hindrance actively fighting against you at that point.

Now consider the articles could have a parameter attached to them, say a number of likes and we want that number to be updated when we navigate back from an article detail to the list.

A real-world example to grasp this concept is simple: navigate to Reddit, scroll a little, open a post, and navigate back. Observe the likes updated, and the scroll position kept intact.

To do this, the ideal way is to hide the content of a container listing the articles when we navigate to the detail of an article, rendering the article itself in another container such as:

<div class="articles"></div>
<div class="articleDetail"></div>

This can be done in quite a few ways in Blazor, however, we can't easily change the URL while preserving the original content, essentially what history.pushState does. If we call this function ourselves, Blazor will avoid intercepting the navigation, see https://github.com/dotnet/aspnetcore/blob/main/src/Components/Web.JS/src/Services/NavigationManager.ts#L184 as to why and how.

Similar issues:

42443

Describe the solution you'd like

The clearest way to solve this would be to introduce a new API:

+NavigationManager.PushState(string url, string? historyEntryState = null)
ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

lofcz commented 1 year ago

@SteveSandersonMS pinging as I can offer to implement this, however, I'd like to gather feedback on the proposal first.

*This would target .NET 9 for clarity.

SteveSandersonMS commented 1 year ago

Thanks for offering. It sounds like a potentially complex feature so whether or not we would accept it depends a lot on how simple the implementation could be, how simple any options would be that developers would need to control, and whether we can be confident enough it won't cause other problems or complexities.

If you're interested in exploring this please go ahead! Just wanted to be clear up front that if it turns out to be complex or a potential risk area then we would be less likely to take it as a core framework feature, and might want to suggest ways to plug it in as an external package instead.

lofcz commented 1 year ago

Thanks for the reply, as of .NET 8 I don't see a way to do this as an external package without a lot of infighting with the built-in NavigationManager. I've created a quick prototype and snatching & returning control over navigation to the NavigationManager is fairly complex to do reliably. Hence I'd like to start a discussion on this topic in case anyone has other ideas on how this could be implemented (possibly with existing APIs).

I believe this feature should be supported by the framework as it's a very commonly used one in real-world applications. Going through similar issues on this topic I've identified at least five that in one way or another hint at this problem.

Consider even a simple blog-like web application, currently, the user experience suffers from the problem described above. This need is strongly supported by the original proposal of NavigationApi in JS https://chromestatus.com/feature/6232287446302720

I would appreciate any further input on the issue.

lofcz commented 1 year ago

@SteveSandersonMS I cobbled a quick prototype together - as of now, this is implemented only for Blazor Server, Webassembly & WebView implementation is pending.

https://github.com/lofcz/aspnetcore/commit/6bb0c3c10b7b0129e683e3616dc7ee2649ece938

It seems working from the preliminary tests I did, however I have a hard time developing E2E tests as the way Basic test app routing is set up doesn't support resolving the correct page after refresh if the page is in the form of /subdir/...

image (after refreshing the select is reset)

I've considered extending the Router demo with my two pages representing a list of some kind and a detail. However, to test it fully I'd need to do something like:

1. /subdir/pushStateOverview
2. /subdir/pushStateDetail/{id}
3. user refreshes the tab
4. /subdir/pushStateDetail/{id} still has the correct content

Could you advise on how to proceed? Thanks.

SteveSandersonMS commented 1 year ago

Checking the implementation, I could use a bit of clarification of your goals. I thought your goal was to deal with scroll position restoration, but it seems like the code only deals with calling history.pushState.

Isn't this the same as what the framework already does if you call NavigationManager.NavigateTo?

lofcz commented 1 year ago

My intention is to be able to push to history in a way that doesn't trigger navigation and at the same time will be picked by the built-in navigation manager (when the user navigates back / forward). Things such as restoring scroll position or other end-user-specific requests are not in the scope of the proposal, I've merely illustrated how the proposed API would be typically used. The goal here is to provide the bare minimum for users to enable implementation of such features without having to resort to listening to popstate event before Blazor and conditionally canceling the bubbling before Blazor reacts to the event itself.

In simple terms, consider the Reddit example above. You are scrolling through the posts and you open one of them. What you do at the business code level is:

  1. Change the URL to the one representing the detail of the post.
  2. Store the current scroll position.
  3. Hide the current content sans the navbar, footer, etc., and load the post detail.
  4. If the user refreshes the page the correct content will be loaded because of (1).
  5. If the user instead navigates back either via the "back button" or some in-app link, we can show the original content we hid in (3), update data, or do some other book-keeping and finally we hide / clear the container we used to load the post detail in (3). This way we avoid the delay of loading the posts again which in turn delays our being able to restore the scroll position leading to a very unpleasant end-user experience where the page "jumps" after a while to the place where they left off.

The only thing missing in Blazor to enable this is to change the URL without actually triggering the navigation (1).

Hence I think this proposal isn't that complicated and could be worked into a mergable PR.

Please let me know if this clarifies the proposal.

Edit: thinking of it, this could probably be done as an extension of NavigationOptions and handled via existing NavigateTo as the implementation is in fact just a reduced version of an already working feature.

public readonly struct NavigationOptions
{
    /// <summary>
    /// If true, bypasses client-side routing and forces the browser to load the new page from the server, whether or not the URI would normally be handled by the client-side router.
    /// </summary>
    public bool ForceLoad { get; init; }

    /// <summary>
    /// If true, replaces the currently entry in the history stack.
    /// If false, appends the new entry to the history stack.
    /// </summary>
    public bool ReplaceHistoryEntry { get; init; }

    /// <summary>
    /// Gets or sets the state to append to the history entry.
    /// </summary>
    public string? HistoryEntryState { get; init; }

+    /// <summary>
+    /// If false, the uri will be only pushed/replaced (based on <see cref="ReplaceHistoryEntry " />) without actually navigating to the page.
+    /// </summary>
+    public bool TriggerNavigation{ get; init; } = true;
}
ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

gilm0079 commented 1 year ago

I would vote for this as well. I was under the assumption that the NavigateTo would not execute navigation if I was just updating the URL with new query params, but I'm finding out that my app is going through a dispose/initialize cycle when I use NavigateTo for query param updates. I'm changing my app to use JS history.pushState in the meantime until NavigationManager natively supports silent url updates.

SteveSandersonMS commented 1 year ago

@gilm0079 It's hard for us to be sure whether that's the same requirement as described in this issue or not. There are lots of things you could mean based on what hosting model you're using, when this redirection would occur, and so on. To avoid confusion, would you consider posting a new issue, rather than continuing this thread, and including a minimal code example showing what you are trying to do that causes the unwanted dispose/initialize cycle? Thanks!

gilm0079 commented 1 year ago

@SteveSandersonMS I created a new issue, linked here. There is a minimal solution I have started to work on. It is not replicating with the minimal blazor webassembly so I'll keep adding complexities until it does. Any suggestions would be helpful. Thanks!