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] Enable cancellation of navigation events #42877

Closed
MackinnonBuck opened this issue Jul 22, 2022 · 12 comments · Fixed by #43079
Closed

[Blazor] Enable cancellation of navigation events #42877

MackinnonBuck opened this issue Jul 22, 2022 · 12 comments · Fixed by #43079
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Milestone

Comments

@MackinnonBuck
Copy link
Member

MackinnonBuck commented Jul 22, 2022

Background and Motivation

The ability to asynchronously delay/cancel navigations in Blazor has been a very popular ask from the community. Such a feature would allow Blazor developers to implement custom navigation prompts, save changes before navigating, or silently cancel navigations. There was a previous attempt to solve this problem in a limited fashion by using window.confirm() to display a built-in browser prompt that lets the user confirm navigations. This approach works well for simple cases, but community feedback indicated a desire for a more flexible mechanism, namely a "location changing" event enabling asynchronous callbacks to decide whether a navigation should be allowed or cancelled.

Proposed API

We provide new APIs on NavigationManager to add and remove "location changing" handlers that run when an "internal" navigation is initiated. These handlers are given a LocationChangingContext that provides information about the ongoing navigation in addition to a PreventNavigation() method that prevents the navigation from continuing when invoked. There can be multiple handlers registered that get executed in parallel, where any handler may prevent the navigation.

In addition, we also provide a <NavigationLock/> component that wraps the new NavigationManager APIs. It includes an option for showing a browser prompt for confirming "external" navigations. Note that external navigations will not cause "location changing" handlers to get invoked. This is because the beforeunload event (used to determine if a prompt should display before navigating) must return synchronously, so the result from an asynchronous JS -> .NET callback cannot be utilized.

Note: These changes have already been implemented in #42638. A follow-up PR will address any API review feedback.

Note: All of the following APIs are purely additive. I didn't use the + ... diff format because then you don't get syntax highlighting. NavigationManager is an existing class and all of the members listed here are new. The other two classes are entirely new.

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
    /// <summary>
    /// Adds a handler to process incoming navigation events.
    /// </summary>
    /// <param name="locationChangingHandler">The handler to process incoming navigation events.</param>
    public void AddLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);

    /// <summary>
    /// Removes a previously-added location changing handler.
    /// </summary>
    /// <param name="locationChangingHandler">The handler to remove.</param>
    public void RemoveLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);

    /// <summary>
    /// Notifies the registered handlers of the current location change.
    /// </summary>
    /// <param name="uri">The destination URI. This can be absolute, or relative to the base URI.</param>
    /// <param name="state">The state associated with the target history entry.</param>
    /// <param name="isNavigationIntercepted">Whether this navigation was intercepted from a link.</param>
    /// <returns>A <see cref="ValueTask{TResult}"/> representing the completion of the operation. If the result is <see langword="true"/>, the navigation should continue.</returns>
    protected ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted);

    /// <summary>
    /// Invokes the provided <paramref name="handler"/>, passing it the given <paramref name="context"/>.
    /// This method can be overridden to analyze the state of the handler task even after
    /// <see cref="NotifyLocationChangingAsync(string, string?, bool)"/> completes. For example, this can be useful for
    /// processing exceptions thrown from handlers that continue running after the navigation ends.
    /// </summary>
    /// <param name="handler">The handler to invoke.</param>
    /// <param name="context">The context to pass to the handler.</param>
    /// <returns></returns>
    protected virtual ValueTask InvokeLocationChangingHandlerAsync(Func<LocationChangingContext, ValueTask> handler, LocationChangingContext context);

    /// <summary>
    /// Sets whether navigation is currently locked. If it is, then implementations should not update <see cref="Uri"/> and call
    /// <see cref="NotifyLocationChanged(bool)"/> until they have first confirmed the navigation by calling
    /// <see cref="NotifyLocationChangingAsync(string, string?, bool)"/>.
    /// </summary>
    /// <param name="value">Whether navigation is currently locked.</param>
    protected virtual void SetNavigationLockState(bool value);
}
namespace Microsoft.AspNetCore.Components.Routing;

/// <summary>
/// Contains context for a change to the browser's current location.
/// </summary>
public class LocationChangingContext
{
    /// <summary>
    /// Gets the target location.
    /// </summary>
    public string TargetLocation { get; }

    /// <summary>
    /// Gets the state associated with the target history entry.
    /// </summary>
    public string? HistoryEntryState { get; }

    /// <summary>
    /// Gets whether this navigation was intercepted from a link.
    /// </summary>
    public bool IsNavigationIntercepted { get; }

    /// <summary>
    /// Gets a <see cref="System.Threading.CancellationToken"/> that can be used to determine if this navigation was canceled
    /// (for example, because the user has triggered a different navigation).
    /// </summary>
    public CancellationToken CancellationToken { get; }

    /// <summary>
    /// Prevents this navigation from continuing.
    /// </summary>
    public void PreventNavigation();
}

/// <summary>
/// A component that can be used to intercept navigation events. 
/// </summary>
public class NavigationLock : IComponent, IAsyncDisposable
{
    /// <summary>
    /// Gets or sets a callback to be invoked when an internal navigation event occurs.
    /// </summary>
    public EventCallback<LocationChangingContext> OnBeforeInternalNavigation { get; set; }

    /// <summary>
    /// Gets or sets whether a browser dialog should prompt the user to either confirm or cancel
    /// external navigations.
    /// </summary>
    public bool ConfirmExternalNavigation { get; set; }
}

Usage Examples

<NavigationLock/> Component

@inject IJSRuntime JSRuntime

<NavigationLock OnBeforeInternalNavigation="OnBeforeInternalNavigation" ConfirmExternalNavigation="true" />

@code {
    private async Task OnBeforeInternalNavigation(LocationChangingContext context)
    {
        // This just displays the built-in browser confirm dialog, but you can display a custom prompt
        // for internal navigations if you want.
        var isConfirmed = await JSRuntime.InvokeAsync<bool>("confirm", "Are you sure you want to continue?");

        if (!isConfirmed)
        {
            context.PreventNavigation();
        }
    }
}

NavigationManager APIs

Useful in cases where the lifetime of a navigation lock is not constrained to the lifetime of a specific Blazor component.

Risks

The implementation of this feature is quite involved (see #42638), and its asynchronous nature makes handling cases like overlapping navigations very non-trivial. However, we have ample test coverage for these scenarios, so we're reasonably confident in what we have. Code that doesn't use this feature won't be impacted by its existence; the entire "location changing" step is skipped unless there are handlers registered.

@MackinnonBuck MackinnonBuck added area-blazor Includes: Blazor, Razor Components api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 22, 2022
@ghost
Copy link

ghost commented Jul 22, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 25, 2022

Notes in advance of API review:

  • InvokeLocationChangingHandlerAsync - I'm not keen on this. It feels like the other new API (NotifyLocationChangingAsync) should be enough on its own for platform-specific subclasses to notify the base class to start this flow.
    • In practice, what InvokeLocationChangingHandlerAsync is used for is the same across all three of our hosting models: it's for catching exceptions that occur after a different handler has already completed, and dispatching them to the platform-specific unhandled exception sink.
    • The problem is that somebody implementing a NavigationManager subclass wouldn't really know why they should override InvokeLocationChangingHandlerAsync or what they should do in that override, and things would seem to work anyway unless the developer knew about testing this hard-to-achieve edge case.
    • If we have to have a platform-specific API, perhaps it could instead be a protected virtual void HandleException. Then the NavigationManager base class could deal with this try/catch flow itself, and would just call HandleException if necessary, and we wouldn't have to duplicate the logic or create a more generalised virtual method than we definitely need.

@SteveSandersonMS
Copy link
Member

Unclear if CancellationToken would fire if you called PreventNavigation

@halter73
Copy link
Member

halter73 commented Jul 25, 2022

API Review Notes:

  • NavigationLock is the primary way to interact with this new API. Not NavigationManager.
  • NavigationLock should this also have a bool IsLocked parameter?
    • See previous comment from @SteveSandersonMS.
    • No because the if inside the callback works pretty well without need for extra API.
  • Do navigation handlers run concurrently?
    • Yes. If you want order, put your logic in a single handler that runs things in order.
    • This is why the CancellationToken exists. It's tied to PreventNavigation() we think?
  • The primary reason for the CancellationToken is if navigation occurs again before the handler completes.
  • Why use EventCallback if it doesn't have great async support?
    • It is already it commonly used for async callbacks, and actually has very good async support.
    • We need single parameter which is why it cannot be Func<LocationChangingContext, CancellationToken, ValueTask>.
  • What about using CancellationToken.Register-like IDisposable? We do this for SignalR's HubConnection.On.
  • Why not use a C# event like LocationChanged for LocationChanging?
    • C# event handlers are fire and forget and we want to await the ValueTask?
  • Can we remove InvokeLocationChangingHandlerAsync?
    • We think so. If possible, less public API is better.

Possible API based on feedback:

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public IDisposable RegisterLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
+    protected ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted);
+    protected virtual void HandleException(Exception ex);
+    protected virtual void SetNavigationLockState(bool value);
}

namespace Microsoft.AspNetCore.Components.Routing;

+ public class LocationChangingContext
+ {
+     public string TargetLocation { get; }
+     public string? HistoryEntryState { get; }
+     public bool IsNavigationIntercepted { get; }
+     public CancellationToken CancellationToken { get; }
+     public void PreventNavigation();
+ }

+ public class NavigationLock : IComponent, IAsyncDisposable
+ {
+     public EventCallback<LocationChangingContext> OnBeforeInternalNavigation { get; set; }
+     public bool ConfirmExternalNavigation { get; set; }
+ }

@halter73 halter73 added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 25, 2022
@MackinnonBuck
Copy link
Member Author

... This is why the CancellationToken exists. It's tied to PreventNavigation() we think?

Yes, when a handler calls PreventNavigation(), the CancellationToken will cancel after the handler returns. Cancellation will also occur when a new navigation interupts an ongoing navigation.

What about using CancellationToken.Register-like IDisposable? We do this for HubConnection.On?

I'm open to doing it this way.

The possible API listed in the previous comment looks good to me. Does this need another formal API review before this issue can be api-approved?

@halter73
Copy link
Member

halter73 commented Aug 1, 2022

The possible API listed in the previous comment looks good to me. Does this need another formal API review before this issue can be api-approved?

That sounds good to me. We were just waiting on your feedback. API Approved!

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public IDisposable RegisterLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
+    protected ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted);
+    protected virtual void HandleException(Exception ex);
+    protected virtual void SetNavigationLockState(bool value);
}

namespace Microsoft.AspNetCore.Components.Routing;

+ public class LocationChangingContext
+ {
+     public string TargetLocation { get; }
+     public string? HistoryEntryState { get; }
+     public bool IsNavigationIntercepted { get; }
+     public CancellationToken CancellationToken { get; }
+     public void PreventNavigation();
+ }

+ public class NavigationLock : IComponent, IAsyncDisposable
+ {
+     public EventCallback<LocationChangingContext> OnBeforeInternalNavigation { get; set; }
+     public bool ConfirmExternalNavigation { get; set; }
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Aug 1, 2022
@halter73
Copy link
Member

halter73 commented Aug 1, 2022

One thing I missed earlier is that LocationChangingContext doesn't have a public constructor unlike LocationChangedEventArgs which does. This might be important for testing.

I also forget if we discussed whether we should seal LocationChangingContext and/or NavigationLock. If we decide we want to change either of these things, I think we can get these minor updates approved fairly easily via email.

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Aug 2, 2022
@ghost
Copy link

ghost commented Aug 2, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Aug 2, 2022

API Review Notes:

  • Do we want a constructor for LocationChangingContext?
    • Yes. Even though things like bUnit and xUnit can create instances of the LocationChangingContext without a public constructor, it shouldn't be necessary to resort to reflection.
  • Should we seal the types?
    • Why not? We don't expect developers will want to derive from either NavigationLock or LocationChangingContext.

API update approved.

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public IDisposable RegisterLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
+    protected ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted);
+    protected virtual void HandleException(Exception ex);
+    protected virtual void SetNavigationLockState(bool value);
}

namespace Microsoft.AspNetCore.Components.Routing;

+ public sealed class LocationChangingContext
+ {
+    public LocationChangingContext(string targetLocation, string? historyEntryState, bool isNavigationIntercepted, CancellationToken cancellationToken);
+     public string TargetLocation { get; }
+     public string? HistoryEntryState { get; }
+     public bool IsNavigationIntercepted { get; }
+     public CancellationToken CancellationToken { get; }
+     public void PreventNavigation();
+ }

+ public sealed class NavigationLock : IComponent, IAsyncDisposable
+ {
+     public NavigationLock();
+     public EventCallback<LocationChangingContext> OnBeforeInternalNavigation { get; set; }
+     public bool ConfirmExternalNavigation { get; set; }
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 2, 2022
@halter73 halter73 added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Aug 3, 2022
@halter73
Copy link
Member

halter73 commented Aug 3, 2022

There was more offline feedback about this API, so we're going to edit it hopefully one last time.

API Review Notes:

  • @BrennanConroy asks, "Should the context be required init without a ctor? I know we’ve been playing with that in other APIs."
  • Would everything in LocationChangingContext but HistoryEntryState be required? Should we provide defaults for IsNavigationIntercepted and/or CancellationToken?
    • Providing defaults for IsNavigationIntercepted and CancellationToken seems good to everyone
  • @MackinnonBuck says, "I think “HandleException” implies something more general-purpose than what’s really going on. For example, exceptions thrown when invoking the LocationChanged event won’t get caught and passed to the new HandleException method (they could, but that would be a breaking change). "
    • HandleException could probably be made non-breaking by having it return a bool to opt-in to marking the LocationChanged event as handled.
  • @MackinnonBuck also says, "Furthermore, providing a LocationChangingContext in this method allows for NavigationManager implementations to log more meaningful error messages (e.g. “An exception was thrown while navigating to location XYZ”). We would lose information about the fact that the exception was thrown in a location changing handler if we went the HandleException route."
    • I see the benefit to passing in the LocationChangingContext which preclude the same handler handling LocationChanged exceptions where this context does not exist.

Approved!

namespace Microsoft.AspNetCore.Components;

public abstract class NavigationManager
{
+    public IDisposable RegisterLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
+    protected ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted);
+    protected virtual void HandleLocationChangingHandlerException(Exception ex, LocationChangingContext context);
+    protected virtual void SetNavigationLockState(bool value);
}

namespace Microsoft.AspNetCore.Components.Routing;

+ public sealed class LocationChangingContext
+ {
+     public required string TargetLocation { get; init; }
+     public string? HistoryEntryState { get; init; }
+     public bool IsNavigationIntercepted { get; init; }
+     public CancellationToken CancellationToken { get; init; }
+     public void PreventNavigation();
+ }

+ public sealed class NavigationLock : IComponent, IAsyncDisposable
+ {
+     public NavigationLock();
+     public EventCallback<LocationChangingContext> OnBeforeInternalNavigation { get; set; }
+     public bool ConfirmExternalNavigation { get; set; }
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 3, 2022
@garrettlondon1
Copy link

Is there a roadmap or plan for this feature? Forgive me if I'm new to the process. Thanks

@SteveSandersonMS
Copy link
Member

@garrettlondon1 Yes, this feature will ship in .NET 7.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants