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

[Blazor] Add support for confirming navigations #40149

Closed
pranavkm opened this issue Feb 11, 2022 · 20 comments
Closed

[Blazor] Add support for confirming navigations #40149

pranavkm opened this issue Feb 11, 2022 · 20 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one Priority:1 Work that is critical for the release, but we could probably ship without

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Feb 11, 2022

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. #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

  • Most popular issue for Blazor at the time of writing.

In-scope

  • Add an API that allows components to prevent navigation in a platform-independent way.
  • Provide turnkey solution that integrates with EditForm

Out-of-scope

  • Not a general purpose location changing event handler.

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();
    }
}
@pranavkm pranavkm added enhancement This issue represents an ask for new feature or an enhancement to an existing one area-blazor Includes: Blazor, Razor Components labels Feb 11, 2022
@DaveNay
Copy link

DaveNay commented Feb 11, 2022

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
Copy link
Contributor Author

@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.

@pranavkm pranavkm added this to the .NET 7 Planning milestone Feb 11, 2022
@ghost
Copy link

ghost commented Feb 11, 2022

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.

@pranavkm pranavkm removed this from the .NET 7 Planning milestone Feb 11, 2022
@alxtr3
Copy link

alxtr3 commented Feb 11, 2022

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
Copy link

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:

  • Have the NavigationManager track that a Navigation happened
  • Cancel the browser navigation event
  • Let the NavigationManager await a NavigationChanging event
  • Re-issue the Navigation event if the value from the NavigationChanging func is true (the return value meaning OK to navigate)

What about that is a performance problem?

@ShaunCurtis
Copy link

ShaunCurtis commented Feb 12, 2022

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
Copy link

robertmclaws commented Feb 12, 2022

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
Copy link
Contributor

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 #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 #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.

@mkArtakMSFT mkArtakMSFT added this to the .NET 7 Planning milestone Feb 14, 2022
@ghost
Copy link

ghost commented Feb 14, 2022

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
Copy link

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
Copy link

nvmkpk commented Feb 15, 2022

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
Copy link

ShaunCurtis commented Feb 15, 2022

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.

@mkArtakMSFT mkArtakMSFT added Priority:1 Work that is critical for the release, but we could probably ship without triaged labels Mar 30, 2022
@americanslon
Copy link

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

@Oneguy23
Copy link

Oneguy23 commented May 6, 2022

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
Copy link

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

@uecasm
Copy link

uecasm commented May 18, 2022

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
Copy link

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
Copy link

Liero commented Jul 25, 2022

@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
Copy link

DNF-SaS commented Aug 23, 2022

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

@MackinnonBuck
Copy link
Member

Closing, since this was handled in #42877

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

No branches or pull requests