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.37k stars 9.99k forks source link

[Blazor] Add support for confirming navigations #40149

Closed pranavkm closed 2 years ago

pranavkm commented 2 years ago

Summary

Support for Location change event is a very popular ask from our users. Unfortunately, given the nature of the event adding support for it that uses browser intrinsic is going to be difficult / impossible. https://github.com/dotnet/aspnetcore/pull/24417/ attempted to solve this by hijacking browser navigation and managing it via Blazor. This is a fairly involved solution, with possible significant perf overhead, and we'd like to determine if using a narrower solution would suffice here.

A popular theme in the issue focused on preventing navigation to avoid losing unsaved changes in a component. A well-upvoted comment provides a sample for just this - https://github.com/ShaunCurtis/Blazr.Demo.EditForm/tree/master/Blazr.NavigationLocker. This spec is a way to determine if this is something we could get away with.

Motivation and goals

In-scope

Out-of-scope

Proposed solution

We introduce an API on NavigationManager that allows users to confirm when a navigation is to occur.AddNavigationConfirmationAsync returns an IAsyncDisposable which prompts for a confirmation until the returned value is disposed.

public class NavigationManager
{
+   public ValueTask<IAsyncDisposable> AddNavigationConfirmationAsync(string message, CancellationToken cancellationToken);
}

Here's an example of it in use:

@inject NavigationManager Nav

<form @oninput="OnInput" @onvalidsubmit="OnSubmit">
    ...
    <button type="submit">Submit</button>
</form>

@code {
    IAsyncDisposable? _navigationConfirmation;

    async Task OnInput()
    {
        _navigationConfirmation ??= await Nav.AddNavigationConfirmationAsync("Are you sure you want to exit without saving?");
    }

    async Task OnSubmit() => await RemoveNavigationConfirmationAsync();

    ValueTask IAsyncDisposable.DisposeAsync() => RemoveNavigationConfirmationAsync();

    ValueTask ClearConfirmationAsync() => await (_navigationConfirmation?.DisposeAsync() ?? default);
}

The implementation relies on window.confirm for a navigation that is handled via Blazor's routing and beforeunload event for all other navigation. The message parameter is used as part of the confirm dialog, but is likely to be ignored by the unload event since browsers do not consistently support specifying it.

In addition to this, we introduce a LockNavigationWhenDirty component that uses EditContext's dirty state (IsModified() /MarkAsUnmodified()`) to add / remove navigation confirmations. Here is what this component looks like:

public class LockNavigationWhenDirty : IAsyncDisposable
{
    public LockNavigationWhenDirty(NavigationManager navigationManager);
    [EditorRequired, Parameter] public string Message { get; set; }
}

Usage:

<EditForm ... @onvalidsubmit="OnValidSubmit">
    <LockNavigationWhenDirty /> 
    ...
</EditForm>

@code {
    async OnValidSubmit()
    {
        // Save content
        repository.SaveAsync(..);

        // Release the lock now that we're satisfied
        editContext.MarkAsUnmodified();
    }
}
DaveNay commented 2 years ago

Can the use of window.confirm be replaced with a generic callback that can be handled in user code? The browser dialogs are pretty ugly and do not fit with custom site theming.

pranavkm commented 2 years ago

@DaveNay that seems like a reasonable ask. I think we should be able to support a callback for anchor tags / if the navigation is handled by Blazor's routing.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

alxtr3 commented 2 years ago

not already supporting onlocationchanging and cancel of navigation is just crazy!

also LocationChanged doesn't work when loading a page outside the current blazor app and Dispose() methods on the razor components are not even called when exiting the app.. hahaha I'm sure you will add this to .Net 10 planning haha

robertmclaws commented 2 years ago

I guess for me, I just never really understood why it was such a performance issue. If you can plug into the browser navigation system enough to cancel a navigation, then why wouldn't you:

What about that is a performance problem?

ShaunCurtis commented 2 years ago

I can't say that I'm happy with this. There's no control for the programmer, just another popup box that we have little control over. I want to stop Navigation in a dirty form, and potentially show an in-form alert or toast. A simple bool BlockNavigation flag to turn off navigation, and a NavigationBlocked event to capture blocked navigation attempts. I can guess that's not going to fly because programmers could just turn off routing without realising it!

If the changed proposed above was implemented, I don't think I would use it in the majority of circumstances, just as I don't use the EditContext IsDirty because it's far too simplistic: for example, I can change a value back to it's original and the EditContext is still dirty though the underlying data object isn't.

Can we not do something with the Server JS interop call so we can plug in another Navigation Manager, as we can do with WASM? I'm guessing it wasn't designed that way, but... I'll have a think about that.

robertmclaws commented 2 years ago

If the changed proposed above was implemented, I don't think I would use it in the majority of circumstances, just as I don't use the EditContext IsDirty because it's far too simplistic: for example, I can change a value back to it's original and the EditContext is still dirty though the underlying data object isn't.

100% agree with this. We implemented INotifyPropertyChanged, IChangeTracking, and IRevertibleChangeTracking in our objects instead and eliminated our dependence on EditContext for this very reason.

Baking in a window.confirm means I will have virtually zero control over the branding and design of what happens in these situations. That isn't terribly useful, IMO.

If I can build a router using crossroads + history in javascript to handle this (which was based off the ASP.NET Core Knockout SPA template, BTW) then this problem can be solved in Blazor too.

MariovanZeist commented 2 years ago

Like other commenters here I would advise against using window.confirm to show a simple browser-specific dialog that cannot be styled and is limited in functionality. I think it will confuse the user as the confirmation dialog would stand out, and users might even believe that it's not part of the current application they are using.

I would also caution to use the beforeunload event in conjunction with window.confirm As stated in that paragraph:

To combat unwanted pop-ups, browsers may not display prompts created in beforeunload event handlers unless the page has been interacted with, or may even not display them at all.

(This was also the reason this event wasn't used in the above-mentioned PR, as its implementation and usefulness varied amongst browsers)

Although this proposal initially seems simpler than https://github.com/dotnet/aspnetcore/pull/24417 I would argue the complexity will approach that of the PR, as you still want to be able to cancel navigation in all the below cases:

  1. Calling NavigationManager.NavigateTo directly (except when setting the forceload flag),
  2. Clicking an href element
  3. Performing a history.back or history.forward function or clicking the back / forward history buttons in the browser.

And without using onbeforeunload I would assume that at a minimum, the same events will have to be intercepted I admit with this proposal the handling of those events would be simplified as I suspect the envisioned proposed solution would only have to check a variable and when it's set it would display the window.confirm dialog with a predetermined message and abort the navigation when the user presses cancel.

This proposal would also get rid of the additional Browser -> Server interaction (only when using Blazor Server, and which in the original PR would only be incurred when the developer had actually implemented an EventHandler) but at a high cost of losing all the flexibility and customizability that having a LocationChanging event in C# would bring.

And personally, the performance impact of that additional Browser -> Server interaction in that PR is negligible, as it will

  1. Only happen when the developer actually monitors for location changes.
  2. The user actually does an action that performs navigation.

As the original author of https://github.com/dotnet/aspnetcore/pull/24417 I admit I might be biased towards a similar solution, And although @pranavkm explicitly stated that this was out-of-scope, maybe we could build upon it while addressing some of the criticism. I would love to take another crack at this problem.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

GiffenGood commented 2 years ago

As others have said, a limited solution will not suffice. We do not want to be boxed into using the EditForm or the built-in browser dialog.

LocationChanging event with a cancellable event arg is the obvious and natural way to solve this. I do not understand the technical arguments for why it is not feasible. Is the issue related to providing a solution that works on Blazor Server and WASM?

All SPA frameworks that I have used provide a simple way to prevent a route change. And the wait for this feature is beyond frustrating. We have a very large medical app that we are porting to Blazor and in nearly every product demo I get asked about this feature.

At this point, I would switch to a 3rd party router if I found a reliable one.

nvmkpk commented 2 years ago

I think a simple cancellable event on the router would be good enough. We can live with the limitation of only being able to prevent navigations initiated from within blazor. Navigations initiated outside of blazor should be left to the developer to solve (which is an issue with any web based development).

ShaunCurtis commented 2 years ago

I have updated my implementation which is here - https://github.com/ShaunCurtis/Blazr.Demo.Routing. The basics are a lightly modified Router with an injected interface IBlazrNavigationManager rather than NavigationManager.

You can write your own IBlazrNavigationManager implementation to handle your specific circumstances.

I've included two IBlazrNavigationManager implementations:

  1. A vanilla pass through CoreNavigationManager implementation
  2. A BlazrNavigationManager implementation to route/not route based on a bool state property.

The library also includes a component for JS interaction with beforeunload and events driven by attempts to navigate.

This is what I intend to use in the future.

americanslon commented 2 years ago

Has some plan of action been decided on this? This is an essential feature that is sorely lacking.

Oneguy23 commented 2 years ago

IMHO, something is better than nothing. This is a big miss and having a partial solution would be much better than waiting for a perfect solution.

ShaunCurtis commented 2 years ago

I have seen mention of - "A LocationChanging event for NavigationManager (implement logic that occurs before navigation takes place)" Awaiting details.

uecasm commented 2 years ago

I've just made my own version of a library that blocks navigation on demand. It was inspired by the method used by Shaun's library (I was initially hoping to use a different method, but the DI was uncooperative) but isn't descended from it and isn't directly swappable (and has one component with the same name but a different function, just to be extra confusing).

I basically started with a clean copy of the latest .NET 6 router code (it would be really nice if a lot less things were internal and didn't have to be duplicated!) and hooked in an alternative NavigationManager that includes locking support. Notably this does include hot reload support and I think it's more composable (but I might just be biased).

I've only tested it in WebAssembly, although I don't think there's any inherent reason why it couldn't work on Server too.

(And yes, a LocationChanging event would have been nicer and made most of this unnecessary. But that seemed to be unpopular in the other PR for some reason.)

Aeroverra commented 2 years ago

I don't understand the performance concerns. Blazor already handles the navigation. I'm sure there are plenty of other ways this could be done without an issue. Was surprised to find this didn't already exist.

I agree with the users above in terms of using custom prompts as apposed to the ones provided by the default browser. Think of something like Discord. When you don't save and try to navigate the page shakes and warns you. I am not a UI designer but even I know if you replaced that with an browser alert it would kill the overall feel of the app.

Liero commented 2 years ago

@pranavkm:

  1. instead of <LockNavigationWhenDirty />, could we use <LockNavigation Enabled="editContext.IsModified()" />, which would cover some concerns regarding relying on editContext here?

  2. Will this work with multiple EditContexts in a page?

  3. Can we add EventCallback when navigation was prevented, with possibility to reissue the navigation? This should cover scenarios, when user want's to show custom ui.

I've been using this approach in Blazor Server for some time and I have noticed, that using javascript confirm() dialogs interrupts websockets connection -> If user does not react immediately, his session will expire and the session state will be lost anyway.

DNF-SaS commented 2 years ago

Also waiting for this feature. Even the browser-window is better than nothing.

MackinnonBuck commented 2 years ago

Closing, since this was handled in https://github.com/dotnet/aspnetcore/issues/42877