Feature/navigation lock

[linkdotnet]

This pull requests allows the FakeNavigationManager to prevent navigation when a NavigationLock component is existing. Fixes #804. At the moment this PR can not be merged as the feature is targeted for 7.0.0-rc.1 which will be available September. Also I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

New behaviour

• NavigationLock works with our FakeNavigationManager. That said if user code prevents navigation, so will the FakeNavigationManager
• No new entry will be added to the history if the user prevents navigation
• JSInterop is added to make NavigationLock work

Out of Scope

• In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:

public void ShouldCancelNavigationWhenMagicCondition()
{
    var cut = RenderComponent<...>();
    var navigationManager = Services.GetRequiredService<NavigationManager>();

    // simulate the click on a href
    navigationManager.NavigateTo("/internal");

    // Do some assertion here
}

Most probable we want to have this in the documentation. Still a bit of mixed feelings about that, but I can't imagine another way to guide the user here.

[egil]

I'm going to get to this as soon as I get a little time to do a proper review.

[egil]

I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

Since v1 will also support .net7 I think we need to target main. I will see about getting Codespaces set up here on github such that you can work on this.

Tracking this here: https://github.com/dotnet-foundation/projects/issues/218

[linkdotnet]

I targeted v2 but maybe it makes sense to have this feature available on our main branch as well. As I am mainly working on a Mac M1 I stuck to use >=net6.0. Let me know your thoughts. I am happy to target main instead of v2.

Since v1 will also support .net7 I think we need to target main. I will see about getting Codespaces set up here on github such that you can work on this.

nah don't worry. I still have my old beloved windows notebook. Everytime when I open something the fan goes crazy, but he still works ;)

That said I will retargeted the branch to main and edit the PR to target main

[egil]

• In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:

I think I need a bit more explanation on why this wont work with bUnit. Can you elaborate.

[egil]

OK, looks good so far. I'm still vacationing, but will see if I can get back to this again later today or tomorrow with a follow-up. Thanks for your patience.

[linkdotnet]

• In bUnit this will ever work only with the NavigationManager itself. In theory the "wild" aka "real blazor" we can use the NavigationLock with stuff like that: <a href="/internal">Text</a> and the user can prevent navigation. As we are no browser and do not have a router which would the task for us, we can't implement that. But the user can do the following in a test:

I think I need a bit more explanation on why this wont work with bUnit. Can you elaborate.

Ok. Let's make it visual:

@inject NavigationManager NavigationManager
<a href="/counter">Go to Counter</a>
<button @onclick="@(() => NavigatioNmanager.NavgiateTo("/counter"))">Go to counter</button>

<NavigationLock OnBeforeInternalNavigation="PreventNavigation"></NavigationLock>

@code {
    private void PreventNavigation(LocationChangingContext context) => context.PreventNavigation();
}

Let's ignore bUnit for a second. If we have this component and open our browser, the user can click either on the button or the <a> element and will not be redirected to the counter page (thanks to the NavigationLock and OnBeforeInternalNavigation where we prevent navigation). Important is that the button uses the NavigationManager where the <a> element relies on the router and browser.

Back to bUnit. The <button> case is fairly simple:

public void ButtonCase()
{
    var cut = RenderComponent<MyComponent>();

    var button = cut.Find("button").Click();
    
    // Assert we did not go away for example via history of NavigationManager
}

But how does that work with an <a> element? Simply, it doesn't, We have no "means of clicking" on that element. There is no onclick event handler attached. We would need someone to active the <a> element (a browser) and someone who takes care of the dispatching to the navigationmanager (aka a router), both things we do not have.

The work around to test the <a> case would be:

public void ACase()
{
    var cut = RenderComponent<MyComponent>();

    // That does the same as clicking on the <a> element
    Services.GetRequiredService<NavigationManager>().NavigateTo("/counter");
    
    // Assert we did not go away for example via history of NavigationManager
}

If we are here on the same page, I can add that to the documentation, which is anyway outstanding at the moment.

[linkdotnet]

OK, looks good so far. I'm still vacationing, but will see if I can get back to this again later today or tomorrow with a follow-up. Thanks for your patience.

Then you should stay in vacationing mode ;)

Anyway we can't merge this until somewhen in September until .net7.0 rc1 hits the public

[linkdotnet]

So I rebased onto main and targeted main.

[egil]

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

[linkdotnet]

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

[egil]

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

I think there could be cases where a test wants to see if e.g. the first attempt was blocked but not the second, and that assertion might be more clear written by looking into the history and seeing the first entry being canceled and the second not. An assertion on the number of items in the history to confirm this would not necessarily be correct, since there could be other reasons why there is no entry, e.g. the CUT didn't actually attempt a navigation.

[linkdotnet]

We could extend our NavigationHistory here as well to include whether or not a navigation attempt was completed or cancelled.

Do you have a specific use case in mind? From an user point of view shouldn't it be enough to have no entry?

I think there could be cases where a test wants to see if e.g. the first attempt was blocked but not the second, and that assertion might be more clear written by looking into the history and seeing the first entry being canceled and the second not. An assertion on the number of items in the history to confirm this would not necessarily be correct, since there could be other reasons why there is no entry, e.g. the CUT didn't actually attempt a navigation.

I get your point but still think this will create unit tests which rely on structure or implementation details rather than actual behaviour. Your use-case describes missing unit tests which cover the other cases.

[egil]

I get your point but still think this will create unit tests which rely on structure rather than actual behaviour. Your use-case describes missing unit tests which cover the other cases.

But with navigation from a component under test, the only place a user can verify the correct behavior is through the FakeNavigationManager, there is no other place where the effect of calling NavigateTo is observable as far as I can tell, if it's the component under test that triggers the navigation.

[linkdotnet]

But with navigation from a component under test, the only place a user can verify the correct behavior is through the FakeNavigationManager, there is no other place where the effect of calling NavigateTo is observable as far as I can tell, if it's the component under test that triggers the navigation.

That is true. My point is more what should a user test? For me: Effect / Outcome. With your given example, yes we can have multiple situation which lead to the same outcome, but also there are two different effects leading to the outcome. So let's take your example of first attempt blocked. You would have one test anyway which checks that the navigation was blocked. Then later you would need a setup (aka effect) which gives the expected navigation (aka non-blocking).

Both cases are testable with the current approach without any boilerplate code. I hope you can understand my motivation behind my questioning.

[egil]

I hope you can understand my motivation behind my questioning.

I think I get where you are coming from. I just have feeling that there will be users who will have a weird need to write that assertion, and will request that we add that at some point, maybe they have something crazy like a timer based navigation. It would be a breaking change adding it later, so that's also a motivation for including it now.

[linkdotnet]

I hope you can understand my motivation behind my questioning.

I think I get where you are coming from. I just have feeling that there will be users who will have a weird need to write that assertion, and will request that we add that at some point, maybe they have something crazy like a timer based navigation. It would be a breaking change adding it later, so that's also a motivation for including it now.

Then let's do it. From a design point of view I guess we will always push an entry on the NavigationHistory stack but we will introduce a new boolean flag indicating if the navigation would have been cancelled. Does that sounds reasonable to you?

[egil]

Then let's do it. From a design point of view I guess we will always push an entry on the NavigationHistory stack but we will introduce a new boolean flag indicating if the navigation would have been cancelled. Does that sounds reasonable to you?

I think we also need to actually cancel the navigation like you do in your PR here, so yeah, the flag should be something like NavigationCancelled or perhaps NavigationCompleted, if we want to be more "positive" :). For naming, I think it will be more discoverable to users if it includes the word "cancelled", as that's the feature name in Blazor.

[linkdotnet]

Then let's do it. From a design point of view I guess we will always push an entry on the NavigationHistory stack but we will introduce a new boolean flag indicating if the navigation would have been cancelled. Does that sounds reasonable to you?

I think we also need to actually cancel the navigation like you do in your PR here, so yeah, the flag should be something like NavigationCancelled or perhaps NavigationCompleted, if we want to be more "positive" :). For naming, I think it will be more discoverable to users if it includes the word "cancelled", as that's the feature name in Blazor.

I would stay with "Prevented" as this is the wording of Blazor.

[egil]

Actually, we probably want this to be an enum, since the navigation can both be canceled and failed. With inspiration from WebViewNavigationManager, we see that the call to NotifyLocationChangingAsync is wrapped in an try/catch, because that can of course fail, since that is user code.

protected override void NavigateToCore(string uri, NavigationOptions options)
{
    _ = PerformNavigationAsync();

    async Task PerformNavigationAsync()
    {
        try
        {
            var shouldContinueNavigation = await NotifyLocationChangingAsync(uri, options.HistoryEntryState, false);

            if (!shouldContinueNavigation)
            {
                Log.NavigationCanceled(_logger, uri);
                return;
            }

            _ipcSender.Navigate(uri, options);
        }
        catch (Exception ex)
        {
            // We shouldn't ever reach this since exceptions thrown from handlers are handled in HandleLocationChangingHandlerException.
            // But if some other exception gets thrown, we still want to know about it.
            Log.NavigationFailed(_logger, uri, ex);
            _ipcSender.NotifyUnhandledException(ex);
        }
    }
}

Perhaps:

public enum NavgationStatus
{
  Succeeded,
  Canceled,
  Failed
}

In the case where it fails, I also think we should record the exception in NavigationHistory.

[linkdotnet]

Quite funny.. the last comment:

// We shouldn't ever reach this since exceptions thrown from handlers are handled in HandleLocationChangingHandlerException.
// But if some other exception gets thrown, we still want to know about it.

Why do they have it then? NotifyLocationChangingAsync does handle exceptions internally. Not sure how we should distinguish between "Prevented" and "Failed".

Here the whole method:

NotifyLocationChangingAsync

protected async ValueTask<bool> NotifyLocationChangingAsync(string uri, string? state, bool isNavigationIntercepted)
    {
        _locationChangingCts?.Cancel();
        _locationChangingCts = null;

        var handlerCount = _locationChangingHandlers.Count;

        if (handlerCount == 0)
        {
            return true;
        }

        var cts = new CancellationTokenSource();

        _locationChangingCts = cts;

        var cancellationToken = cts.Token;
        var context = new LocationChangingContext
        {
            TargetLocation = uri,
            HistoryEntryState = state,
            IsNavigationIntercepted = isNavigationIntercepted,
            CancellationToken = cancellationToken,
        };

        try
        {
            if (handlerCount == 1)
            {
                var handlerTask = InvokeLocationChangingHandlerAsync(_locationChangingHandlers[0], context);

                if (handlerTask.IsFaulted)
                {
                    await handlerTask;
                    return false; // Unreachable because the previous line will throw.
                }

                if (context.DidPreventNavigation)
                {
                    return false;
                }

                if (!handlerTask.IsCompletedSuccessfully)
                {
                    await handlerTask.AsTask().WaitAsync(cancellationToken);
                }
            }
            else
            {
                var locationChangingHandlersCopy = ArrayPool<Func<LocationChangingContext, ValueTask>>.Shared.Rent(handlerCount);

                try
                {
                    _locationChangingHandlers.CopyTo(locationChangingHandlersCopy);

                    var locationChangingTasks = new HashSet<Task>();

                    for (var i = 0; i < handlerCount; i++)
                    {
                        var handlerTask = InvokeLocationChangingHandlerAsync(locationChangingHandlersCopy[i], context);

                        if (handlerTask.IsFaulted)
                        {
                            await handlerTask;
                            return false; // Unreachable because the previous line will throw.
                        }

                        if (context.DidPreventNavigation)
                        {
                            return false;
                        }

                        locationChangingTasks.Add(handlerTask.AsTask());
                    }

                    while (locationChangingTasks.Count != 0)
                    {
                        var completedHandlerTask = await Task.WhenAny(locationChangingTasks).WaitAsync(cancellationToken);

                        if (completedHandlerTask.IsFaulted)
                        {
                            await completedHandlerTask;
                            return false; // Unreachable because the previous line will throw.
                        }

                        if (context.DidPreventNavigation)
                        {
                            return false;
                        }

                        locationChangingTasks.Remove(completedHandlerTask);
                    }
                }
                finally
                {
                    ArrayPool<Func<LocationChangingContext, ValueTask>>.Shared.Return(locationChangingHandlersCopy);
                }
            }

            return !context.DidPreventNavigation;
        }
        catch (TaskCanceledException ex)
        {
            if (ex.CancellationToken == cancellationToken)
            {
                // This navigation was in progress when a successive navigation occurred.
                // We treat this as a canceled navigation.
                return false;
            }

            throw;
        }
        finally
        {
            cts.Cancel();
            cts.Dispose();

            if (_locationChangingCts == cts)
            {
                _locationChangingCts = null;
            }
        }
    }

[egil]

Your right. Its actually in the method InvokeLocationChangingHandlerAsync that HandleLocationChangingHandlerException gets called if an unhandled exceptions occurs.

In our override of HandleLocationChangingHandlerException we could just rethrow the exception, and it would bubble up to NavigateToCore which could then set status correctly.

[linkdotnet]

There are two schools of thought right now:

1. We don't handle the exception at all and bubble the exception to user code.
2. We catch the exception and save it somewhere as property on our FakeNavigationManager.

I can see reasons where people raise an exception in an handler and want to check something in the testcode. An enum would not be sufficient (enough) in this case.

If we go with version 1. we don't need a "Failed" state in our enum and less code. Version 2 would mean we have to save the exception somewhere. I personally would let the exception bubble to the user (as the normal Blazor code would do).

[egil]

If we go with version 1. we don't need a "Failed" state in our enum and less code. Version 2 would mean we have to save the exception somewhere. I personally would let the exception bubble to the user (as the normal Blazor code would do).

I would want to set the navigation state to failed, because if an exception is thrown, then the navigation is canceled in Blazor and a failure is logged. Then I would store the exception in the NavgationHistory object for that particular navigation event, in its own property that is null by default.

Perhaps something like this:

public NavigationStatus Status { get; }
public string? State { get; }
public Exception? Exception { get; }

In Blazor WASM, an exception is just logged (https://source.dot.net/#Microsoft.AspNetCore.Components.WebAssembly/Services/WebAssemblyNavigationManager.cs,85), in Blazor Server, an exception is passed to an UnhandledException event handler (https://source.dot.net/#Microsoft.AspNetCore.Components.Server/Circuits/RemoteNavigationManager.cs,120). Either way, I do not think user code/component code will see that exception, so I think its safe for us to just do what Blazor WASM does, i.e. "log" it in the NavgationHistory.

[linkdotnet]

What is public string? State { get; } for?

[egil]

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

[linkdotnet]

I added the exception handling as well (including tests).

The biggest struggle I had with XML-Docs and scoping them to the right DOTNET version You can't use #if NET... in xml comments. Also adding <param> for elements which are not existing will result in compiler error. I had to duplicate some of the xml stuff... but it is working now.

Still not compilable (at least on github)... if you want you can checkout the code and download the nightly builds... that would work

[linkdotnet]

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

[egil]

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

You could argue the gain is that State and Exception are available at the same "level", next to Status, which would be where I would expect it to be if it was also not attached to Options`.

But that also seem a bit overdone, perhaps lets leave it out for now and see if users are confused.

[linkdotnet]

What is public string? State { get; } for?

Ahh forgot that NavigationOptions Options { get; } has the state. It seems redundant to add it as a property directly on NavigationHistory as well and just point it to Options.HistoryEntryState?

Yes exactly. We could do public string? State => Options.HistoryEntryState but I am not sure what we gain from that.

You could argue the gain is that State and Exception are available at the same "level", next to Status, which would be where I would expect it to be if it was also not attached to Options`.

But that also seem a bit overdone, perhaps lets leave it out for now and see if users are confused.

Yes. I like that. If we add it preemptively without need we also have to maintain that in case of changes.

[linkdotnet]

I added changelog as well as docu