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

Allow cancellation of navigation events in Blazor #42638

Merged
merged 32 commits into from
Jul 22, 2022

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jul 8, 2022

Allow cancellation of navigation events in Blazor

Introduces new APIs that enable the cancellation of internal and external navigation events.

API Overview

New NavigationManager APIs can be used to add and remove handlers that may asynchronously cancel internal navigations:

public abstract class NavigationManager
{
    public void AddLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
    public bool RemoveLocationChangingHandler(Func<LocationChangingContext, ValueTask> locationChangingHandler);
}

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

The <NavigationLock /> component wraps these APIs so this functionality can easily be added to a Blazor component:

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

@code {
    private void OnBeforeInternalNavigation(LocationChangingContext context)
    {
        // ...
    }
}

When ConfirmExternalNavigation is true, the browser's built-in confirmation dialog prompts the user if they would like to continue before performing an external navigation.

Important behavioral considerations

Since internal navigations can be canceled asynchronously, it's possible that multiple overlapping navigations may occur. These might include cases where the user is rapidly clicking the back button on a page or clicking multiple links before a decision about whether to permit the navigation is made. Following is a summary of how this asynchrony is handled:

  • If there are any "location changing" handlers registered, all navigations are initially reverted, then replayed if the navigation does not get canceled.
  • If overlapping navigation requests are made, the latest request always cancels earlier requests. This means:
    • The app may treat multiple back/forward button clicks as a single click.
    • If the user clicks multiple links before the navigation completes, the most recently clicked link will be navigated to.

It's also important to note that the <NavigationLock /> component only takes effect after being rendered (just like any other Blazor component). Practically, this means it's possible that the user may quickly navigate past a page that was supposed to intercept the navigation, but it didn't because the user navigated away before the component had a chance to render.

Some other details to be aware of:

  • Programmatically-initiated navigations (e.g. NavigationManager.NavigateTo()) will also invoke the "location changing" handlers.
  • The navigation behavior for existing apps does not change if these features are not used.

Fixes #14962

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 8, 2022
Added a safeguard to ensure overlapping navigations are correctly canceled during high network latency scenarios.
@SteveSandersonMS
Copy link
Member

public CancellationToken CancellationToken { get; }
public bool IsCanceled { get; }
public void Cancel();

This triplet could be confusing, because we're using the word "cancel" to mean two different things (if I'm following correctly). The CancellationToken tells you if this OnLocationChanging call has been cancelled, whereas Cancel/IsCanceled are about the navigation itself being cancelled.

I'd suggest we disambiguate by not using the phrase "cancel" about the navigation itself, and saying something like "PreventNavigation" instead, e.g.:

public CancellationToken CancellationToken { get; }
public void PreventNavigation();
public bool IsNavigationPrevented { get; }

@SteveSandersonMS
Copy link
Member

Also to be honest, I'm not sure what is the use case for IsNavigationPrevented. It seems like the objective might be to do with orchestrating across multiple handlers, but given asynchrony, it doesn't quite cover it because even if you check once, another handler might then prevent navigation later.

What if instead, there was just PreventNavigation and CancellationToken, and the act of calling PreventNavigation would also cause the cancellation token to fire? In that case, other handlers would be notified that their opinion is no longer relevant since we're going to prevent navigation regardless, so they can stop doing work if they want to.

I'm not 100% sure how usable this would end up being (whether there are any cases where people would have to suppress a TaskCancelledException) but on the surface it seems like it might be a valid and simple pattern.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 11, 2022

More API thoughts:

public string Location { get; }

I'm guessing this is the destination location, not the current location, but just to be clear maybe rename to DestinationUrl or TargetUrl or similar (and I'd suggest "Url" over "location" since it matches the "Url" terminology on NavigationManager).

UPDATE I now realise that LocationChangedEventArgs already has a property called Location to refer to the destination. It's marginally less ambiguous in that case since the navigation has already happened so you don't have the current/destination distinction, but I agree with keeping consistent with that since it's the closest other API in the framework. So how about DestinationLocation or TargetLocation?

public bool ForceLoad { get; }

Do we need this info? If it's a client-originated navigation (Blazor.navigateTo(...)), I don't think the flag could ever be true, because if it was we'd have bypassed the navigation system anyway. So I think this can only be true for server-originated navigations, which might be a confusing subtlety. I'm not sure there are many cases where a developer needs this info to decide whether to allow the navigation to continue. If you have a concrete example then great, otherwise maybe we drop this flag (to avoid the subtlety mentioned above) and only add it later if a clear use case arises.

@MackinnonBuck
Copy link
Member Author

@javiercn I've addressed some of your feedback around cancellation and added some unit tests (with a few more still remaining).

/cc @SteveSandersonMS in case you have any thoughts about the recent modifications.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great, I have a couple of open questions/suggestions left, but this will be ready to go after resolving those.

@javiercn
Copy link
Member

UPDATE: Actually, the unfortunate thing about having more than two arguments in the location changing handler is that EventCallback only allows for a single parameter. So we would have to re-wrap the LocationChangingContext and CancellationToken into a single object before so it can be passed to the OnBeforeInternalNavigation event callback when using the component. And since using is arguably the primary way people are going to implement location changing handlers, I think it's probably best to leave CancellationToken as a property on LocationChangingContext

I think I get what you are saying and that's a valid reason to keep the cancellation token in the same place.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

Thanks @MackinnonBuck for persevering through all the nuances on the implementation details.

We still need to solve the problem we are discussing offline, but I am signing off already as I think we will be good to go after that.

@SteveSandersonMS
Copy link
Member

/cc @SteveSandersonMS in case you have any thoughts about the recent modifications.

It looks great to me. Reading through the comments, it seems like this has been worked out in great detail now. Thanks also to @javiercn for the quality feedback.

@MackinnonBuck
Copy link
Member Author

I'm going to merge this now - I will address any further feedback in a follow-up PR along with the API review feedback.

Thank you @SteveSandersonMS and @javiercn for your detailed feedback and collaboration on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose location changing event for NavigationManger
7 participants